Re: [Patch 3/9]: Replace scr_readw/writew by scr_readg/writeg, etc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The loop evaluating screen blankness in fbcon_prepare_logo increments the pointer incorrectly, leading to overlapping/unaligned 32-bit glyph reads and an out-of-bounds read.
- [High] Incorrect pointer arithmetic in fbcon_switch when scrolling the logo leads to severe screen buffer corruption in 32-bit glyph mode.
--

commit 161dfd956a8c1063b40c6d0ff761c9b6540eff43
Author: Alan Mackenzie <acm@muc.de>

Replace scr_readw/writew by scr_readg/writeg, etc

This patch updates vt and fbcon drivers to handle 32-bit glyphs by
replacing 16-bit specific scr_readw/writew with scr_readg/writeg,
adjusting pointers and attributes accordingly.

> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c

[ ... ]

> @@ -663,8 +663,8 @@ static void fbcon_prepare_logo(struct vc_data *vc, struct fb_info *info,
>  	q = (unsigned short *) (vc->vc_origin +
>  				vc->vc_size_row * rows);
>  	step = logo_lines * cols;
> -	for (r = q - logo_lines * cols; r < q; r++)
> -		if (scr_readw(r) != vc->vc_video_erase_char)
> +	for (r = q - logo_lines * cols * GLYPH_HW; r < q; r++)
> +		if (scr_readg(r) != vc->vc_video_erase_char)

[Severity: High]
Could this loop read out of bounds when evaluating screen blankness?
The pointer r is of type unsigned short *, so r++ advances it by 2 bytes.
However, scr_readg() reads 4 bytes, resulting in overlapping reads. On the
final iteration (when r equals q - 1), it appears scr_readg() would read 2
bytes past the end of the screen buffer allocation. Should this increment by
GLYPH_HW instead?

>  			break;
>  	if (r != q && new_rows >= rows + logo_lines) {
>  		save = kmalloc(array3_size(logo_lines, new_cols, GLYPH_SZ),

[ ... ]

> @@ -2161,8 +2165,23 @@ static bool fbcon_switch(struct vc_data *vc)
>  		struct vc_data *conp2 = vc_cons[logo_shown].d;
>  
>  		if (conp2->vc_top == logo_lines
> -		    && conp2->vc_bottom == conp2->vc_rows)
> +		    && conp2->vc_bottom == conp2->vc_rows) {
> +			/* Scroll the bottom part of the screen up to fill the
> +			 * logo lines.
> +			 */
> +			i = conp2->vc_bottom - conp2->vc_top;
> +			d = (unsigned short *)conp2->vc_origin;
> +			s = (unsigned short *)(conp2->vc_origin +
> +					       logo_lines * conp2->vc_size_row);
> +			while (i--) {
> +				scr_memcpyw(d, s, conp2->vc_size_row);
> +				d += conp2->vc_cols;
> +				s += conp2->vc_cols;

[Severity: High]
Does this pointer arithmetic corrupt the console screen buffer for 32-bit
glyphs? The pointers d and s are unsigned short *, so advancing them by
vc_cols moves them forward by vc_cols * 2 bytes. Since conp2->vc_size_row
is vc_cols * 4 bytes for 32-bit glyphs, this seems to advance the pointers
by exactly half a row.

Because of this, the next loop iteration would overwrite the middle of the
previously copied row. Furthermore, the subsequent scr_memset_worl() would
start clearing from the wrong offset. Should these pointers be advanced by
conp2->vc_cols * GLYPH_HW?

> +			}
> +			scr_memset_worl(d, conp2->vc_video_erase_char,
> +				    conp2->vc_size_row * logo_lines);
>  			conp2->vc_top = 0;
> +		}
>  		logo_shown = FBCON_LOGO_CANSHOW;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/apCEDM2sWv_M354-@MAC.fritz.box?part=3