On Wed, May 04, 2022 at 11:57:22PM +0200, Javier Martinez Canillas wrote:
The driver is calling framebuffer_release() in its .remove callback, but this will cause the struct fb_info to be freed too early. Since it could be that a reference is still hold to it if user-space opened the fbdev.
This would lead to a use-after-free error if the framebuffer device was unregistered but later a user-space process tries to close the fbdev fd.
The correct thing to do is to only unregister the framebuffer in the driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy.
Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Javier Martinez Canillas javierm@redhat.com
I think this should have a Fixes: line for the patch from Thomas which changed the remove_conflicting_fb code:
27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
I think we should also mention that strictly speaking the code flow is now wrong, because hw cleanup (like iounmap) should be done from ->remove while sw cleanup (like calling framebuffer_release()) is the only thing that should be done from ->fb_destroy. But the current code matches what was happening before 27599aacbaef so more minimal "fix"
With those details added Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Same for the next patch. -Daniel
drivers/video/fbdev/simplefb.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index 94fc9c6d0411..2c198561c338 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -84,6 +84,10 @@ struct simplefb_par { static void simplefb_clocks_destroy(struct simplefb_par *par); static void simplefb_regulators_destroy(struct simplefb_par *par);
+/*
- fb_ops.fb_destroy is called by the last put_fb_info() call at the end
- of unregister_framebuffer() or fb_release(). Do any cleanup here.
- */
static void simplefb_destroy(struct fb_info *info) { struct simplefb_par *par = info->par; @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info) if (info->screen_base) iounmap(info->screen_base);
- framebuffer_release(info);
- if (mem) release_mem_region(mem->start, resource_size(mem));
} @@ -545,8 +551,8 @@ static int simplefb_remove(struct platform_device *pdev) { struct fb_info *info = platform_get_drvdata(pdev);
- /* simplefb_destroy takes care of info cleanup */ unregister_framebuffer(info);
framebuffer_release(info);
return 0;
}
2.35.1