fbdev: fix use-after-free in store_modes()

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 <[email protected]>

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.

index d9743ef353552..ea196603c7a87 100644

[ ... ]

@@ -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?

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

drivers/video/fbdev/core/fbmem.c:fb_set_var() {
        ...
        ret = fbcon_mode_deleted(info, &mode1);
        if (!ret)
                fb_delete_videomode(&mode1, &info->modelist);
        ...
}

Sashiko AI review · https://sashiko.dev/#/patchset/aj4FKJtIuU97rEA4@dev?part=1