Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] fbcon: incomplete par->rotated rollback on error in fbcon_do_set_font() - [High] fbcon: incomplete par->rotated state save and restore in fbcon_do_set_font() --
commit 40279fe676962065434b918b61507d77c5b4a060 Author: Yao Yiqi <[email protected]> fbcon: Rebuild rotated font buffer on font change This patch rebuilds the rotated font buffer in fbcon_do_set_font() when changing fonts on the visible console so that vc_resize() does not render using a rotated font cache sized for the old font. If rotation or resizing fails, it restores the saved rotated buffer and rolls back the font change. > diff --git a/drivers/video/fbdev/core/fbcon.c > b/drivers/video/fbdev/core/fbcon.c > index 01715873ea49b..7605e4237ca5b 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -2414,6 +2414,11 @@ static int fbcon_do_set_font(struct vc_data *vc, int > w, int h, int charcount, > font_data_t *old_fontdata = p->fontdata; > const u8 *old_data = vc->vc_font.data; > unsigned short old_hi_font_mask = vc->vc_hi_font_mask; > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > + u8 *saved_rotated_buf = NULL; > + size_t saved_rotated_bufsize = 0; > + bool rotated_saved = false; > +#endif > > font_data_get(data); > > @@ -2432,6 +2437,29 @@ 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. Clear the buffer so the > + * rebuild allocates a fresh one; the old buffer is kept until the > + * font change can no longer fail. > + */ > + 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); [Severity: High] Can clearing only par->rotated.buf and par->rotated.bufsize here cause the active rotated font buffer to be freed and lost when re-setting the same font? In drivers/video/fbdev/core/fbcon_rotate.c, fbcon_rotate_font() checks whether the cache metadata matches before allocating a new buffer: drivers/video/fbdev/core/fbcon_rotate.c:fbcon_rotate_font() { if (par->p->fontdata == par->rotated.fontdata && 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) return 0; } At this commit, before the subsequent commit 4c61e799dc92 ("fbcon: Retry rotated font allocation after failure") adds a check for par->rotated.buf, re-applying a font with the same fontdata and geometry (for example via fbcon_set_font() deduplication or fbcon_set_def_font()) hits this early return with par->rotated.buf still NULL. fbcon_do_set_font() then calls kfree(saved_rotated_buf), leaving par->rotated.buf NULL. > + if (ret) > + goto err_out; > + } > + } > + > if (resize) { > int cols, rows; > > @@ -2448,12 +2476,27 @@ static int fbcon_do_set_font(struct vc_data *vc, int > w, int h, int charcount, > update_screen(vc); > } > > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > + if (rotated_saved) > + kfree(saved_rotated_buf); > +#endif > if (old_fontdata) > font_data_put(old_fontdata); > > return 0; > > err_out: > +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_ROTATION > + /* > + * Drop the buffer built for the new font and restore the saved > + * one. > + */ > + if (rotated_saved) { > + kfree(par->rotated.buf); > + par->rotated.buf = saved_rotated_buf; > + par->rotated.bufsize = saved_rotated_bufsize; > + } > +#endif [Severity: High] What happens to the cache key fields in par->rotated when rotate_font() or vc_resize() fails and jumps to err_out? In fbcon_rotate_font(), the cache key fields are updated with the new font's pointer and geometry before font_data_rotate() is called: drivers/video/fbdev/core/fbcon_rotate.c:fbcon_rotate_font() { ... par->rotated.fontdata = par->p->fontdata; par->rotated.buf_rotate = par->rotate; par->rotated.buf_width = vc->vc_font.width; par->rotated.buf_height = vc->vc_font.height; par->rotated.buf_charcount = vc->vc_font.charcount; ... buf = font_data_rotate(...); } If font_data_rotate() or the subsequent vc_resize() in fbcon_do_set_font() fails, err_out restores par->rotated.buf and par->rotated.bufsize to the old font's buffer and restores p->fontdata and vc->vc_font to the old font, but leaves par->rotated.{fontdata,buf_rotate,buf_width,buf_height, buf_charcount} set to the failed new font's metadata. Because par->rotated.buf is restored to the non-NULL saved_rotated_buf, even with commit 4c61e799dc92 ("fbcon: Retry rotated font allocation after failure") applied, switching via fbcon_switch() to another console that uses that same font and larger geometry (or a subsequent font operation that reuses the freed font_data_t address with the larger geometry) will see all cache key fields match and par->rotated.buf != NULL in fbcon_rotate_font(). It will return early without rebuilding the buffer, and the rotated blitting and cursor callbacks (cw_putcs/ccw_putcs/ud_putcs and cw_cursor/ccw_cursor/ud_cursor) will index the smaller saved_rotated_buf using the larger cell size, triggering a slab-out-of-bounds read. Would saving and clearing the entire par->rotated struct before calling rotate_font(), and restoring the whole struct in err_out, avoid both issues? [ ... ] > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
