Re: [RFC PATCH] fbdev: maxinefb: fix build issues
From: Randy Dunlap <hidden>
Date: 2026-08-10 00:27:56
Also in:
dri-devel, linux-mips, sashiko-reviews
[adding Cc:s] On 8/9/26 5:02 PM, sashiko-bot@kernel.org wrote:
quoted hunk
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] Initialization function returns positive value `1` on failure, breaking module load semantics and causing subsequent `registered_fb` array corruption on unload. - [High] `maxinefb_fix.smem_start` is initialized with a virtual address (`CKSEG1ADDR`) instead of the hardware physical address, breaking user-space `mmap()`. ----- Patch [1]: [RFC PATCH] fbdev: maxinefb: fix build issues ---commit e391418fa2f21f8c420eccb513c031674ef184c6 Author: Randy Dunlap [off-list ref] fbdev: maxinefb: fix build issues This commit fixes build errors and warnings for the maxinefb driver on 32-bit and 64-bit MACH_DECSTATION. It updates KSEG1ADDR to CKSEG1ADDR and adds the static keyword to missing prototypes.quoted
diff --git a/drivers/video/fbdev/maxinefb.c b/drivers/video/fbdev/maxinefb.c index 52528eb4dfb41..8aa1389e2d4c8 100644 --- a/drivers/video/fbdev/maxinefb.c +++ b/drivers/video/fbdev/maxinefb.c[ ... ]quoted
@@ -111,7 +98,7 @@ static const struct fb_ops maxinefb_ops = { .fb_setcolreg = maxinefb_setcolreg, }; -int __init maxinefb_init(void) +static int __init maxinefb_init(void)[Severity: High] This is a pre-existing issue, but does returning 1 on failure in this function cause issues with module load semantics? In maxinefb_init(): if (register_framebuffer(&fb_info) < 0) return 1; If register_framebuffer() fails, returning a positive integer causes the kernel to treat the module as successfully loaded because the module init framework expects negative error codes for failure.
Ack. I'll change that.
Later, when the module is unloaded, maxinefb_exit() calls unregister_framebuffer() on the unregistered fb_info. Since it was never successfully registered, its node field is 0. Does this cause the generic do_unregister_framebuffer() to blindly execute registered_fb[0] = NULL, corrupting the kernel's framebuffer state by deleting the reference to whatever valid framebuffer occupied node 0?quoted
diff --git a/include/video/maxinefb.h b/include/video/maxinefb.h index 6aeb4acca2bdb..2b66b32f355cf 100644 --- a/include/video/maxinefb.h +++ b/include/video/maxinefb.h[ ... ]quoted
@@ -13,13 +13,13 @@ /* * IMS332 video controller register base address */ -#define MAXINEFB_IMS332_ADDRESS KSEG1ADDR(0x1c140000) +#define MAXINEFB_IMS332_ADDRESS CKSEG1ADDR(0x1c140000) /* * Begin of DECstation 5000/xx onboard framebuffer memory, default resolution * is 1024x768x8 */ -#define DS5000_xx_ONBOARD_FBMEM_START KSEG1ADDR(0x0a000000) +#define DS5000_xx_ONBOARD_FBMEM_START CKSEG1ADDR(0x0a000000)
Maciej or anyone: can you comment of this, please?
[Severity: High]
This is a pre-existing issue, but does assigning a virtual address to
maxinefb_fix.smem_start break user-space mmap()?
In maxinefb_init():
fb_start = DS5000_xx_ONBOARD_FBMEM_START;
...
maxinefb_fix.smem_start = fb_start;
The smem_start field is passed to vm_iomap_memory() by the generic framebuffer
mmap handler (fb_mmap()). Because vm_iomap_memory() strictly expects a physical
address, does passing a KSEG1 virtual address set up user-space page tables
pointing to an invalid physical address?
This could trigger an invalid physical memory access and a kernel panic when
user-space accesses the mapped memory.-- ~Randy