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

From: Thomas Zimmermann
Date: Mon May 09 2022 - 14:14:00 EST

Hi Javier

Am 09.05.22 um 18:33 schrieb Javier Martinez Canillas:
On 5/9/22 17:51, Andrzej Hajda wrote:

[snip]

+
Regarding drm:
What about drm_fb_helper_fini? It calls also framebuffer_release and is
called often from _remove paths (checked intel/radeon/nouveau). I guess
it should be fixed as well. Do you plan to fix it?

I think you are correct. Maybe we need something like the following?

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d265a73313c9..b09598f7af28 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -631,7 +631,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
if (info) {
if (info->cmap.len)
fb_dealloc_cmap(&info->cmap);
- framebuffer_release(info);

What about cmap? I am not an fb expert, but IMO cmap can be accessed
from userspace as well.


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.

Best regards
Thomas

[1] https://elixir.bootlin.com/linux/v5.17.6/source/drivers/video/fbdev/core/fbcmap.c#L231


[0]: https://www.spinics.net/lists/dri-devel/msg346257.html
[1]: https://www.spinics.net/lists/dri-devel/msg346261.html


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