Re: [Patch 9/9]: vt: Misc changes, e.g. to #include directives

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `con_font_get()` can lead to a kernel heap buffer overflow.
- [Critical] Pre-calculating the allocation size `c` based on the per-console `vc->vc_font.charcount` causes a heap buffer overflow in `vgacon_font_get()` due to a per-console versus global state mismatch.
--

commit 501185cf94f089f684db150e409e50e6d10bc832
Author: Alan Mackenzie <acm@muc.de>

vt: Misc changes, e.g. to #include directives

Rearrange some #include directives, add some clarifying comments, and some
code changes which didn't fit elsewhere.

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index baf7c98429e9c..0c389a5643576 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
[ ... ]
> @@ -4967,14 +4968,16 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
>  {
>  	struct console_font font;
>  	int c;
> -	unsigned int vpitch = op->op == KD_FONT_OP_GET_TALL ? op->height : 32;
> +	unsigned int vpitch =
> +		op->op == KD_FONT_OP_GET_TALL ? vc->vc_font.height : 32;
>  
>  	if (vpitch > max_font_height)
>  		return -EINVAL;
>  
>  	void *font_data __free(kvfree) = NULL;
> +	c = DIV_ROUND_UP(vc->vc_font.width, 8) * vpitch * vc->vc_font.charcount;
>  	if (op->data) {
> -		font.data = font_data = kvzalloc(max_font_size, GFP_KERNEL);
> +		font.data = font_data = kvzalloc(c, GFP_KERNEL);

[Severity: Critical]
Does this introduce a race condition regarding the allocation size?

Since the buffer size c is now calculated using vc->vc_font.width and
vc->vc_font.charcount before acquiring the console_lock, couldn't another
thread concurrently call con_font_set(), update the console font dimensions
to be larger, and cause the newly sized font data to be written into this
undersized heap allocation later when vc_sw->con_font_get() is called inside
the lock?

Additionally, can this cause an issue with the vgacon driver due to a mismatch
between per-console and global state?

The vgacon driver tracks whether a 512-character font is in use globally
via vga_512_chars. However, vc->vc_font.charcount is maintained per-console.

If a 512-character font is set on one terminal (setting vga_512_chars to
true), but con_font_get() is called on a different terminal that still
has a 256-character font, this calculation will size the allocation for 256
characters.

The vgacon_font_get() callback will then observe the global flag and
force a read of 512 characters, writing out of bounds into the smaller
buffer during vgacon_do_font_op().

>  		if (!font.data)
>  			return -ENOMEM;
>  	} else
> @@ -4991,8 +4994,6 @@ static int con_font_get(struct vc_data *vc, struct console_font_op *op)
>  			return ret;
>  	}
>  
> -	c = DIV_ROUND_UP(font.width, 8) * vpitch * font.charcount;
> -
>  	if (op->data && font.charcount > op->charcount)
>  		return -ENOSPC;
>  	if (font.width > op->width || font.height > op->height)

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