Re: [PATCH 2/2] fbcon: Retry rotated font allocation after failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fbcon: OOB read via dimension-swapped font loading
- [Medium] fbcon: broken fallback to unrotated rendering on rotate_font failure
--

commit 1bccc1e9cd74782edba7f76b0ad2e196a29fd948
Author: Yao Yiqi <yaoyiqi3@huawei.com>

fbcon: Retry rotated font allocation after failure

fbcon_rotate_font() records the font and rotation it is about to handle in
par->rotated.fontdata and par->rotated.buf_rotate before calling
font_data_rotate(). If that allocation fails it clears par->rotated.buf,
but the cached font/rotation state already looks up to date, so the next
call takes the early-out and never retries.

Fixes: e4ef723d8975 ("fbcon: Avoid OOB font access if console rotation fails")
Cc: stable@vger.kernel.org

> The rotated putcs/cursor ops keep returning early on the NULL buffer and
> the console stays blank until the font or the rotation is changed, even
> once memory is available again.

[Severity: Medium]
Is the fallback to unrotated rendering on rotate_font() failure broken?

When par->bitops->rotate_font() fails, existing callers attempt to fall back
to unrotated rendering:

drivers/video/fbdev/core/fbcon.c:fbcon_init() {
    ...
	if (par->bitops->rotate_font && par->bitops->rotate_font(info, vc)) {
		par->rotate = FB_ROTATE_UR;
		set_blitting_type(vc, info);
	}
    ...
}

But set_blitting_type() invokes fbcon_set_rotation() unconditionally, which
overwrites the fallback state back to the originally requested rotation:

drivers/video/fbdev/core/fbcon.c:fbcon_set_rotation() {
    ...
	if (!(info->flags & FBINFO_MISC_TILEBLITTING) &&
	    par->p->con_rotate < 4)
		par->rotate = par->p->con_rotate;
    ...
}

Because the bitops are left as rotated, and par->rotated.buf is NULL, all
drawing operations abort and the screen remains completely blank:

drivers/video/fbdev/core/fbcon_ccw.c:ccw_putcs() {
    ...
	if (!par->rotated.buf)
		return;
    ...
}

Also, it appears the call to rotate_font() in fbcon_do_set_font() ignores
its return value entirely:

drivers/video/fbdev/core/fbcon.c:fbcon_do_set_font() {
    ...
	if (con_is_visible(vc) && par->bitops->rotate_font)
		par->bitops->rotate_font(info, vc);
    ...
}

> diff --git a/drivers/video/fbdev/core/fbcon_rotate.c b/drivers/video/fbdev/core/fbcon_rotate.c
> index 6cdbc96eeca6f..6b148e0e81e66 100644
> --- a/drivers/video/fbdev/core/fbcon_rotate.c
> +++ b/drivers/video/fbdev/core/fbcon_rotate.c
> @@ -21,7 +21,9 @@ int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
>  	unsigned char *buf;
>  	int ret;
>  
> -	if (par->p->fontdata == par->rotated.fontdata && par->rotate == par->rotated.buf_rotate)
> +	if (par->p->fontdata == par->rotated.fontdata &&
> +	    par->rotate == par->rotated.buf_rotate &&
> +	    par->rotated.buf)
>  		return 0;

[Severity: High]
Does this early return still rely only on the fontdata pointer?

If a new font shares the same raw byte array (reused via font_data_is_equal)
but has different dimensions, it looks like it could bypass rotated buffer
reallocation because the fontdata pointer remains the same.

Rendering operations could then use the new, larger cellsize to index the
un-updated rotated buffer, resulting in an out-of-bounds read:

drivers/video/fbdev/core/fbcon_cw.c:cw_putcs_aligned() {
    ...
	src = par->rotated.buf + (scr_readw(s++) & charmask) * cellsize;
    ...
}

>  
>  	par->rotated.fontdata = par->p->fontdata;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014085102.497789-1-yaoyiqi3@huawei.com?part=2