Re: [PATCH] fbdev: Fix fb_new_modelist to prevent null-ptr-deref in fb_videomode_to_var

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Buffer overflow in `show_modes()` due to unbounded accumulation of offset.
--

commit c6eace86a62190121a8b0640238eca3a23f8495d
Author: Ian Bridges <icb@fastmail.org>

fbdev: Fix fb_new_modelist to prevent null-ptr-deref in fb_videomode_to_var

This commit fixes a null pointer dereference in fb_videomode_to_var by
ensuring that the current mode is kept in the modelist during
fb_new_modelist() if it is not already present.

> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index e5221653ec2b9..2f1c56e5a7a23 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c

[ ... ]

> @@ -767,6 +767,18 @@ int fb_new_modelist(struct fb_info *info)
>  	if (list_empty(&info->modelist))
>  		return 1;
>  
> +	/*
> +	 * The new modelist may not contain the current mode (info->var), and
> +	 * fbcon_new_modelist() below only re-points consoles mapped to this
> +	 * framebuffer. Add the current mode here so info->var keeps a match
> +	 * even when fbcon is unbound.
> +	 */

[Severity: High]
This is a pre-existing issue, but while reviewing how modelists are populated
here, I noticed an older problem when the modelist is read from sysfs.

Does this code overflow the sysfs buffer on the kernel heap?

If a driver populates the framebuffer's modelist with many entries, or root
writes them via the store_modes sysfs interface, the offset i accumulates
the return value of mode_string() continuously in show_modes():

drivers/video/fbdev/core/fbsysfs.c:show_modes() {
    ...
    i += mode_string(buf, i, mode);
    ...
}

The mode_string() function passes this offset directly to snprintf:

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);
}

Because snprintf returns the number of characters that would have been
written regardless of truncation, the offset will eventually exceed PAGE_SIZE
if there are enough modes. 

When offset is greater than PAGE_SIZE, the expression PAGE_SIZE - offset
integer-underflows to a massive positive size_t value, causing snprintf
to write past the 1-page sysfs buffer allocation.

> +	if (!fb_match_mode(&info->var, &info->modelist)) {
> +		fb_var_to_videomode(&mode, &info->var);
> +		if (fb_add_videomode(&mode, &info->modelist))
> +			return 1;
> +	}
> +
>  	fbcon_new_modelist(info);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ajyq2Fr-2fMfftGC@dev?part=1