Re: [PATCH] fbdev: fix use-after-free of fb_display[i].mode in store_modes()

[Ian Bridges <icb@fastmail.org>]

On Mon, Jun 22, 2026 at 08:07:49AM +0000, sashiko-bot@kernel.org wrote:

I'm new to submitting kernel patches, so please correct me if I have
handled this wrong. All four findings are pre-existing and none are
introduced by this patch, which I scoped to the reported
fb_display[i].mode use-after-free. Responses inline.

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Use-After-Free in show_mode() due to dangling fb_info->mode pointer.
> - [High] Data Race and potential Use-After-Free / list corruption when iterating fb_info->modelist.
> - [High] NULL pointer dereference in fbcon_new_modelist() for unallocated consoles.
> - [Medium] Data Race and locking violation modifying fb_display in do_unregister_framebuffer().
> --
> 
> commit 5e256dfc1372231ec16feb6ee8a3218859b0dd4f
> Author: Ian Bridges <icb@fastmail.org>
> 
> fbdev: fix use-after-free of fb_display[i].mode in store_modes()
> 
> store_modes() replaces a framebuffer's modelist and frees the old entries
> with fb_destroy_modelist(), but fb_display[i].mode was left dangling.
> This commit fixes the use-after-free by clearing the stale pointers with
> fbcon_delete_modelist() before freeing the old list.
> 
> Closes: https://syzkaller.appspot.com/bug?extid=81c7c6b52649fd07299d
> 
> > diff --git a/drivers/video/fbdev/core/fbsysfs.c b/drivers/video/fbdev/core/fbsysfs.c
> > index baa2bae0fb5b3..c761dcf21dafb 100644
> > --- a/drivers/video/fbdev/core/fbsysfs.c
> > +++ b/drivers/video/fbdev/core/fbsysfs.c
> > @@ -111,8 +111,10 @@ static ssize_t store_modes(struct device *device,
> >  	if (fb_new_modelist(fb_info)) {
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can this fb_new_modelist()
> call lead to a NULL pointer dereference for unallocated consoles?
> 
> If an unallocated console (where vc_cons[i].d is NULL) is mapped to a
> framebuffer, fbcon_set_disp() populates fb_display[i].mode and returns early.
> 
> When store_modes() calls fb_new_modelist() -> fbcon_new_modelist(),
> it checks if con2fb_map and fb_display[i].mode are set. Could it then fetch
> vc = vc_cons[i].d and unconditionally dereference vc->vc_num?
> 
> drivers/video/fbdev/core/fbcon.c:fbcon_new_modelist() {
>     ...
>     vc = vc_cons[i].d;
>     ...
>     fbcon_set_disp(info, &var, vc->vc_num);
>     ...
> }

Confirmed. fbcon_new_modelist() dereferences vc_cons[i].d without a NULL
check, and a console can have con2fb_map and fb_display[i].mode set while
vc_cons[i].d is NULL. I hit this while writing the reproducer for this bug.
It is a separate issue, so I'd argue it needs its own patch adding the NULL
check.

> 
> >  		fb_destroy_modelist(&fb_info->modelist);
> >  		list_splice(&old_list, &fb_info->modelist);
> > -	} else
> > +	} else {
> > +		fbcon_delete_modelist(&old_list);
> 
> [Severity: Medium]
> This is a pre-existing issue, but since we are properly calling
> fbcon_delete_modelist() under console_lock() here, should other callers do
> the same?
> 
> Looking at do_unregister_framebuffer(), it executes without holding
> console_lock():
> 
> drivers/video/fbdev/core/fbmem.c:do_unregister_framebuffer() {
>     ...
>     fbcon_delete_modelist(&fb_info->modelist);
>     ...
> }

Agreed. do_unregister_framebuffer() runs under registration_lock, not
console_lock, so the fbcon_delete_modelist() call added by a1f305893074
writes fb_display[] without console_lock. The call this patch adds to
store_modes() runs under console_lock and is safe. The unregister call site
looks worth a separate fix.

> 
> Since fbcon_delete_mode() iterates the global fb_display array and locklessly
> assigns NULL to fb_display[i].mode, could this cause a data race with
> concurrent readers like FBIOPUT_VSCREENINFO?
> 
> >  		fb_destroy_modelist(&old_list);
> 
> [Severity: High]
> This is a pre-existing issue, but does destroying old_list leave
> fb_info->mode dangling?
> 
> If fb_info->mode was previously set to point into a struct fb_modelist
> element from this list, it doesn't appear to be cleared before the list
> is freed.
> 
> Could a subsequent read of the sysfs mode attribute trigger a use-after-free
> here:
> 
> drivers/video/fbdev/core/fbsysfs.c:show_mode() {
>     ...
>     return mode_string(buf, 0, fb_info->mode);
> }

Confirmed, and it still reproduces with this patch applied. store_modes()
frees the old modelist that fb_info->mode points into, and this patch only
clears fb_display[i].mode. Writing a mode to /sys/class/graphics/fb0/mode,
then a new list to /sys/class/graphics/fb0/modes, then reading
/sys/class/graphics/fb0/mode faults in mode_string() called from
show_mode(). Since it is the same free in store_modes(), I will fold the
fix into a v2 of this patch and clear fb_info->mode alongside
fb_display[i].mode.

> 
> [Severity: High]
> This is also a pre-existing issue, but does modifying fb_info->modelist
> under lock_fb_info() race with sysfs readers?
> 
> Looking at show_modes() and store_mode(), they appear to iterate
> fb_info->modelist locklessly:
> 
> drivers/video/fbdev/core/fbsysfs.c:show_modes() {
>     ...
>     list_for_each_entry(modelist, &fb_info->modelist, list) {
>     ...
> }

Agreed. show_modes() and store_mode() walk fb_info->modelist without
lock_fb_info(), so they can race the splice and free in store_modes(). This
looks like a separate locking fix.

> 
> Since store_modes() splices, modifies, and destroys this list, could a
> concurrent sysfs read/write follow corrupted pointers into freed memory?
> 
> > +	}
> >  
> >  	unlock_fb_info(fb_info);
> >  	console_unlock();
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/ajjoDhAi2y4ArSlz@dev?part=1

To summarise, I will send a v2 of this patch that clears both stale pointers
in store_modes() (fb_display[i].mode and fb_info->mode), and keep the
fbcon_new_modelist() NULL check and the two locking issues as separate
patches.

Thanks,
Ian