Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Module unloading fails to clean up the internal Topcat framebuffer, leading to a Use-After-Free. - [High] Global static structures prevent supporting multiple Topcat devices, leading to state and data corruption. -- commit 27805523b119df6cf717d257600582e127002d74 Author: Haoxiang Li <[email protected]> video: hpfb: Unregister DIO driver on init failure hpfb_init() registers the DIO driver via dio_register_driver(). If a later error occurs, the function returns directly without unregistering the DIO driver. Unregister the DIO driver before returning from these error paths. > diff --git a/drivers/video/fbdev/hpfb.c b/drivers/video/fbdev/hpfb.c > index a1144b1509826..1405712e5b4ff 100644 > --- a/drivers/video/fbdev/hpfb.c > +++ b/drivers/video/fbdev/hpfb.c > @@ -407,10 +407,13 @@ static int __init hpfb_init(void) > err = copy_from_kernel_nofault(&i, (unsigned char *)INTFBVADDR + > DIO_IDOFF, 1); > > if (!err && (i == DIO_ID_FBUFFER) && topcat_sid_ok(sid = > DIO_SECID(INTFBVADDR))) { > - if (!request_mem_region(INTFBPADDR, DIO_DEVSIZE, "Internal > Topcat")) > + if (!request_mem_region(INTFBPADDR, DIO_DEVSIZE, "Internal > Topcat")) { > + dio_unregister_driver(&hpfb_driver); > return -EBUSY; > + } > printk(KERN_INFO "Internal Topcat found (secondary id %02x)\n", > sid); > if (hpfb_init_one(INTFBPADDR, INTFBVADDR)) { [Severity: High] This isn't a bug introduced by this patch, but since we are modifying the initialization path, does hpfb_init_one() safely handle multiple Topcat devices? The driver uses statically allocated global variables (fb_info, fb_regs, fb_bitmask) to manage device state, and lacks safeguards against multiple device probes. If a system contains multiple Topcat devices (e.g., an internal Topcat and a DIO Topcat, or multiple DIO cards), hpfb_init_one() will execute multiple times: drivers/video/fbdev/hpfb.c:hpfb_init_one() { ... fb_regs = virt_base; fboff = (in_8(fb_regs + HPFB_FBOMSB) << 8) | in_8(fb_regs + HPFB_FBOLSB); ... } Subsequent calls will blindly overwrite the global fb_regs and call register_framebuffer() again on the exact same fb_info pointer. Could this corrupt the kernel's framebuffer linked lists and redirect all subsequent I/O hardware operations for the first device into the second device's registers? > + dio_unregister_driver(&hpfb_driver); > return -ENOMEM; > } > } [Severity: High] This is a pre-existing issue, but related to the internal Topcat initialized here, what cleans up the internal Topcat when the module is unloaded? Looking at hpfb_cleanup_module(): drivers/video/fbdev/hpfb.c:hpfb_cleanup_module() { dio_unregister_driver(&hpfb_driver); } It unregisters the DIO driver but completely neglects the internal Topcat which is initialized directly in hpfb_init(), bypassing the DIO driver core. It fails to call unregister_framebuffer(), iounmap(), or fb_dealloc_cmap(). If the internal Topcat was initialized, its fb_info structure remains actively registered in the kernel's framebuffer core after module unload. Because fb_info is a statically allocated global variable within the module, unloading the module frees its memory. Will any subsequent access to the framebuffer (e.g., via /dev/fb0 or sysfs) dereference this freed memory and lead to a Use-After-Free? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
