Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Missing locking in sysfs readers (`show_mode`, `show_modes`) creates a TOCTOU race leading to Use-After-Free. - [High] Buffer overflow via integer underflow in `mode_string()`. - [High] `FBIOPUT_VSCREENINFO` ioctl with `FB_ACTIVATE_INV_MODE` deletes modes without clearing dangling pointers. - [High] Unchecked NULL pointer dereference in `display_to_var()` leads to kernel panic. -- commit f37a3c1901e8e451a34ca5fd10fa0a9b6e643e23 Author: Ian Bridges <[email protected]> fbdev: fix use-after-free in store_modes() store_modes() replaces a framebuffer's modelist with modes from userspace. This patch fixes use-after-free bugs by clearing fb_display[i].mode and fb_info->mode pointers before freeing the old modelist. Link: https://lore.kernel.org/all/ajjoDhAi2y4ArSlz@dev/ diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c index d9743ef353552..ea196603c7a87 100644 --- a/drivers/video/fbdev/core/fbsysfs.c +++ b/drivers/video/fbdev/core/fbsysfs.c [ ... ] @@ -108,8 +109,15 @@ static ssize_t store_modes(struct device *device, if (fb_new_modelist(fb_info)) { fb_destroy_modelist(&fb_info->modelist); list_splice(&old_list, &fb_info->modelist); [Severity: High] This isn't a bug introduced by this patch, but is there a missing lock in the sysfs readers that could lead to a use-after-free? While store_modes() correctly takes lock_fb_info() before replacing the modelist, show_mode() and show_modes() appear to read fb_info->mode and iterate fb_info->modelist locklessly: drivers/video/fbdev/core/fbsysfs.c:show_mode() { if (!fb_info->mode) return 0; return mode_string(buf, 0, fb_info->mode); } drivers/video/fbdev/core/fbsysfs.c:show_modes() { ... list_for_each_entry(modelist, &fb_info->modelist, list) { mode = &modelist->mode; i += mode_string(buf, i, mode); } ... } If store_modes() executes concurrently, replaces the modelist, frees the old nodes via fb_destroy_modelist(), and sets fb_info->mode to NULL, could a concurrent sysfs read hit a use-after-free or NULL pointer dereference? [Severity: High] This is a pre-existing issue, but does show_modes() have an integer underflow leading to a buffer overflow? In show_modes(), the offset accumulates indefinitely based on the return value of snprintf: drivers/video/fbdev/core/fbsysfs.c:show_modes() { ... list_for_each_entry(modelist, &fb_info->modelist, list) { mode = &modelist->mode; i += mode_string(buf, i, mode); } ... } If there are enough modes in the modelist, i will exceed PAGE_SIZE. This value is then passed as the offset to mode_string(): drivers/video/fbdev/core/fbsysfs.c:mode_string() { return snprintf(&buf[offset], PAGE_SIZE - offset, "%c:%dx%d%c-%d\n", m, mode->xres, mode->yres, v, mode->refresh); } When offset is greater than PAGE_SIZE, does PAGE_SIZE - offset underflow to a massive unsigned limit, causing snprintf() to write out of bounds beyond the sysfs page buffer? > - } else > + } else { > + /* > + * fb_display[i].mode and fb_info->mode both point into the old > + * list. Clear them before it is freed. > + */ > + fbcon_delete_modelist(&old_list); > + fb_info->mode = NULL; [Severity: High] This isn't a bug introduced by this patch, but does clearing the modelist here expose an unchecked NULL pointer dereference? The patch modifies store_modes() to call fbcon_delete_modelist(), which clears disp->mode to NULL. However, functions like fbcon_resize() and fbcon_switch() unconditionally call display_to_var(): drivers/video/fbdev/core/fbcon.c:display_to_var() { ... fb_videomode_to_var(var, disp->mode); ... } drivers/video/fbdev/core/fbcon.c:fbcon_resize() { ... display_to_var(&var, p); fb_videomode_to_var(&var, mode); ... } Can a user write to sysfs modes, setting fb_display[i].mode to NULL, followed by a console resize or VT switch that leads to a kernel panic when fb_videomode_to_var() dereferences the NULL mode? > fb_destroy_modelist(&old_list); > + } > > unlock_fb_info(fb_info); > console_unlock(); [Severity: High] This is a pre-existing issue, but does fb_set_var() suffer from the exact same dangling pointer issue this patch fixes in store_modes()? When executing an FBIOPUT_VSCREENINFO ioctl with FB_ACTIVATE_INV_MODE, fb_set_var() checks if the mode is used by mapped consoles: drivers/video/fbdev/core/fbmem.c:fb_set_var() { ... ret = fbcon_mode_deleted(info, &mode1); if (!ret) fb_delete_videomode(&mode1, &info->modelist); ... } Since fbcon_mode_deleted() intentionally skips unmapped consoles, it will return 0 if the mode is only referenced by unmapped consoles. Does this result in fb_delete_videomode() freeing the mode while fb_display[i].mode in the unmapped console and fb_info->mode still point to the freed memory? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/aj4FKJtIuU97rEA4@dev?part=1
