Re: [PATCH v3 3/3] fbdev: serialize mode sysfs access with lock_fb_info()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] show_modes() iterates fb_info->modelist without holding console_lock(), which is required to protect against concurrent list modifications by fbcon.
- [High] Sysfs store handlers (store_bpp, store_rotate, store_virtual, store_pan, store_blank) bypass lock_fb_info(), risking torn reads and concurrent driver execution.
--

commit 52388e2c8c0d0f6bb72f9bedf44a38f798ee348d
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: serialize mode sysfs access with lock_fb_info()

show_mode(), show_modes(), and store_mode() access fb_info->modelist
and fb_info->mode without holding lock_fb_info(). This commit adds
lock_fb_info() in these functions to serialize with store_modes() and
avoid data races.

Cc: stable@vger.kernel.org

> 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

[ ... ]

> @@ -137,12 +164,15 @@ static ssize_t show_modes(struct device *device, struct device_attribute *attr,
>  	const struct fb_videomode *mode;
>  
>  	i = 0;
> +	lock_fb_info(fb_info);
>  	list_for_each_entry(modelist, &fb_info->modelist, list) {

[Severity: High]
This is a pre-existing issue, but show_modes() iterates fb_info->modelist
without holding console_lock(), which is required to protect against
concurrent list modifications by fbcon.

Can concurrent VT switches call fbcon_switch() -> fb_set_var() ->
fb_add_videomode() and modify fb_info->modelist under console_lock()
without holding lock_fb_info()? Would an unprivileged user reading
this sysfs attribute risk a torn pointer or out-of-bounds access
since console_lock() is missing here?

>  		mode = &modelist->mode;
>  		i += mode_string(buf, PAGE_SIZE, i, mode);
>  		if (i >= PAGE_SIZE - 1)
>  			break;
>  	}
> +	unlock_fb_info(fb_info);
> +
>  	return i;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but other sysfs store handlers
(store_bpp, store_rotate, store_virtual, store_pan, store_blank) bypass
lock_fb_info(), risking torn reads and concurrent driver execution.

For example, in store_bpp(), store_rotate(), and store_virtual(),
fb_info->var is read locklessly before activating it. In store_pan()
and store_blank(), driver functions like fb_pan_display() or
fb_blank_from_user() are invoked without taking lock_fb_info().

Does this bypass the mutual exclusion that drivers rely on in the
equivalent ioctl paths? Could this result in applying corrupt
configurations to hardware or concurrent execution of non-reentrant
driver code?

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