Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Ineffective fallback logic on memory allocation failure causes a NULL pointer dereference in the console rendering path. - [High] Out-of-bounds access or NULL pointer dereference in fbcon_init() due to missing selection pre-clearing fix. - [High] Out-of-bounds access when changing font size on an actively rotated console. - [High] Ignored vc_resize() return value leaves stale geometry, causing an out-of-bounds memory access. -- --- Patch [1]: [PATCH V3] fbdev: Fix out-of-bounds access when rotating console after font resize --- commit f333e84fec0269170d8087f048fca7b2a06cbd02 Author: Zizhi Wo <[email protected]> fbdev: Fix out-of-bounds access when rotating console after font resize This commit fixes an out-of-bounds access when rotating the console after a font resize. It resolves the issue by pre-clearing the selection before switching bitops in fbcon_modechanged() and fbcon_set_all_vcs(), ensuring that par->rotated.buf is rebuilt before subsequent updates. Link: https://lore.kernel.org/all/[email protected]/ Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/video/fbdev/core/fbcon.c > b/drivers/video/fbdev/core/fbcon.c > index 9f5c4c101581d..23b3c536d53d1 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -2641,9 +2641,31 @@ static void fbcon_modechanged(struct fb_info *info) > fbcon_info_from_console(par->currcon) != info) > return; > > + /* > + * Clear the selection before switching bitops. Without this, the > + * clear_selection() inside vc_resize() below repaints the highlighted > + * cells through the new bitops while the console > geometry(vc_rows/vc_cols) > + * has not been updated to match, so the repaint is computed from a > + * half-switched geometry and overflows the framebuffer address. > + * Pre-clearing makes that repaint a no-op. > + */ > + clear_selection(); > + > p = &fb_display[vc->vc_num]; > set_blitting_type(vc, info); > > + /* > + * Rebuild par->rotated.buf for the new rotation now that bitops have > + * switched. The new putcs/cursor ops read this buffer; if it is still > + * sized for the old rotation, fbcon_putcs() and the cursor path reached > + * via update_screen() below overflow it. Mirrors fbcon_switch(); fall > + * back to unrotated rendering on allocation failure. > + */ > + if (par->bitops->rotate_font && par->bitops->rotate_font(info, vc)) { > + par->rotate = FB_ROTATE_UR; > + set_blitting_type(vc, info); > + } [Severity: High] This is a pre-existing issue, but does this fallback logic actually prevent the crash on allocation failure? When rotate_font() fails, par->rotated.buf is freed and set to NULL. The fallback attempts to revert to unrotated rendering by setting par->rotate = FB_ROTATE_UR and calling set_blitting_type(vc, info). However, set_blitting_type() internally calls fbcon_set_rotation(info), which resets par->rotate back to the failing requested rotation from par->p->con_rotate: drivers/video/fbdev/core/fbcon.c:set_blitting_type() { ... fbcon_set_rotation(info); fbcon_set_bitops(par); } This causes the rotated bitops to be selected again. Subsequent screen updates will then dereference the now-NULL par->rotated.buf. Should par->p->con_rotate also be updated to FB_ROTATE_UR in this fallback path? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but is it safe to ignore the return value of vc_resize() later in fbcon_modechanged()? If vc_resize() fails (e.g., due to kzalloc returning -ENOMEM), it leaves vc->vc_cols and vc->vc_rows unmodified, potentially at older, larger values. The execution then continues unconditionally to update_screen(): drivers/video/fbdev/core/fbcon.c:fbcon_modechanged() { ... vc_resize(vc, cols, rows); ... update_screen(vc); } This loops over the stale, oversized vc->vc_cols. In ccw_putcs(), the calculation: image.dy = vyres - ((xx + count) * vc->vc_font.width) can underflow because the stale xx + count is too large for the new vyres. This results in a massive image.dy offset and a page fault when calculating the framebuffer address, reintroducing the out-of-bounds access upon allocation failure. [Severity: High] This is a pre-existing issue, but does fbcon_init() suffer from the same missing selection pre-clearing issue this patch addresses? In fbcon_init(), set_blitting_type() is called to switch bitops before rotate_font() is invoked: drivers/video/fbdev/core/fbcon.c:fbcon_init() { set_blitting_type(vc, info); ... if (init) { vc->vc_cols = new_cols; vc->vc_rows = new_rows; } else vc_resize(vc, new_cols, new_rows); ... } If the console is visible and has a selection, vc_resize() triggers clear_selection(), which forces a screen redraw. Since rotate_font() has not yet been called, par->rotated.buf may be NULL or improperly sized, leading the newly-switched bitops to read out-of-bounds or dereference NULL. [Severity: High] This isn't a bug introduced by this patch, but could changing the font size on an actively rotated console cause an out-of-bounds access? When a KDFONTOP ioctl increases the font size on a rotated console, fbcon_do_set_font() updates the font dimensions and calls vc_resize(): drivers/video/fbdev/core/fbcon.c:fbcon_do_set_font() { ... ret = vc_resize(vc, cols, rows); ... } vc_resize() triggers a screen update using the new font geometry. However, fbcon_do_set_font() never calls par->bitops->rotate_font() to reallocate par->rotated.buf for the new dimensions. During the redraw, rotated rendering functions like ccw_putcs() calculate memory offsets based on the larger font sizes, reading past the end of the existing, too-small par->rotated.buf. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
