Re: [PATCH v2 1/2] fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Uninitialized variable `ret` returned when `pages == 0` and `page_address_mode` is enabled.

Pre-existing issues:
- [High] Passing a virtual address to `__free_pages()` instead of a `struct page *`.
- [High] Use-After-Free of physical framebuffer memory due to unrevoked mmap mappings on device removal.
--

--- Patch [1]: [PATCH v2 1/2] fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller ---
commit e1d9663a6aba54b57c6911f220e7fb969872972f
Author: Jakub Turek <jakub.turek@elsta.tech>

fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller

The SH1107 display controller is very similar to SSD1307 but the main
difference is that SH1107 has different memory layout and uses page mode
adressation only.

> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index 644b8d97b3811..19dc7671fa7ac 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c

[ ... ]

> @@ -274,13 +308,28 @@ static int ssd1307fb_update_rect(struct ssd1307fb_par *par, unsigned int x,
>  				u8 byte = vmem[(8 * i + k) * line_length +
>  					       j / 8];
>  				u8 bit = (byte >> (j % 8)) & 1;
> +
>  				data |= bit << k;
>  			}
>  			array->data[array_idx++] = data;
>  		}
> +
> +		if (par->page_address_mode) {
> +			ret = ssd1307fb_set_page_pos(par, par->page_offset + i,
> +						     par->col_offset + x);
> +			if (ret < 0)
> +				goto out_free;
> +
> +			ret = ssd1307fb_write_array(par->client, array, width);
> +			if (ret < 0)
> +				goto out_free;
> +
> +			array_idx = 0;
> +		}
>  	}
>  
> -	ret = ssd1307fb_write_array(par->client, array, width * pages);
> +	if (!par->page_address_mode)
> +		ret = ssd1307fb_write_array(par->client, array, width * pages);
>  
>  out_free:
>  	kfree(array);
>  	return ret;

[Severity: Medium]
Can this return an uninitialized variable from ssd1307fb_update_rect() in
drivers/video/fbdev/ssd1307fb.c?

When the rectangle has a height of 0 and y % 8 is 0, the calculated pages
evaluates to 0. If par->page_address_mode is true, both of the
if (!par->page_address_mode) blocks and the for loop are skipped entirely.

This leaves ret completely uninitialized. Is it possible for
ssd1307fb_defio_fillrect to call the __damage_area callback even if
sys_fillrect aborted on a 0-height rectangle, returning uninitialized memory?

[ ... ]

> @@ -658,6 +722,7 @@ static int ssd1307fb_probe(struct i2c_client *client)
>  
>  	par->contrast = 127;
>  	par->vcomh = par->device_info->default_vcomh;
> +	par->page_address_mode = par->device_info->page_mode_only;
>  
>  	/* Setup display timing */
>  	if (device_property_read_u32(dev, "solomon,dclk-div", &par->dclk_div))

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path in
drivers/video/fbdev/ssd1307fb.c pass a virtual address to a function expecting
a struct page pointer?

In ssd1307fb_probe():

        vmem = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
                                        get_order(vmem_size));
...
fb_defio_error:
        __free_pages(vmem, get_order(vmem_size));

Since __get_free_pages() returns a virtual address, passing it to
__free_pages() (which expects a struct page pointer) will misinterpret the
virtual address as a page descriptor. Could this cause memory corruption or a
kernel panic when manipulating buddy allocator lists?

A similar pattern appears in ssd1307fb_remove():

        __free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));

[Severity: High]
This is a pre-existing issue, but does removing the device cause a physical
memory use-after-free if userspace has an active mmap?

During device unbinding in ssd1307fb_remove() in
drivers/video/fbdev/ssd1307fb.c, the driver frees the physical framebuffer
memory:

        __free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));

If an unprivileged userspace process mapped the device memory via mmap, the
page table entries are never shot down using unmap_mapping_range(). Does
this allow userspace to retain read/write access to physical pages that have
been returned to the kernel allocator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-feat-sh1107-v2-0-0d2893e44ebd@elsta.tech?part=1