Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather than .remove

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 04.05.22 um 23:57 schrieb Javier Martinez Canillas:
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@xxxxxxxx>
Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Please see my question below.

---

  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));

The original problem with fbdev hot-unplug was that vmwgfx needed the framebuffer region to be released. If we release it only after userspace closed it's final file descriptor, vmwgfx could have already failed.

I still don't fully get why this code apparently works or at least doesn't blow up occasionally. Any ideas?

Best regards
Thomas

  }
@@ -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;
  }

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux