linux-kernel - Re: [Linaro-mm-sig] Re: [PATCH v2] drm/gem: Fix GEM handle release errors

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:   Tue, 9 Aug 2022 18:02:27 +0800
From:   Chen Jeffy <jeffy.chen@...k-chips.com>
To:     Christian König <christian.koenig@....com>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Cc:     Andy Yan <andy.yan@...k-chips.com>,
        Jianqun Xu <jay.xu@...k-chips.com>,
        Maxime Ripard <mripard@...nel.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linaro-mm-sig@...ts.linaro.org, David Airlie <airlied@...ux.ie>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        linux-media@...r.kernel.org, Daniel Vetter <daniel@...ll.ch>
Subject: Re: [Linaro-mm-sig] Re: [PATCH v2] drm/gem: Fix GEM handle release
 errors

Hi Christian,

On 8/9 星期二 17:08, Christian König wrote:
> Hi Jeffy,
> 
> Am 09.08.22 um 09:55 schrieb Christian König:
>> [SNIP]
>>>>
>>>>>
>>>>>
>>>>> So we are allowing GEM object to have multiple handles, and GEM 
>>>>> object could have at most one dma-buf, doesn't that means that 
>>>>> dma-buf could map to multiple handles?
>>>>
>>>> No, at least not for the same GEM file private. That's the reason 
>>>> why the rb is indexed by the dma_buf object and not the handle.
>>>>
>>>> In other words the rb is so that you have exactly one handle for 
>>>> each dma_buf in each file private.
>>>
>>> I don't think so, because if user get multiple handles for the same 
>>> GEM obj and use drm_gem_prime_handle_to_fd() for those handles
>>
>> Mhm, that works? This is illegal and should have been prevented somehow.
> 
> At least I see the problem now. I'm just not sure how to fix it.
> 
> Your v2 patch indeed prevents leakage of the drm_prime_member for the 
> additional handles, but those shouldn't have been added in the first place.
> 
> The issue is that with this we make it unpredictable which handle is 
> returned. E.g. if we have handle 2,5,7 it can be that because of 
> re-balancing the tree sometimes 2 and sometimes 5 is returned.

Maybe cache the latest returned handle in the obj(after 
drm_gem_prime_fd_to_handle), and clear it when that handle been deleted 
in drm_gem_handle_delete()?

Something like:
drm_gem_prime_fd_to_handle
   handle = drm_prime_lookup_buf_handle(buf)
   obj = obj_from_handle(handle)
   if !obj->primary_handle
     obj->primary_handle = handle
   return obj->primary_handle

Or maybe limit GEM obj with a single lifetime handle?

> 
> That's not really a good idea and breaks a couple of assumptions as far 
> as I know.
> 
> Ideas?
> 
> Thanks,
> Christian.
>