Re: [PATCH] fbdev: protect mode sysfs reads 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] The patch fails to resolve the local kernel Use-After-Free because it misses lockless traversal and assignment in `store_mode()`.
- [High] Unprivileged local DoS via WARN_ON_ONCE triggered by snprintf size underflow in show_modes().
--

commit 781c0f45188865f8a46896496380e20ca20b9af8
Author: Melbin K Mathew <mlbnkm1@gmail.com>

fbdev: protect mode sysfs reads with lock_fb_info()

This commit adds lock_fb_info() around mode sysfs reads to protect
against concurrent modifications to the modelist by store_modes().

> diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> index ea196603c7a87..6bdb25f7be018 100644
> --- a/drivers/video/fbdev/core/fbsysfs.c
> +++ b/drivers/video/fbdev/core/fbsysfs.c

[ ... ]

> @@ -82,11 +82,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;
> +		have_mode = true;
> +	}
> +	unlock_fb_info(fb_info);

[Severity: High]
This is a pre-existing issue, but does store_mode() also need similar locking
protection to fully prevent the use-after-free?

While this patch protects show_mode() and show_modes(), store_mode() still
traverses the modelist locklessly:

drivers/video/fbdev/core/fbsysfs.c:store_mode() {
    ...
    list_for_each_entry(modelist, &fb_info->modelist, list) {
    ...
}

If store_modes() concurrently clears and frees the modelist, could
store_mode() traverse freed list elements?

Additionally, store_mode() locklessly assigns the mode pointer:

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

If store_modes() freed the old modelist in the interim, could this resurrect
a dangling pointer into fb_info->mode and bypass the fix for subsequent
readers?

[ ... ]

> @@ -134,10 +143,13 @@ 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) {
>  		mode = &modelist->mode;
>  		i += mode_string(buf, i, mode);
>  	}

[Severity: High]
This is a pre-existing issue, but can this loop trigger a WARN_ON_ONCE
due to integer underflow if the modelist is large?

If the accumulated length in i exceeds PAGE_SIZE (for example, with a very
large modelist), the size calculation in mode_string() appears to underflow:

drivers/video/fbdev/core/fbsysfs.c:mode_string() {
    ...
    return snprintf(&buf[offset], PAGE_SIZE - offset, "%c:%dx%d%c-%d\n",
                    m, mode->xres, mode->yres, v, mode->refresh);
}

Since offset is unsigned, PAGE_SIZE - offset would become a massive positive
value, which vsnprintf() detects as exceeding INT_MAX and triggers a
WARN_ON_ONCE. Could an unprivileged local user trigger a kernel panic on
systems with panic_on_warn=1 by repeatedly reading the modes sysfs file?

> +	unlock_fb_info(fb_info);
> +
>  	return i;
>  }
>

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