Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Incorrect check of pcim_request_region() return value treats success as failure. - [High] The error path for a failed MMIO region request jumps to a cleanup label that calls `framebuffer_release()` with an uninitialized `info` pointer. --
--- Patch [1]: [PATCH 1/5] fbdev: aty128fb: Convert to managed PCI and ioremap API --- commit 1e12b2171d8c5fcb82cbff1de0aa1745b4062be0 Author: Shixiong Ou <[email protected]> fbdev: aty128fb: Convert to managed PCI and ioremap API Fix missing pci_disable_device() in probe and remove. Use pcim_enable_device(), pcim_request_region(), devm_ioremap() and devm_ioremap_wc() to replace manual resource management. > diff --git a/drivers/video/fbdev/aty/aty128fb.c > b/drivers/video/fbdev/aty/aty128fb.c > index bcb10e66221c8..b76bfe014f41b 100644 > --- a/drivers/video/fbdev/aty/aty128fb.c > +++ b/drivers/video/fbdev/aty/aty128fb.c [ ... ] > fb_addr = pci_resource_start(pdev, 0); > - if (!request_mem_region(fb_addr, pci_resource_len(pdev, 0), > - "aty128fb FB")) { > + if (!pcim_request_region(pdev, 0, "aty128fb FB")) { [Severity: High] Since pcim_request_region() returns 0 on success and a negative error code on failure, does checking it with '!' cause the driver to fail probe when the region is actually successfully requested? > printk(KERN_ERR "aty128fb: cannot reserve frame " > "buffer memory\n"); > return -ENODEV; > } > > reg_addr = pci_resource_start(pdev, 2); > - if (!request_mem_region(reg_addr, pci_resource_len(pdev, 2), > - "aty128fb MMIO")) { > + if (!pcim_request_region(pdev, 2, "aty128fb MMIO")) { [Severity: High] Does this have the same pcim_request_region() return value check issue as the previous block? > printk(KERN_ERR "aty128fb: cannot reserve MMIO region\n"); > - goto err_free_fb; > + goto err_free_info; > } > > /* We have the resources. Now virtualize them */ > info = framebuffer_alloc(sizeof(struct aty128fb_par), &pdev->dev); [Severity: High] If the above goto is taken, the code jumps to err_free_info which calls framebuffer_release(info). Since info hasn't been allocated or initialized yet at that point, will this dereference an uninitialized pointer during the cleanup path? [ ... ] > err_free_info: > framebuffer_release(info); > -err_free_mmio: > - release_mem_region(pci_resource_start(pdev, 2), > - pci_resource_len(pdev, 2)); > -err_free_fb: > - release_mem_region(pci_resource_start(pdev, 0), > - pci_resource_len(pdev, 0)); > return -ENODEV; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
