Re: [Patch 7/9]: vt: Handle up to 2^21 glyphs, rather than 256/512

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The intended support for large 21-bit fonts is functionally broken, and allocations near the 4MB boundary trigger a kernel warning.
--

commit 0f8b2aafff067c8299c320bab75fcb965a117b84
Author: Alan Mackenzie <acm@muc.de>

vt: Handle up to 2^21 glyphs, rather than 256/512

This commit increases the maximum number of font glyphs supported by the
vt subsystem to 2^21 to supplement tests with full Unicode limits.

> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9208970f731e0..baf7c98429e9c 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
[ ... ]
> @@ -5015,17 +5016,23 @@ static int con_font_set(struct vc_data *vc, const struct console_font_op *op)
[ ... ]
>  	size = DIV_ROUND_UP(op->width, 8) * vpitch * op->charcount;
> +#ifndef CONFIG_FB_GLYPH_21BIT
>  	if (size > max_font_size)
>  		return -ENOSPC;
> -
> +#endif
>  	void *font_data __free(kfree) = font.data = memdup_user(op->data, size);

[Severity: High]
Will memdup_user() reliably handle the newly permitted size allocations?

Because memdup_user() relies on kmalloc(), it is bounded to allocations
up to 4MB (MAX_PAGE_ORDER). If a user requests a font with 2^21 glyphs, the
size could easily exceed this limit, causing the allocation to silently fail
with -ENOMEM. 

Would it be better to use vmemdup_user() here to fully support the larger
21-bit limit?

> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 576be479d9286..a610071c89f53 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
[ ... ]
> @@ -2525,10 +2531,18 @@ static int fbcon_set_font(struct vc_data *vc, const struct console_font *font,
>  	int i, ret;
>  	font_data_t *new_data;
>  
> -	/* Is there a reason why fbconsole couldn't handle any charcount >256?
> -	 * If not this check should be changed to charcount < 256 */
> +#ifdef CONFIG_FB_GLYPH_21BIT
> +	if (charcount < 256)
> +		return -EINVAL;

[Severity: High]
Does this relaxed limit expose a kernel warning crash vector in the page
allocator?

By allowing charcount to reach 2^21, font allocations can approach or exactly
hit the 4MB limit. When font_data_import() is called later in this function
with these parameters, it does the following:

lib/fonts/fonts.c:font_data_import() {
    ...
    alloc_size = size + sizeof(*font_data);
    font_data = kmalloc(alloc_size, GFP_USER);
    ...
}

If a user with CAP_SYS_TTY_CONFIG configures a font where the requested size
is exactly 4MB (which passes the memdup_user() check in con_font_set()), the
addition of sizeof(*font_data) will push the alloc_size slightly over 4MB. 

Since this kmalloc() lacks the __GFP_NOWARN flag and requires an order 11
allocation, it will trigger a WARN_ON_ONCE in the page allocator. This could
crash systems configured with panic_on_warn.

> +#else
> +	/* There is no longer any reason why fbconsole can't handle
> +	 * any charcount >256, when CONFIG_FB_GLYPH_21BIT is #defined.
> +	 * Hence this check has been changed to charcount < 256
> +	 * above.
> +	 */
>  	if (charcount != 256 && charcount != 512)
>  		return -EINVAL;
> +#endif

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