* Re: [PATCH v2] fbdev: fix use-after-free in store_modes()
2026-06-26 4:50 [PATCH v2] fbdev: fix use-after-free in store_modes() Ian Bridges
@ 2026-06-26 5:09 ` sashiko-bot
2026-06-26 13:02 ` Helge Deller
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-06-26 5:09 UTC (permalink / raw)
To: Ian Bridges; +Cc: dri-devel
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 <icb@fastmail.org>
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] fbdev: fix use-after-free in store_modes()
2026-06-26 4:50 [PATCH v2] fbdev: fix use-after-free in store_modes() Ian Bridges
2026-06-26 5:09 ` sashiko-bot
@ 2026-06-26 13:02 ` Helge Deller
1 sibling, 0 replies; 3+ messages in thread
From: Helge Deller @ 2026-06-26 13:02 UTC (permalink / raw)
To: Ian Bridges, Simona Vetter, linux-fbdev, dri-devel, linux-kernel
On 6/26/26 06:50, Ian Bridges wrote:
> store_modes() replaces a framebuffer's modelist with modes from userspace.
> On success it frees the old modelist with fb_destroy_modelist(). Two
> fields still point into that freed list.
>
> One pointer is fb_display[i].mode, the mode a console is using.
> fbcon_new_modelist() moves these pointers to the new list. It only does so
> for consoles still mapped to the framebuffer. An unmapped console is
> skipped and keeps its stale pointer. Unbinding fbcon, for example, sets
> con2fb_map[i] to -1 but leaves fb_display[i].mode set. An
> FBIOPUT_VSCREENINFO ioctl with FB_ACTIVATE_INV_MODE later reaches
> fbcon_mode_deleted(). That function reads the stale fb_display[i].mode
> through fb_mode_is_equal(). The read is a use-after-free.
>
> The other pointer is fb_info->mode, the current mode. It is set through
> the mode sysfs attribute. store_modes() does not update fb_info->mode, so
> it is left pointing into the freed list. show_mode(), the attribute's read
> handler, dereferences the stale fb_info->mode through mode_string(). The
> read is a use-after-free.
>
> Clear both pointers before freeing the list. Commit a1f305893074 ("fbcon:
> Set fb_display[i]->mode to NULL when the mode is released") added the
> helper fbcon_delete_modelist(). It clears every fb_display[i].mode that
> points into a given list. So far it is called only from the unregister
> path. Call it from store_modes() too, and set fb_info->mode to NULL.
>
> Reported-by: syzbot+81c7c6b52649fd07299d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=81c7c6b52649fd07299d
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/all/ajjoDhAi2y4ArSlz@dev/
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ian Bridges <icb@fastmail.org>
> ---
> Added in v2: clear fb_info->mode, which is left dangling by the same free
> in store_modes(). Sashiko flagged that second pointer while reviewing
> v1 [1].
>
> [1] https://lore.kernel.org/all/20260622080749.D7FC61F000E9@smtp.kernel.org/
>
> drivers/video/fbdev/core/fbsysfs.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
applied.
Thanks!
Helge
^ permalink raw reply [flat|nested] 3+ messages in thread