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

[sashiko-bot@kernel.org]

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);
    ...
}

>  		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);
    ...
}

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);
}

[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) {
    ...
}

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