Re: [PATCH] fbdev: bitblit: bound-check glyph index in bit_cursor()

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