Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] fb_release calls module_put before put_fb_info, allowing a concurrent module unload to unmap the driver's code before fb_destroy is executed. - [High] If fbcon_fb_registered fails, do_register_framebuffer returns an error without unwinding the registration, leaving a freed pointer in the global registered_fb array. - [High] Concurrent execution of `unregister_framebuffer` and `FBIOPUT_VSCREENINFO` causes Use-After-Free on `info->modelist`. -- commit 58269d102e8231ba399afb90f35aad905f703fbf Author: Weiming Shi <[email protected]> fbdev: vfb: defer cleanup until the last reference This patch defers the freeing of the vfb colormap and screen buffer until the final reference to the framebuffer is released. It accomplishes this by moving the cleanup code into a new fb_destroy callback to prevent use-after-free issues during concurrent ioctls. Link: https://lore.kernel.org/linux-fbdev/[email protected]/ > diff --git a/drivers/video/fbdev/vfb.c b/drivers/video/fbdev/vfb.c > index 5b7965f36c5ed..de137b2bdaedc 100644 > --- a/drivers/video/fbdev/vfb.c > +++ b/drivers/video/fbdev/vfb.c > @@ -87,6 +94,7 @@ static const struct fb_ops vfb_ops = { > .fb_pan_display = vfb_pan_display, > __FB_DEFAULT_SYSMEM_OPS_DRAW, > .fb_mmap = vfb_mmap, > + .fb_destroy = vfb_destroy, [Severity: High] This isn't a bug introduced by this patch, but does adding this callback expose a race with module unloading? In drivers/video/fbdev/core/fb_chrdev.c:fb_release(), the module reference is released before calling put_fb_info(): module_put(info->fbops->owner); unlock_fb_info(info); put_fb_info(info); If a user opens the device and a concurrent rmmod is initiated, the rmmod blocks until the module refcount reaches zero. When the user closes the device, fb_release() calls module_put(), which unblocks rmmod and unloads the module. Then fb_release() proceeds to call put_fb_info(), which executes: if (fb_info->fbops->fb_destroy) fb_info->fbops->fb_destroy(fb_info); Since the module has been unloaded, info->fbops->fb_destroy now points to unmapped memory. Does the core framework need to defer module_put() until after put_fb_info(), or is there another mechanism preventing this? > }; > [ ... ] > @@ -485,9 +493,6 @@ static void vfb_remove(struct platform_device *dev) > > if (info) { > unregister_framebuffer(info); [Severity: High] This is another pre-existing issue in the core framework, but can unregister_framebuffer() race with concurrent ioctls modifying the modelist? In drivers/video/fbdev/core/fbmem.c:do_unregister_framebuffer(), the modelist items are freed without holding info->lock: fb_destroy_modelist(&fb_info->modelist); registered_fb[fb_info->node] = NULL; If an ioctl like FBIOPUT_VSCREENINFO concurrently executes fb_set_var() on an open file descriptor, it iterates over the modelist: if (info->modelist.prev && info->modelist.next && !list_empty(&info->modelist)) ret = fb_add_videomode(&mode, &info->modelist); Can this lead to a use-after-free when accessing the freed modelist pointers? Is it possible to hold info->lock during the modelist cleanup? [Severity: High] This isn't a bug introduced by this patch, but is there a risk of a memory leak or use-after-free in the core registration error path? In drivers/video/fbdev/core/fbmem.c:do_register_framebuffer(), if fbcon_fb_registered() fails, it returns an error but leaves the pointer in the global registered_fb[] array: return fbcon_fb_registered(fb_info); If this probe error path executes, the driver attempts to free the framebuffer, but it fails because info->count is still 1. If the driver frees the backing videomemory, subsequent opens of /dev/fb0 could fetch the leaked fb_info from the array and operate on freed videomemory. Can do_register_framebuffer() unwind the registered_fb[i] assignment on this error path? > - vfree(videomemory); > - fb_dealloc_cmap(&info->cmap); > - framebuffer_release(info); > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
