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

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

[Weiming Shi]

FBIOGETCMAP takes a shallow snapshot of info->cmap and performs the
usercopy after dropping info->lock. vfb_remove() frees the colormap
immediately after unregistering the framebuffer, even when an open file
still holds a reference to fb_info. A concurrent driver unbind can
therefore free the colormap while the ioctl copies it to userspace.

KASAN reports:

  BUG: KASAN: slab-use-after-free in _copy_to_user
  Read of size 512 by task poc/125

   _copy_to_user (./include/linux/instrumented.h:129 ./include/linux/uaccess.h:201 lib/usercopy.c:24)
   fb_cmap_to_user (./include/linux/uaccess.h:230 drivers/video/fbdev/core/fbcmap.c:211)
   do_fb_ioctl (drivers/video/fbdev/core/fb_chrdev.c:114)

   Allocated by task 1:
   fb_alloc_cmap_gfp (./include/linux/slab.h:973 ./include/linux/slab.h:1290 drivers/video/fbdev/core/fbcmap.c:108)
   vfb_probe (drivers/video/fbdev/vfb.c:459)

   Freed by task 124:
   fb_dealloc_cmap (drivers/video/fbdev/core/fbcmap.c:151)
   vfb_remove (drivers/video/fbdev/vfb.c:489)

unregister_framebuffer() drops the registration reference, and fbdev calls
fb_destroy after the last put_fb_info(). Move the registered framebuffer's
cleanup into an fb_destroy callback so its colormap and screen buffer stay
alive until all file references have been released.

Fixes: 5e266e2e0e19 ("vfb: fix memory leaks in removal path")
Reported-by: co+c25629c98ba36ebe@bugs.sh
Closes: https://lore.kernel.org/linux-fbdev/f2Kf9GYn1lKR5S1dbvGVtykMxK1RlgP5z8sW@bugs.sh/
Assisted-by: Codex:gpt-5
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
 drivers/video/fbdev/vfb.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

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
@@ -78,6 +78,13 @@ static int vfb_pan_display(struct fb_var_screeninfo *var,
 static int vfb_mmap(struct fb_info *info,
 		    struct vm_area_struct *vma);
 
+static void vfb_destroy(struct fb_info *info)
+{
+	vfree(info->screen_buffer);
+	fb_dealloc_cmap(&info->cmap);
+	framebuffer_release(info);
+}
+
 static const struct fb_ops vfb_ops = {
 	.owner		= THIS_MODULE,
 	__FB_DEFAULT_SYSMEM_OPS_RDWR,
@@ -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,
 };
 
     /*
@@ -485,9 +493,6 @@ static void vfb_remove(struct platform_device *dev)
 
 	if (info) {
 		unregister_framebuffer(info);
-		vfree(videomemory);
-		fb_dealloc_cmap(&info->cmap);
-		framebuffer_release(info);
 	}
 }
 
-- 
2.55.0

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

[sashiko-bot]

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

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

[Helge Deller]

On 9/9/26 21:10, Weiming Shi wrote:
> FBIOGETCMAP takes a shallow snapshot of info->cmap and performs the
> usercopy after dropping info->lock. vfb_remove() frees the colormap
> immediately after unregistering the framebuffer, even when an open file
> still holds a reference to fb_info. A concurrent driver unbind can
> therefore free the colormap while the ioctl copies it to userspace.
> 
> KASAN reports:
> 
>    BUG: KASAN: slab-use-after-free in _copy_to_user
>    Read of size 512 by task poc/125
> 
>     _copy_to_user (./include/linux/instrumented.h:129 ./include/linux/uaccess.h:201 lib/usercopy.c:24)
>     fb_cmap_to_user (./include/linux/uaccess.h:230 drivers/video/fbdev/core/fbcmap.c:211)
>     do_fb_ioctl (drivers/video/fbdev/core/fb_chrdev.c:114)
> 
>     Allocated by task 1:
>     fb_alloc_cmap_gfp (./include/linux/slab.h:973 ./include/linux/slab.h:1290 drivers/video/fbdev/core/fbcmap.c:108)
>     vfb_probe (drivers/video/fbdev/vfb.c:459)
> 
>     Freed by task 124:
>     fb_dealloc_cmap (drivers/video/fbdev/core/fbcmap.c:151)
>     vfb_remove (drivers/video/fbdev/vfb.c:489)
> 
> unregister_framebuffer() drops the registration reference, and fbdev calls
> fb_destroy after the last put_fb_info(). Move the registered framebuffer's
> cleanup into an fb_destroy callback so its colormap and screen buffer stay
> alive until all file references have been released.
> 
> Fixes: 5e266e2e0e19 ("vfb: fix memory leaks in removal path")
> Reported-by: co+c25629c98ba36ebe@bugs.sh
> Closes: https://lore.kernel.org/linux-fbdev/f2Kf9GYn1lKR5S1dbvGVtykMxK1RlgP5z8sW@bugs.sh/
> Assisted-by: Codex:gpt-5
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
>   drivers/video/fbdev/vfb.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
applied.

Thanks!
Helge