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 <[email protected]> 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
