Re: [PATCH v2 19/40] fbdev: defio: assert FBINFO_VIRTFB, drop VM_IO, add VM_MIXEDMAP

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver passes a virtual address to __free_pages(), causing an out-of-bounds pointer dereference and kernel panic.
- [High] Ignored return value of fb_deferred_io_init() leads to NULL pointer dereferences in mmap, open, and cleanup error paths.
- [High] Missing .driver_data initialization in i2c_device_id array leads to NULL pointer dereference on sysfs instantiation.
--

commit d46565518c57c096a66d45b42bbe529fae5740d7
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>

fbdev: defio: assert FBINFO_VIRTFB, drop VM_IO, add VM_MIXEDMAP

Currently all drivers which use defio allocate system memory. All of them
also set FBINFO_VIRTFB, other than ssd1307fb, however this driver allocates
system RAM, so simply failed to set this flag when it ought to.

> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index c4fdecafd8560..958514a354338 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c
> @@ -763,6 +763,8 @@ static int ssd1307fb_probe(struct i2c_client *client)
>  	info->fix.smem_start = __pa(vmem);
>  	info->fix.smem_len = vmem_size;
>  
> +	info->flags = FBINFO_VIRTFB;
> +
>  	fb_deferred_io_init(info);

[Severity: High]
This isn't a bug introduced by this patch, but if memory allocation fails
in fb_deferred_io_init(), it returns -ENOMEM but ssd1307fb_probe() ignores
it, leaving info->fbdefio_state as NULL and info->deferred_work
uninitialized.

Subsequent probe failure or device removal calls fb_deferred_io_cleanup(),
which can unconditionally call flush_delayed_work() on the uninitialized
workqueue and then dereference the NULL fbdefio_state:

drivers/video/fbdev/core/fb_defio.c:fb_deferred_io_cleanup() {
	...
	mutex_lock(&fbdefio_state->lock);
	...
}

Additionally, userspace calling open() or mmap() could trigger NULL pointer
dereferences in fb_deferred_io_open() and fb_deferred_io_mmap().

Does the ignored return value of fb_deferred_io_init() lead to NULL pointer
dereferences in mmap, open, and cleanup error paths?

>  
>  	i2c_set_clientdata(client, info);

[Severity: Critical]
This is a pre-existing issue, but the driver allocates video memory using
__get_free_pages(), which returns a virtual address (void *).

During probe failure, it passes this virtual address directly to
__free_pages():

drivers/video/fbdev/ssd1307fb.c:ssd1307fb_probe() {
	...
fb_defio_error:
	__free_pages(vmem, get_order(vmem_size));
	...
}

And during module unload, it does the same:

drivers/video/fbdev/ssd1307fb.c:ssd1307fb_remove() {
	...
	__free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));
	...
}

Because C implicitly casts void * to struct page *, the kernel could treat
the zeroed video memory as a page struct, reading garbage PFNs, corrupting
the buddy allocator state, and crashing.

Does passing a virtual address to __free_pages() in these paths cause an
out-of-bounds pointer dereference and kernel panic? Should this be using
free_pages() instead?


[Severity: High]
This isn't a bug introduced by this patch, but if the driver is instantiated
via the legacy I2C sysfs interface, device_get_match_data() falls back to
matching against ssd1307fb_i2c_id:

drivers/video/fbdev/ssd1307fb.c:ssd1307fb_i2c_id[] = {
	{ .name = "ssd1305fb" },
	...
};

Because entries in this array omit the .driver_data field, it defaults to
NULL. The probe function then unconditionally dereferences this NULL pointer:

drivers/video/fbdev/ssd1307fb.c:ssd1307fb_probe() {
	...
	par->vcomh = par->device_info->default_vcomh;
	...
}

Does the missing .driver_data initialization in the i2c_device_id array
lead to a NULL pointer dereference on sysfs instantiation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-b4-mmap-prepare-vma-flag-sanify-v2-0-7d9781ed5361@kernel.org?part=19