Re: [Patch 4/9]: Amend internal manipulation of glyph structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing bounds check in `bit_cursor` allows an out-of-bounds read of kernel memory, leading to a severe information leak, exacerbated by the new `vc_char_mask`.
- [High] `vc_do_resize` corrupts `vc_video_erase_char` by overwriting the correct value from `update_attr(vc)` with a manually calculated, malformed value.
- [High] `fbcon_init` fails to initialize `vc_char_mask` and `vc_attr_shift_pos` when `init == false`, truncating 512-character fonts to 256 characters during console driver binding.
- [High] The patch removed the left-shift of attributes for 512-character fonts in `build_attr()`, but failed to update `vc_invert_attr()` and `csi_RSB()`, which still expect the shifted format.
- [High] `vc_complement_mask` is incorrectly initialized for 512-character fonts due to out-of-order execution between `fbcon_init` and `visual_init`.
--

commit 2c79dd51730bb5b8d8d112b0649dbd28e356ba79
Author: Alan Mackenzie <acm@muc.de>

vt: 32b glyph: 4. Amend internal manipulation of glyph structure

This commit replaces hard-coded bitmask constants with new variables
in the vc_data structure to handle the extraction and insertion of bit
fields for 16-bit and 32-bit glyphs.

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1051,19 +1036,37 @@ static void visual_init(struct vc_data *vc, int num, bool init)
>  		con_free_unimap(vc);
>  	vc->uni_pagedict_loc = &vc->uni_pagedict;
>  	vc->uni_pagedict = NULL;
> -	vc->vc_hi_font_mask = 0;
> +	vc->vc_hi_font_mask = 0; /* Probably redundant, 2025-01-16. */
>  	vc->vc_complement_mask = 0;
>  	vc->vc_can_do_color = 0;
>  	vc->vc_cur_blink_ms = DEFAULT_CURSOR_BLINK_MS;
> +
>  	vc->vc_sw->con_init(vc, init);

[ ... ]

>  	if (!vc->vc_complement_mask)
> -		vc->vc_complement_mask = vc->vc_can_do_color ? 0x7700 : 0x0800;
> +		vc->vc_complement_mask = (vc->vc_can_do_color ? 0x77 : 0x08)
> +			<< vc->vc_attr_shift_pos;

[Severity: High]
Can this fail to correct a wrongly initialized complement mask?

visual_init() calls the console driver init (which reaches fbcon_init)
before vc_attr_shift_pos is fully initialized. fbcon_init() configures
vc_complement_mask falling back to a shift of 8.

When execution returns here, the zero-check skips execution because the
mask is already non-zero. For 512-character fonts, does this leave the
mask targeting bits 8-15 instead of 9-15, causing the mouse pointer to
corrupt the character glyph index?

[ ... ]

> @@ -1185,12 +1188,31 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
>  	unsigned int new_cols, new_rows, new_row_size, new_screen_size;
>  	unsigned short *oldscreen, *newscreen;
>  	u32 **new_uniscr = NULL;
> -
> +	unsigned int new_vc_char_mask = vc->vc_char_mask;
> +	unsigned int new_vc_attr_mask = vc->vc_attr_mask;
> +	unsigned int new_vc_attr_shift_pos = vc->vc_attr_shift_pos;
> +	unsigned int new_vc_video_erase_char = vc->vc_video_erase_char;
>  	WARN_CONSOLE_UNLOCKED();
>  
>  	if (cols > VC_MAXCOL || lines > VC_MAXROW)
>  		return -EINVAL;
>  
> +#ifndef CONFIG_FB_GLYPH_21BIT
> +	if (vc->vc_hi_font_mask == 0x100) {
> +		new_vc_char_mask = 0x1ff;
> +		new_vc_attr_mask = 0xfe00;
> +		new_vc_attr_shift_pos = 9;
> +	} else {
> +		new_vc_char_mask = 0xff;
> +		new_vc_attr_mask = 0xff00;
> +		new_vc_attr_shift_pos = 8;
> +	}
> +#endif
> +	new_vc_video_erase_char =
> +		(vc->vc_video_erase_char & vc->vc_char_mask) |
> +		((vc->vc_video_erase_char >> vc->vc_attr_shift_pos) <<
> +		 new_vc_attr_shift_pos);

[Severity: High]
Could this recalculation corrupt the erase character during a resize?

