Re: [PATCH] fbdev: ssd1307fb: defer I2C transfers from damage callbacks

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Concurrent writes during device removal schedule deferred work after cleanup, leading to a NULL pointer dereference in the workqueue.

Pre-existing issues:
- [Critical] The framebuffer screen memory is freed prematurely in the remove path, allowing concurrent writes to corrupt freed pages.
--

--- Patch [1]: [PATCH] fbdev: ssd1307fb: defer I2C transfers from damage callbacks ---
commit 34ea34fa043e51cd59d74662902a40b400c4fb97
Author: Hui Su <sh_def@163.com>

fbdev: ssd1307fb: defer I2C transfers from damage callbacks

This commit tracks damage in the driver's private data and merges multiple
updates into a bounding rectangle. It queues the deferred I/O work
immediately for damage reported by fbdev drawing and write helpers to avoid
entering the sleeping I2C transfer path from atomic context.

> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index 644b8d97b3811..c4fdecafd8560 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c
[ ... ]
> @@ -302,19 +310,49 @@ static int ssd1307fb_blank(int blank_mode, struct fb_info *info)
>  		return ssd1307fb_write_cmd(par->client, SSD1307FB_DISPLAY_ON);
>  }
>  
> +static void ssd1307fb_schedule_damage(struct fb_info *info, u32 x, u32 y,
> +				      u32 width, u32 height)
> +{
> +	struct ssd1307fb_par *par = info->par;
> +	unsigned long flags;
> +	u32 x2, y2;
[ ... ]
> +	spin_unlock_irqrestore(&par->damage_lock, flags);
> +
> +	/* Advance an already-pending mmap update as well. */
> +	mod_delayed_work(system_wq, &info->deferred_work, 0);

[Severity: Critical]
Could this introduce a regression by unconditionally queuing the deferred
work after the driver's remove function has already cleaned it up?

If a userspace program is executing a write() that blocks in fb_sys_write()
during copy_from_user(), and the driver is concurrently unbound:

ssd1307fb_remove()
  unregister_framebuffer()
  fb_deferred_io_cleanup()
    info->fbdefio_state = NULL;

When the blocked write() resumes and completes, the driver wrapper invokes
ssd1307fb_schedule_damage(), calling mod_delayed_work() here. When the
work executes, fb_deferred_io_work() will attempt to lock the NULL
fbdefio_state, leading to a crash.

> +}

[ ... ]

While reviewing the above, I also noticed a related issue in the remove path:

> static void ssd1307fb_remove(struct i2c_client *client)
> {
[ ... ]
> 	fb_deferred_io_cleanup(info);
> 	__free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len));

[Severity: Critical]
This is a pre-existing issue, but can this lead to a use-after-free of the
screen memory?

If a concurrent write() is blocked in fb_sys_write() during copy_from_user(),
ssd1307fb_remove() frees the screen buffer pages here without waiting for
active file operations to conclude.

When the blocked write() resumes, copy_from_user() would write the
user-provided data directly into the newly freed physical pages. Should
this resource cleanup be deferred to an .fb_destroy() callback instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803183957.3481000-1-sh_def@163.com?part=1