Messages in this thread | | | Date | Tue, 10 May 2022 00:22:55 +0200 | Subject | Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release() | From | Andrzej Hajda <> |
| |
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) >
| |