When fbcon_set_disp() triggers a resize, this section recalculates
new_vc_video_erase_char by shifting the old value, dropping bits.
Later in vc_do_resize(), the correctly rebuilt erase character
from update_attr() is explicitly overwritten with this malformed
new_vc_video_erase_char.

Will this cause severe background color corruption following a display
mode or font change?

[ ... ]

> @@ -2141,8 +2163,10 @@ static void csi_RSB(struct vc_data *vc)
>  		break;
>  	case CSI_RSB_MAKE_CUR_COLOR_DEFAULT:
>  		vc->vc_def_color = vc->vc_attr;
> +#ifndef CONFIG_FB_GLYPH_21BIT
>  		if (vc->vc_hi_font_mask == 0x100)
>  			vc->vc_def_color >>= 1;
> +#endif
>  		default_attr(vc);
>  		update_attr(vc);
>  		break;

[Severity: High]
Does this code still need to shift the default color?

The patch removes the left-shift of attributes for 512-character fonts
in build_attr(). Since vc_attr is now in an unshifted format, shifting
it right here halves the color value on a color reset instead of undoing
the shift.

[ ... ]

> @@ -3049,10 +3075,12 @@ static inline unsigned char vc_invert_attr(const struct vc_data *vc)
>  	if (!vc->vc_can_do_color)
>  		return vc->vc_attr ^ 0x08;
>  
> +#ifndef CONFIG_FB_GLYPH_21BIT
>  	if (vc->vc_hi_font_mask == 0x100)
>  		return   (vc->vc_attr & 0x11) |
>  			((vc->vc_attr & 0xe0) >> 4) |
>  			((vc->vc_attr & 0x0e) << 4);
> +#endif

[Severity: High]
Are these bitwise operations still valid for the new attribute layout?

The legacy logic in vc_invert_attr() applies bitwise transformations
(0x11, 0xe0, 0x0e) meant for attributes shifted left by 1. Applying
these to the new unshifted layout scrambles the foreground and
background colors when the selection or cursor is inverted.

> diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
> --- a/drivers/video/fbdev/core/bitblit.c
> +++ b/drivers/video/fbdev/core/bitblit.c
> @@ -260,7 +260,7 @@ static void bit_cursor(struct vc_data *vc, struct fb_info *info, bool enable,
>  {
>  	struct fb_cursor cursor;
>  	struct fbcon_par *par = info->fbcon_par;
> -	unsigned short charmask = vc->vc_hi_font_mask ? 0x1ff : 0xff;
> +	unsigned int charmask = vc->vc_char_mask;
>  	int w = DIV_ROUND_UP(vc->vc_font.width, 8), c;
>  	int y = real_y(par->p, vc->state.y);
>  	int attribute, use_sw = vc->vc_cursor_type & CUR_SW;

[Severity: Critical]
Does this sequence allow an out-of-bounds read of kernel memory?

The function extracts the character index from the screen buffer without
validating it against vc_font.charcount. Under CONFIG_FB_GLYPH_21BIT,
vc_char_mask expands to 0x1fffff, removing the implicit bitmask limit.

When calculating the pointer later in bit_cursor():

src = vc->vc_font.data + ((c & charmask) * (w * vc->vc_font.height));

Could a user with write access to /dev/vcsaN write crafted 32-bit
values to the screen buffer to leak sensitive memory to the display?

> 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
> @@ -1156,14 +1156,8 @@ static void fbcon_init(struct vc_data *vc, bool init)
>  	}
>  
>  	vc->vc_can_do_color = (fb_get_color_depth(&info->var, &info->fix)!=1);
> -	vc->vc_complement_mask = vc->vc_can_do_color ? 0x7700 : 0x0800;
> -	if (vc->vc_font.charcount == 256) {
> -		vc->vc_hi_font_mask = 0;
> -	} else {
> -		vc->vc_hi_font_mask = 0x100;
> -		if (vc->vc_can_do_color)
> -			vc->vc_complement_mask <<= 1;
> -	}
> +	vc->vc_complement_mask = (vc->vc_can_do_color ? 0x77 : 0x08) <<
> +		(vc->vc_attr_shift_pos ? vc->vc_attr_shift_pos : 8);

[Severity: High]
Will this skip initializing vc_char_mask when init == false?

When switching console drivers, visual_init() clears vc_hi_font_mask
and calls fbcon_init() with init == false. Because the initialization of
the font mask was removed here, vc_do_resize() forcefully applies a
256-character mask (0xff).

Does this truncate valid 512-character fonts during driver binding,
rendering the upper 256 characters unreachable?

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