Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The bounds checking for glyph indices is missing in software rotation paths (fbcon_ccw.c, fbcon_cw.c, fbcon_ud.c), leaving them vulnerable to out-of-bounds heap reads. -- --- Patch [1]: [PATCH] fbdev: bitblit: bound-check glyph index in bit_cursor() --- commit eb8c6b0049962d4bc836a2579e035e53104adf59 Author: Rik van Riel <[email protected]> fbdev: bitblit: bound-check glyph index in bit_cursor() This commit adds bounds checking for the glyph index in bit_cursor() to prevent out-of-bounds reads when fetching glyphs. It addresses cases where the screen buffer contains values larger than the current font's glyph count, which can happen after a font resize or via malicious writes to /dev/vcsa. Link: https://lore.kernel.org/all/[email protected]/ Closes: https://syzkaller.appspot.com/bug?extid=61b1db46218109869c14 > diff --git a/drivers/video/fbdev/core/bitblit.c > b/drivers/video/fbdev/core/bitblit.c > index 65681dcc5930d..39f44258d793f 100644 > --- a/drivers/video/fbdev/core/bitblit.c > +++ b/drivers/video/fbdev/core/bitblit.c > @@ -273,9 +273,14 @@ static void bit_cursor(struct vc_data *vc, struct > fb_info *info, bool enable, > if (!vc->vc_font.data) > return; > > - c = scr_readw((u16 *) vc->vc_pos); > + c = scr_readw((u16 *) vc->vc_pos); > attribute = get_attribute(info, c); > - src = vc->vc_font.data + ((c & charmask) * (w * vc->vc_font.height)); > + c &= charmask; > + > + /* Clamp to font size, same as bit_putcs_aligned() */ > + if (c >= vc->vc_font.charcount) > + c = 0; > + src = vc->vc_font.data + (c * (w * vc->vc_font.height)); [Severity: High] This isn't a bug introduced by this patch, but while checking the bounds for the glyph index in bitblit.c is a great fix, could similar out-of-bounds reads still occur in the software rotation paths? Looking at the rotation code (fbcon_ccw.c, fbcon_cw.c, fbcon_ud.c), the bounds checking appears to be missing entirely. For example, in fbcon_ccw.c: drivers/video/fbdev/core/fbcon_ccw.c:ccw_cursor() { ... c = scr_readw((u16 *) vc->vc_pos); attribute = get_attribute(info, c); src = par->rotated.buf + ((c & charmask) * (w * vc->vc_font.width)); ... } And in the putcs equivalents: drivers/video/fbdev/core/fbcon_ccw.c:ccw_putcs_aligned() { ... src = par->rotated.buf + (scr_readw(s--) & charmask) * cellsize; ... } If a user loads a custom font with fewer than 256 or 512 glyphs (e.g., 100 glyphs), the rotation buffer par->rotated.buf is sized dynamically based on charcount. If they then write an arbitrary 16-bit character index (e.g., 255) to the screen buffer via /dev/vcsa using vcs_write(), these rotated blit functions will mask the index with 0xff or 0x1ff and directly use it to index par->rotated.buf. Without clamping against vc->vc_font.charcount, wouldn't this result in reading past the end of the heap-allocated rotation buffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260807221956.4e9dde5c@fangorn?part=1
