Re: [PATCH] video: hpfb: Unregister DIO driver on init failure

[sashiko-bot@kernel.org]

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 <haoxiang_li2024@163.com>

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/20260622064915.767194-1-haoxiang_li2024@163.com?part=1