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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] fbcon: stale rotated font cache key in fbcon_rotate_font() on error rollback
--

commit 4c61e799dc92607244ca176d7070e7e3d2fde5e4
Author: Yao Yiqi <yaoyiqi3@huawei.com>

fbcon: Retry rotated font allocation after failure

This commit checks par->rotated.buf in the early-out condition of
fbcon_rotate_font() so that a failed rotated font allocation is retried on
the next call instead of leaving the console blank.

> diff --git a/drivers/video/fbdev/core/fbcon_rotate.c b/drivers/video/fbdev/core/fbcon_rotate.c
> index 660a3fa81baed..7afd63dcee7be 100644
> --- a/drivers/video/fbdev/core/fbcon_rotate.c
> +++ b/drivers/video/fbdev/core/fbcon_rotate.c
> @@ -25,7 +25,8 @@ int fbcon_rotate_font(struct fb_info *info, struct vc_data *vc)
>  	    par->rotate == par->rotated.buf_rotate &&
>  	    vc->vc_font.width == par->rotated.buf_width &&
>  	    vc->vc_font.height == par->rotated.buf_height &&
> -	    vc->vc_font.charcount == par->rotated.buf_charcount)
> +	    vc->vc_font.charcount == par->rotated.buf_charcount &&
> +	    par->rotated.buf)
>  		return 0;
>  
>  	par->rotated.fontdata = par->p->fontdata;

[Severity: High]
Should fbcon_rotate_font() update the cache key fields in par->rotated only
after font_data_rotate() succeeds, rather than relying solely on checking
par->rotated.buf in the early-out?

Because fbcon_rotate_font() records par->rotated.fontdata, buf_rotate,
buf_width, buf_height, and buf_charcount before calling font_data_rotate(),
those fields still get overwritten with the new font's metadata when
font_data_rotate() fails.

In drivers/video/fbdev/core/fbcon.c:fbcon_do_set_font(), the caller saves
par->rotated.buf before calling rotate_font() and restores the old buffer on
failure:

drivers/video/fbdev/core/fbcon.c:fbcon_do_set_font() {
	if (con_is_visible(vc)) {
		set_blitting_type(vc, info);
		if (par->bitops->rotate_font) {
#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION
			saved_rotated_buf = par->rotated.buf;
			saved_rotated_bufsize = par->rotated.bufsize;
			par->rotated.buf = NULL;
			par->rotated.bufsize = 0;
			rotated_saved = true;
#endif
			ret = par->bitops->rotate_font(info, vc);
			if (ret)
				goto err_out;
		}
	}
	...
err_out:
	if (rotated_saved) {
		kfree(par->rotated.buf);
		par->rotated.buf = saved_rotated_buf;
		par->rotated.bufsize = saved_rotated_bufsize;
	}
}

If rotate_font() (or the subsequent vc_resize()) fails in fbcon_do_set_font()
when switching a visible console from a smaller font (e.g. 8x8) to a larger
font (e.g. 16x32), err_out restores par->rotated.buf to the smaller font's
non-NULL saved_rotated_buf while leaving par->rotated.{fontdata, buf_rotate,
buf_width, buf_height, buf_charcount} set to the failed larger font's
metadata.

If another console already uses that same larger font (or if changing to a
built-in/shared font matching that stale key is retried), the next
fbcon_switch() -> fbcon_rotate_font() sees all key fields match and
par->rotated.buf != NULL, so it returns 0 without reallocating or rotating
the buffer. Could the rotated rendering callbacks (cw_putcs(), ccw_putcs(),
ud_putcs(), cw_cursor(), ccw_cursor(), ud_cursor()) then read out of bounds
from the smaller restored par->rotated.buf using the larger font's geometry?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019090206.1094958-1-yyaqia@gmail.com?part=3