Re: [PATCH v2 2/2] fbdev: serialize mode sysfs access with lock_fb_info()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] fb_info->mode can become a dangling pointer if the active mode is deleted via ioctl(FBIOPUT_VSCREENINFO) with FB_ACTIVATE_INV_MODE, leading to a Use-After-Free in show_mode().
--

commit 243c82e865602245e21ae16b7cdd7738ccfa6a53
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: serialize mode sysfs access with lock_fb_info()

Take lock_fb_info() in show_mode(), show_modes(), and store_mode() to
serialize with store_modes(). In show_mode(), copy the mode to the
stack and format after dropping the lock. In store_mode(), split
activate() into a _locked variant to avoid double-locking.

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index af21dc5052df3..d3d60c555bb86 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c

[ ... ]

> @@ -85,11 +103,20 @@ static ssize_t show_mode(struct device *device, struct device_attribute *attr,
>  			 char *buf)
>  {
>  	struct fb_info *fb_info = dev_get_drvdata(device);
> +	struct fb_videomode mode;
> +	bool have_mode = false;
>  
> -	if (!fb_info->mode)
> +	lock_fb_info(fb_info);
> +	if (fb_info->mode) {
> +		mode = *fb_info->mode;

[Severity: High]
This is a pre-existing issue, but can fb_info->mode become a dangling pointer
and lead to a use-after-free here?

If the active mode is deleted via ioctl(FBIOPUT_VSCREENINFO) with
FB_ACTIVATE_INV_MODE, it appears fb_delete_videomode() unlinks and frees
the mode node:

drivers/video/fbdev/core/modedb.c:fb_delete_videomode() {
    ...
    list_del(pos);
    kfree(pos);
    ...
}

However, it does not seem to clear the fb_info->mode reference. If a user
subsequently reads /sys/class/graphics/fb0/mode, would this dereference the
freed memory even while holding the lock?

> +		have_mode = true;
> +	}
> +	unlock_fb_info(fb_info);
> +
> +	if (!have_mode)
>  		return 0;
>  
> -	return mode_string(buf, PAGE_SIZE, 0, fb_info->mode);
> +	return mode_string(buf, PAGE_SIZE, 0, &mode);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701221757.231490-1-mlbnkm1@gmail.com?part=2