linux-kernel - Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in fb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 May 2022 20:12:28 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        linux-kernel@...r.kernel.org
Cc:     linux-fbdev@...r.kernel.org,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Helge Deller <deller@....de>, dri-devel@...ts.freedesktop.org,
        Daniel Vetter <daniel.vetter@...el.com>
Subject: Re: [PATCH v3 1/4] fbdev: Prevent possible use-after-free in
 fb_release()

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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)