[PATCH v2] fbdev: fix use-after-free in store_modes()

[PATCH v2] fbdev: fix use-after-free in store_modes()

[Ian Bridges]

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(-)

diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
index d9743ef35355..ea196603c7a8 100644
--- a/drivers/video/fbdev/core/fbsysfs.c
+++ b/drivers/video/fbdev/core/fbsysfs.c
@@ -10,6 +10,7 @@
 #include <linux/major.h>
 
 #include "fb_internal.h"
+#include "fbcon.h"
 
 static int activate(struct fb_info *fb_info, struct fb_var_screeninfo *var)
 {
@@ -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);
-	} 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;
 		fb_destroy_modelist(&old_list);
+	}
 
 	unlock_fb_info(fb_info);
 	console_unlock();
-- 
2.47.3

Re: [PATCH v2] fbdev: fix use-after-free in store_modes()

[sashiko-bot]

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

Re: [PATCH v2] fbdev: fix use-after-free in store_modes()

[Helge Deller]

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