Re: [RFC PATCH] fbdev: maxinefb: fix build issues

[adding Cc:s]

On 8/9/26 5:02 PM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.
> 
>> 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
> [ ... ]
>> @@ -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?
> 
>> 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
> [ ... ]
>> @@ -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