Re: [PATCH v3 1/3] fbdev: bound mode sysfs output to the sysfs buffer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unprotected value copy of large `fb_info->var` struct leads to torn reads and state corruption in sysfs store functions.
--

commit 03248859e392cd00678961d93e5c27717ac23498
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: bound mode sysfs output to the sysfs buffer

This commit updates mode_string() to take a size parameter and use scnprintf()
to prevent buffer overruns when generating sysfs output. It also modifies
show_modes() to stop accumulating mode strings once the sysfs buffer is full.

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index ea196603c7a87..af21dc5052df3 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c
[ ... ]
> @@ -64,7 +67,7 @@ static ssize_t store_mode(struct device *device, struct device_attribute *attr,
>  
>  	list_for_each_entry(modelist, &fb_info->modelist, list) {
>  		mode = &modelist->mode;
> -		i = mode_string(mstr, 0, mode);
> +		i = mode_string(mstr, sizeof(mstr), 0, mode);
>  		if (strncmp(mstr, buf, max(count, i)) == 0) {
>  
>  			var = fb_info->var;

[Severity: High]
This is a pre-existing issue, but seeing the struct copy here prompted a
question about other sysfs store functions in this file.

In store_mode(), var = fb_info->var; is protected by lock_fb_info(fb_info)
acquired earlier in the function. However, other store functions in this file
perform this same large struct copy locklessly before acquiring the lock:

drivers/video/fbdev/core/fbsysfs.c:store_bpp() {
    ...
    var = fb_info->var;
    ...
}

Similar unprotected copies occur in store_rotate(), store_virtual(), and
store_pan().

Can these unprotected copies lead to torn reads if executed concurrently with
an ioctl or another sysfs write that modifies the display state?

If a torn read occurs and a single field is updated before passing the torn
structure to activate(), could this commit corrupted state back to the
hardware?

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