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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 2227 bytes --]

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]