From: Thomas Zimmermann <tzimmermann@suse.de> To: Javier Martinez Canillas <javierm@redhat.com>, Andrzej Hajda <andrzej.hajda@intel.com>, linux-kernel@vger.kernel.org Cc: linux-fbdev@vger.kernel.org, Daniel Vetter <daniel.vetter@ffwll.ch>, Helge Deller <deller@gmx.de>, dri-devel@lists.freedesktop.org, Daniel Vetter <daniel.vetter@intel.com> Subject: Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release() Date: Tue, 10 May 2022 10:04:32 +0200 [thread overview] Message-ID: <79aaea41-5dab-f896-ab3d-d6bc9a5de615@suse.de> (raw) In-Reply-To: <1c36d431-d5c0-7278-c9e0-61867e9dc174@redhat.com> [-- Attachment #1.1: Type: text/plain, Size: 4359 bytes --] Hi Am 10.05.22 um 00:42 schrieb Javier Martinez Canillas: > On 5/10/22 00:22, Andrzej Hajda wrote: > > [snip] > >>> 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. >> > > Indeed, you are correct again. [0] is the final version of the patch I've > but don't have an i915 test machine to give it a try. I'll test tomorrow > on my test systems to verify that it doesn't cause any regressions since > with other DRM drivers. You have to go through all DRM drivers that call drm_fb_helper_fini() and make sure that they free fb_info. For example armada appears to be leaking now. [1] Best regards Thomas [1] https://elixir.bootlin.com/linux/v5.17.6/source/drivers/gpu/drm/armada/armada_fbdev.c#L152 > > I think that besides this patch, drivers shouldn't need to call to the > drm_fb_helper_fini() function directly. Since that would be called during > drm_fbdev_fb_destroy() anyways. > > We should probably remove that call in all drivers and make this helper > function static and just private to drm_fb_helper functions. > > Or am I missing something here ? > > [0]: > From 5170cafcf2936da8f1c53231e3baa7d7a2b16c61 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javierm@redhat.com> > Date: Tue May 10 00:39:55 2022 +0200 > Subject: [RFT PATCH] drm/fb-helper: Don't deallocate fb colormap and free fb info > too early > > Currently these are done in drm_fb_helper_fini() but this helper is called > by drivers in their .remove callback, which could lead to a use-after-free > if a process has opened the emulated fbdev node while a driver is removed. > > For example, in i915 driver the call chain during remove is the following: > > struct pci_driver i915_pci_driver = { > ... > .remove = i915_pci_remove, > ... > }; > > i915_pci_remove > i915_driver_remove > intel_modeset_driver_remove_noirq > intel_fbdev_fini > intel_fbdev_destroy > drm_fb_helper_fini > framebuffer_release > > Later the process will close the fbdev node file descriptor leading to the > mentioned use-after-free bug in drm_fbdev_fb_destroy(), due the following: > > drm_fbdev_fb_destroy > drm_fbdev_release(info->par); <-- info was already freed on .remove > > To prevent that, let's move the framebuffer_release() call to the end of > the drm_fbdev_fb_destroy() function. > > Also, the call to fb_dealloc_cmap() in drm_fb_helper_fini() is too early > and is more correct to do it in drm_fbdev_fb_destroy() as well. After a > call to drm_fbdev_release() has been made. > > Reported-by: Andrzej Hajda <andrzej.hajda@intel.com> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index d265a73313c9..7288fbd26bcc 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); > @@ -2112,6 +2106,9 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper) > static void drm_fbdev_fb_destroy(struct fb_info *info) > { > drm_fbdev_release(info->par); > + if (info->cmap.len) > + fb_dealloc_cmap(&info->cmap); > + framebuffer_release(info); > } > > static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) -- 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 #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Thomas Zimmermann <tzimmermann@suse.de> To: Javier Martinez Canillas <javierm@redhat.com>, Andrzej Hajda <andrzej.hajda@intel.com>, linux-kernel@vger.kernel.org Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, linux-fbdev@vger.kernel.org, Helge Deller <deller@gmx.de>, dri-devel@lists.freedesktop.org, Daniel Vetter <daniel.vetter@intel.com> Subject: Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release() Date: Tue, 10 May 2022 10:04:32 +0200 [thread overview] Message-ID: <79aaea41-5dab-f896-ab3d-d6bc9a5de615@suse.de> (raw) In-Reply-To: <1c36d431-d5c0-7278-c9e0-61867e9dc174@redhat.com> [-- Attachment #1.1: Type: text/plain, Size: 4359 bytes --] Hi Am 10.05.22 um 00:42 schrieb Javier Martinez Canillas: > On 5/10/22 00:22, Andrzej Hajda wrote: > > [snip] > >>> 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. >> > > Indeed, you are correct again. [0] is the final version of the patch I've > but don't have an i915 test machine to give it a try. I'll test tomorrow > on my test systems to verify that it doesn't cause any regressions since > with other DRM drivers. You have to go through all DRM drivers that call drm_fb_helper_fini() and make sure that they free fb_info. For example armada appears to be leaking now. [1] Best regards Thomas [1] https://elixir.bootlin.com/linux/v5.17.6/source/drivers/gpu/drm/armada/armada_fbdev.c#L152 > > I think that besides this patch, drivers shouldn't need to call to the > drm_fb_helper_fini() function directly. Since that would be called during > drm_fbdev_fb_destroy() anyways. > > We should probably remove that call in all drivers and make this helper > function static and just private to drm_fb_helper functions. > > Or am I missing something here ? > > [0]: > From 5170cafcf2936da8f1c53231e3baa7d7a2b16c61 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javierm@redhat.com> > Date: Tue May 10 00:39:55 2022 +0200 > Subject: [RFT PATCH] drm/fb-helper: Don't deallocate fb colormap and free fb info > too early > > Currently these are done in drm_fb_helper_fini() but this helper is called > by drivers in their .remove callback, which could lead to a use-after-free > if a process has opened the emulated fbdev node while a driver is removed. > > For example, in i915 driver the call chain during remove is the following: > > struct pci_driver i915_pci_driver = { > ... > .remove = i915_pci_remove, > ... > }; > > i915_pci_remove > i915_driver_remove > intel_modeset_driver_remove_noirq > intel_fbdev_fini > intel_fbdev_destroy > drm_fb_helper_fini > framebuffer_release > > Later the process will close the fbdev node file descriptor leading to the > mentioned use-after-free bug in drm_fbdev_fb_destroy(), due the following: > > drm_fbdev_fb_destroy > drm_fbdev_release(info->par); <-- info was already freed on .remove > > To prevent that, let's move the framebuffer_release() call to the end of > the drm_fbdev_fb_destroy() function. > > Also, the call to fb_dealloc_cmap() in drm_fb_helper_fini() is too early > and is more correct to do it in drm_fbdev_fb_destroy() as well. After a > call to drm_fbdev_release() has been made. > > Reported-by: Andrzej Hajda <andrzej.hajda@intel.com> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/gpu/drm/drm_fb_helper.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index d265a73313c9..7288fbd26bcc 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); > @@ -2112,6 +2106,9 @@ static void drm_fbdev_release(struct drm_fb_helper *fb_helper) > static void drm_fbdev_fb_destroy(struct fb_info *info) > { > drm_fbdev_release(info->par); > + if (info->cmap.len) > + fb_dealloc_cmap(&info->cmap); > + framebuffer_release(info); > } > > static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) -- 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 #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --]
next prev parent reply other threads:[~2022-05-10 8:04 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-05 21:59 [PATCH v3 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers Javier Martinez Canillas 2022-05-05 21:59 ` Javier Martinez Canillas 2022-05-05 22:04 ` [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb_release() Javier Martinez Canillas 2022-05-05 22:04 ` Javier Martinez Canillas 2022-05-09 14:56 ` Andrzej Hajda 2022-05-09 14:56 ` Andrzej Hajda 2022-05-09 15:30 ` Javier Martinez Canillas 2022-05-09 15:51 ` Andrzej Hajda 2022-05-09 15:51 ` Andrzej Hajda 2022-05-09 16:33 ` Javier Martinez Canillas 2022-05-09 18:12 ` Thomas Zimmermann 2022-05-09 18:12 ` Thomas Zimmermann 2022-05-09 20:03 ` Javier Martinez Canillas 2022-05-09 20:03 ` Javier Martinez Canillas 2022-05-09 22:22 ` Andrzej Hajda 2022-05-09 22:22 ` Andrzej Hajda 2022-05-09 22:42 ` Javier Martinez Canillas 2022-05-09 22:42 ` Javier Martinez Canillas 2022-05-10 7:19 ` Andrzej Hajda 2022-05-10 7:19 ` Andrzej Hajda 2022-05-10 7:50 ` Javier Martinez Canillas 2022-05-10 7:50 ` Javier Martinez Canillas 2022-05-11 13:18 ` Daniel Vetter 2022-05-11 13:18 ` Daniel Vetter 2022-05-10 8:04 ` Thomas Zimmermann [this message] 2022-05-10 8:04 ` Thomas Zimmermann 2022-05-10 8:30 ` Javier Martinez Canillas 2022-05-10 8:30 ` Javier Martinez Canillas 2022-05-10 8:37 ` Thomas Zimmermann 2022-05-10 8:37 ` Thomas Zimmermann 2022-05-10 8:50 ` Thomas Zimmermann 2022-05-10 8:50 ` Thomas Zimmermann 2022-05-10 9:06 ` Javier Martinez Canillas 2022-05-10 9:06 ` Javier Martinez Canillas 2022-05-10 9:39 ` Thomas Zimmermann 2022-05-10 9:44 ` Javier Martinez Canillas 2022-05-09 18:32 ` Thomas Zimmermann 2022-05-09 18:32 ` Thomas Zimmermann 2022-05-09 20:00 ` Javier Martinez Canillas 2022-05-09 20:00 ` Javier Martinez Canillas 2022-05-11 13:15 ` Daniel Vetter 2022-05-11 13:15 ` Daniel Vetter 2022-05-05 22:04 ` [PATCH v3 2/4] fbdev: simplefb: Cleanup fb_info in .fb_destroy rather than .remove Javier Martinez Canillas 2022-05-05 22:04 ` Javier Martinez Canillas 2022-05-05 22:05 ` [PATCH v3 3/4] fbdev: efifb: " Javier Martinez Canillas 2022-05-05 22:05 ` Javier Martinez Canillas 2022-05-06 13:07 ` Andrzej Hajda 2022-05-06 13:07 ` Andrzej Hajda 2022-05-06 13:18 ` Javier Martinez Canillas 2022-05-08 15:40 ` [fbdev] c6a2b1a999: BUG:KASAN:use-after-free_in_efifb_destroy kernel test robot 2022-05-08 15:40 ` kernel test robot 2022-05-08 15:40 ` kernel test robot 2022-05-05 22:06 ` [PATCH v3 4/4] fbdev: vesafb: Cleanup fb_info in .fb_destroy rather than .remove Javier Martinez Canillas 2022-05-05 22:06 ` Javier Martinez Canillas 2022-05-06 7:34 ` [PATCH v3 0/4] fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers Javier Martinez Canillas 2022-05-06 7:34 ` Javier Martinez Canillas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=79aaea41-5dab-f896-ab3d-d6bc9a5de615@suse.de \ --to=tzimmermann@suse.de \ --cc=andrzej.hajda@intel.com \ --cc=daniel.vetter@ffwll.ch \ --cc=daniel.vetter@intel.com \ --cc=deller@gmx.de \ --cc=dri-devel@lists.freedesktop.org \ --cc=javierm@redhat.com \ --cc=linux-fbdev@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.