Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release()

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

 





On 09.05.2022 22:03, Javier Martinez Canillas wrote:
On 5/9/22 20:12, Thomas Zimmermann wrote:

[snip]

I actually thought about the same but then remembered what Daniel said in [0]
(AFAIU at least) that these should be done in .remove() so the current code
looks like matches that and only framebuffer_release() should be moved.

For vesafb a previous patch proposed to also move a release_region() call to
.fb_destroy() and Daniel also said that it was iffy and shouldn't be done [1].

But I'm also not fb expert so happy to move fb_dealloc_cmap() as well if that
is the correct thing to do.
The cmap data structure is software state that can be accessed via icotl
as long as the devfile is open. Drivers update the hardware from it. See
[1].  Moving that cleanup into fb_destroy seems appropriate to me.

I see, that makes sense. Then something like the following instead?

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..ce0d89c49e42 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -627,12 +627,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
         cancel_work_sync(&fb_helper->resume_work);
         cancel_work_sync(&fb_helper->damage_work);
- info = fb_helper->fbdev;
-       if (info) {
-               if (info->cmap.len)
-                       fb_dealloc_cmap(&info->cmap);
-               framebuffer_release(info);
-       }
         fb_helper->fbdev = NULL;
mutex_lock(&kernel_fb_helper_lock);
@@ -2111,7 +2105,11 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
   */
  static void drm_fbdev_fb_destroy(struct fb_info *info)
  {
+       if (info->cmap.len)
+               fb_dealloc_cmap(&info->cmap);
+
         drm_fbdev_release(info->par);
+       framebuffer_release(info);

I would put drm_fbdev_release at the beginning - it cancels workers which could expect cmap to be still valid.

Regards
Andrzej


  }
static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)





[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