Re: [PATCH v2] fbdev: fix use-after-free 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] 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