Re: [PATCH] fbdev: vfb: defer cleanup until the last reference

[sashiko-bot@kernel.org]

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 <bestswngs@gmail.com>

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/f2Kf9GYn1lKR5S1dbvGVtykMxK1RlgP5z8sW@bugs.sh/

> 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/20260909191022.847295-2-bestswngs@gmail.com?part=1