linux-kernel - Re: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation

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] [day] [month] [year] [list]

Message-ID: <6b8182a1-7cdc-7369-5c34-e6d0c24efcca@amd.com>
Date:   Fri, 14 Jan 2022 08:16:30 +0100
From:   Christian König <christian.koenig@....com>
To:     John Stultz <john.stultz@...aro.org>
Cc:     "Ruhl, Michael J" <michael.j.ruhl@...el.com>,
        "guangming.cao@...iatek.com" <guangming.cao@...iatek.com>,
        "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "wsd_upstream@...iatek.com" <wsd_upstream@...iatek.com>,
        "libo.kang@...iatek.com" <libo.kang@...iatek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "yf.wang@...iatek.com" <yf.wang@...iatek.com>,
        "linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "lmark@...eaurora.org" <lmark@...eaurora.org>,
        "benjamin.gaignard@...aro.org" <benjamin.gaignard@...aro.org>,
        "bo.song@...iatek.com" <bo.song@...iatek.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "labbott@...hat.com" <labbott@...hat.com>,
        "mingyuan.ma@...iatek.com" <mingyuan.ma@...iatek.com>,
        "jianjiao.zeng@...iatek.com" <jianjiao.zeng@...iatek.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>
Subject: Re: [PATCH v3] dma-buf: dma-heap: Add a size check for allocation

Am 14.01.22 um 00:26 schrieb John Stultz:
> On Thu, Jan 13, 2022 at 5:05 AM Christian König
> <christian.koenig@....com> wrote:
>> Am 13.01.22 um 14:00 schrieb Ruhl, Michael J:
>>>> -----Original Message-----
>>>> From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On Behalf Of
>>>> Ruhl, Michael J
>>>>> -----Original Message-----
>>>>> From: dri-devel <dri-devel-bounces@...ts.freedesktop.org> On Behalf Of
>>>>> guangming.cao@...iatek.com
>>>>> +   /*
>>>>> +    * Invalid size check. The "len" should be less than totalram.
>>>>> +    *
>>>>> +    * Without this check, once the invalid size allocation runs on a process
>>>>> that
>>>>> +    * can't be killed by OOM flow(such as "gralloc" on Android devices), it
>>>>> will
>>>>> +    * cause a kernel exception, and to make matters worse, we can't find
>>>>> who are using
>>>>> +    * so many memory with "dma_buf_debug_show" since the relevant
>>>>> dma-buf hasn't exported.
>>>>> +    */
>>>>> +   if (len >> PAGE_SHIFT > totalram_pages())
>>>> If your "heap" is from cma, is this still a valid check?
>>> And thinking a bit further, if I create a heap from something else (say device memory),
>>> you will need to be able to figure out the maximum allowable check for the specific
>>> heap.
>>>
>>> Maybe the heap needs a callback for max size?
>> Well we currently maintain a separate allocator and don't use dma-heap,
>> but yes we have systems with 16GiB device and only 8GiB system memory so
>> that check here is certainly not correct.
> Good point.
>
>> In general I would rather let the system run into -ENOMEM or -EINVAL
>> from the allocator instead.
> Probably the simpler solution is to push the allocation check to the
> heap driver, rather than doing it at the top level here.
>
> For CMA or other contiguous heaps, letting the allocator fail is fast
> enough. For noncontiguous buffers, like the system heap, the
> allocation can burn a lot of time and consume a lot of memory (causing
> other trouble) before a large allocation might naturally fail.

Yeah, letting a alloc_page() loop run for a while is usually not nice at 
all :)

You can still do a sanity check here, e.g. the size should never have 
the most significant bit set for example.

Regards,
Christian.

>
> thanks
> -john