Re: [Patch 6/9]: vt: Use u32 and typedef u1632 to handle whole glyphs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Using a raw 32-bit pointer dereference for VGA I/O memory causes hardware faults on strict architectures and overflows 16-bit hardware video RAM.
- [High] Casting a 32-bit stack variable's address to a 16-bit pointer in `con_putc` leads to blank or NUL characters on all big-endian architectures when 21-bit glyphs are disabled.
- [High] Pointer arithmetic in `fbcon_switch` implicitly assumes 16-bit glyphs, causing buffer misalignment and screen corruption when 32-bit glyphs are enabled.
- [Medium] Fallback console translation macros were not updated for the new `u1632` types, resulting in data truncation and build warnings.
--

commit 6fc77218ba179478ec0c7c9f101ceadca53ba76a
Author: Alan Mackenzie <acm@muc.de>

vt: Use u32 and typedef u1632 to handle whole glyphs

vt: 32b glyph: 6. Use u32 and typedef u1632 to handle whole glyphs

The glyph size u1632 is an either 32- or 16-bit unsigned word
depending on whether or not CONFIG_FB_GLYPH_21BIT is #defined.

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 8c7cbc47836fd..9208970f731e0 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
[ ... ]
> @@ -322,12 +322,12 @@ static inline u16 *screenpos(const struct vc_data *vc, unsigned int offset,
>  	return (u16 *)(origin + offset);
>  }
>  
> -static void con_putc(struct vc_data *vc, u16 ca, unsigned int y, unsigned int x)
> +static void con_putc(struct vc_data *vc, u32 ca, unsigned int y, unsigned int x)
>  {
>  	if (vc->vc_sw->con_putc)
>  		vc->vc_sw->con_putc(vc, ca, y, x);
>  	else
> -		vc->vc_sw->con_putcs(vc, &ca, 1, y, x);
> +		vc->vc_sw->con_putcs(vc, (u16 *)&ca, 1, y, x);

[Severity: High]
Does casting this 32-bit local variable address to a 16-bit pointer expose
the endian-dependent memory layout to the backend driver?

On big-endian architectures where 21-bit glyphs are disabled, this cast
points to the most-significant 16 bits of the 32-bit variable (which are
zero). This leads to the backend reading 0x0000 and rendering a NUL character
instead of the true glyph.

[ ... ]
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 3e7cba53bdfc2..576be479d9286 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
[ ... ]
> @@ -2166,6 +2166,7 @@ static bool fbcon_switch(struct vc_data *vc)
>  	struct fbcon_par *par;
>  	struct fbcon_display *p = &fb_display[vc->vc_num];
>  	struct fb_var_screeninfo var;
> +	unsigned short *d, *s;
>  	int i, ret, prev_console;
>  
>  	info = fbcon_info_from_console(vc->vc_num);

[Severity: High]
Does the pointer arithmetic in fbcon_switch implicitly assume 16-bit glyphs?

When a logo is displayed and the console is switched, the row pointers are
advanced using vc_cols:

			while (i--) {
				scr_memcpyw(d, s, conp2->vc_size_row);
				d += conp2->vc_cols;
				s += conp2->vc_cols;
			}

When CONFIG_FB_GLYPH_21BIT is enabled, vc_size_row is vc_cols * 4 bytes.
However, d and s are unsigned short *, so adding vc_cols only advances the
pointer by vc_cols * 2 bytes. Can this cause subsequent rows to overwrite
the second half of the previous row?

[ ... ]
> diff --git a/include/linux/consolemap.h b/include/linux/consolemap.h
> index 539d488fdc03f..5e50b4d4bd0b3 100644
> --- a/include/linux/consolemap.h
> +++ b/include/linux/consolemap.h
[ ... ]
> @@ -32,7 +42,7 @@ unsigned int ucs_get_width(uint32_t cp);
>  u32 ucs_recompose(u32 base, u32 mark);
>  u32 ucs_get_fallback(u32 cp);
>  #else
> -static inline u16 inverse_translate(const struct vc_data *conp, u16 glyph,
> +static inline u1632 inverse_translate(const struct vc_data *conp, u16 glyph,
>  		bool use_unicode)
>  {
>  	return glyph;

[Severity: Medium]
Were the fallback console translation macros missed when updating to the new
u1632 types?

The fallback for inverse_translate still accepts a u16 glyph, truncating any
32-bit glyph passed into it.

Additionally, the set_translate fallback below this (not shown in the diff)
still returns unsigned short *, which causes a type mismatch when assigned to
vc->vc_translate (which is now a u1632 *).

[ ... ]

Additionally, I have a question regarding the scr_writeg macro defined in
include/linux/vt_buffer.h, which is heavily used by these console updates:

include/linux/vt_buffer.h:
#define scr_writeg(val, addr) \
        (*((u32 *)(addr)) = (val))

[Severity: Critical]
Does using a raw 32-bit pointer dereference for VGA I/O memory cause hardware
faults on strict architectures (e.g., Alpha, MIPS)?

This bypasses architectural I/O accessors like writel/writew, which are
required for accessing MMIO space on these architectures. Furthermore, can
this overflow 16-bit hardware video RAM, since VGA VRAM requires exactly
16-bit accesses per character cell?

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