Re: [PATCH v3 2/3] fbcon: Rebuild rotated font buffer on font change

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 16.10.26 um 09:30 schrieb Yao Yiqi:
> From: Yao Yiqi <yaoyiqi3@huawei.com>
>
> fbcon_rotate_font() caches a rotated copy of the console font in
> par->rotated.buf. The buffer is sized from the font's width, height
> and charcount, and is rebuilt in fbcon_init(), fbcon_switch() and
> fbcon_modechanged(), but not in fbcon_do_set_font().
>
> When the console font is changed through KDFONTOP/KD_FONT_OP_SET, the
> new font data and geometry are installed and vc_resize() is called.
> The redraw that follows reaches the rotated bitops while the buffer
> still holds the old font: the rotated putcs/cursor ops index
> par->rotated.buf with the new cell size and character codes, so a
> stale, too-small buffer is read out of bounds.
>
> KASAN reports, for example:
>
>    BUG: KASAN: slab-out-of-bounds in soft_cursor
>    Read of size 64 at addr ...
>     memcpy
>     soft_cursor
>     cw_cursor
>     fbcon_cursor
>     hide_cursor
>     redraw_screen
>     vc_do_resize
>     fbcon_do_set_font
>     fbcon_set_font
>     con_font_op
>     vt_ioctl
>
> Rebuild the buffer for the visible console right after the new font
> geometry has been installed and before any redraw. Select the
> console's blitting state with set_blitting_type() first, as
> fbcon_init(), fbcon_switch() and fbcon_modechanged() do before their
> rotate_font() calls: fbcon_rotate_font() reads the source font
> through par->p, which may otherwise still refer to another console of
> this framebuffer. On tile-blitting hardware, the same call uploads
> the new font's map to the text engine, which previously happened only
> when the console was switched. Background consoles keep their rebuild
> at fbcon_switch() time, as the buffer is shared per framebuffer and
> must not be rebuilt for a console that is not being displayed.
>
> If vc_resize() fails, err_out restores the previous font. The rotated
> buffer that was rebuilt for the new font earlier in the function
> cannot be restored with it: font_data_rotate() has already reused or
> freed the buffer that belonged to the old font. Select the blitting
> type and rebuild the buffer again after the restore: the upload to
> tile-blitting hardware and the rotated cache would otherwise keep
> describing the rejected font, and par->rotated.fontdata would keep
> referencing the font data whose last reference the error path drops.
> A failing rebuild leaves par->rotated.buf NULL, so the rotated ops
> skip drawing instead of reading a mismatched buffer; the next patch
> retries the allocation on the following call.
>
> Unlike the other callers, this one does not fall back to unrotated
> rendering on failure. The "par->rotate = FB_ROTATE_UR;
> set_blitting_type(vc, info);" fallback they use does not actually
> switch to unrotated rendering: set_blitting_type() calls
> fbcon_set_rotation(), which immediately re-derives par->rotate from
> par->p->con_rotate. Replicating that pattern here would not change
> the outcome; repairing the fallback is left to separate work.

It's way too long and boring to read. Can you please try to shorten the 
AI-generated commit description?


>
> Fixes: 6cc50e1c5b57 ("[PATCH] fbcon: Console Rotation - Add support to rotate font bitmap")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yao Yiqi <yaoyiqi3@huawei.com>
> ---
> Changes in v3:
>
> - Also select the blitting type again in the error path of
>    fbcon_do_set_font(). On tile-blitting hardware, set_blitting_type()
>    uploads the font map to the text engine; without the second call, a
>    failed vc_resize() left the engine with the rejected font (found by
>    the Sashiko review).

The problem is that rotate_font can fail as well. Handling that in the 
error path is not possible; hence results might be worse than keeping 
the rejected font.  Can you try to keep the original rotated font and 
reinstall that directly?

Best regards
Thomas


>
> Changes in v2:
>
> - Rebuild the buffer again in the error path of fbcon_do_set_font().
>    Without this, a failed vc_resize() left the rotated cache describing
>    the rejected font while the console rendered with the restored one,
>    and par->rotated.fontdata referenced freed font data (found by the
>    Sashiko review).
> - Select the console's blitting state with set_blitting_type() before
>    rebuilding, like the other rotate_font() callers, instead of writing
>    par->p directly.
> - Reworded the note about the unrotated-rendering fallback, which is
>    now left to separate work.
> ---
>   drivers/video/fbdev/core/fbcon.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index 01715873ea49..d90a2ad45053 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2432,6 +2432,17 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
>   	else if (!vc->vc_hi_font_mask && charcount == 512)
>   		set_vc_hi_font(vc, true);
>   
> +	/*
> +	 * Select the console's blitting state and update par->rotated to
> +	 * match the new font, so that the rendering below does not use a
> +	 * cache that was built for the old font.
> +	 */
> +	if (con_is_visible(vc)) {
> +		set_blitting_type(vc, info);
> +		if (par->bitops->rotate_font)
> +			par->bitops->rotate_font(info, vc);
> +	}
> +
>   	if (resize) {
>   		int cols, rows;
>   
> @@ -2466,6 +2477,18 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
>   	else if (!old_hi_font_mask && vc->vc_hi_font_mask)
>   		set_vc_hi_font(vc, false);
>   
> +	/*
> +	 * Restore the rendering state for the old font: re-select the
> +	 * blitting type, which re-uploads the font map on tile-blitting
> +	 * hardware, and overwrite par->rotated with the restored font's
> +	 * state.
> +	 */
> +	if (con_is_visible(vc)) {
> +		set_blitting_type(vc, info);
> +		if (par->bitops->rotate_font)
> +			par->bitops->rotate_font(info, vc);
> +	}
> +
>   	font_data_put(data);
>   
>   	return ret;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)