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