RE: [PATCH v2] dma-buf/heaps: system_heap: Avoid DoS by limiting single allocations to half of all memory

[김재원]

>On Thu, 06 Apr 2023 11:17:12 +0900 Jaewon Kim <jaewon31.kim@xxxxxxxxxxx> wrote:
>
>> >> +       if (len / PAGE_SIZE > totalram_pages())
>> >> +               return ERR_PTR(-ENOMEM);
>> >
>> >We're catering for a buggy caller here, aren't we?  Are such large
>> >requests ever reasonable?
>> >
>> >How about we decide what's the largest reasonable size and do a
>> >WARN_ON(larger-than-that), so the buggy caller gets fixed?
>> 
>> Yes we're considering a buggy caller. I thought even totalram_pages() / 2 in
>> the old ion system is also unreasonable. To avoid the /2, I changed it to
>> totalram_pages() though.
>> 
>> Because userspace can request that size repeately, I think WARN_ON() may be
>> called to too often, so that it would fill the kernel log buffer.
>
>Oh geeze.  I trust that userspace needs elevated privileges of some form?
>

I'm sorry I don't know the whole Android dma-buf allocation process. AFAIK
Android apps do not allocate itself, they request dma-buf memory to the
Android system process named of Allocator. Then the Allocator seems to have
privileges to use the dma-buf heap. So normal Android apps do NOT need the
privileges.

>If so, then spamming dmesg isn't an issue - root can do much worse than
>that.
>
>> Even we think WARN_ON_ONCE rather than WARN_ON, the buggy point is not kernel
>> layer. Unlike page fault mechanism, this dma-buf system heap gets the size from 
>> userspace, and it is allowing unlimited size. I think we can't fix the buggy
>> user space with the kernel warning log. So I think warning is not enough,
>> and we need a safeguard in kernel layer.
>
>I really dislike that ram/2 thing - it's so arbitrary, hence is surely
>wrong for all cases.  Is there something more thoughtful we can do?
>
>I mean, top priority here is to inform userspace that it's buggy so
>that it gets fixed (assuming this requires elevated privileges).  And
>userspace which requests (totalram_pages()/2 - 1) bytes is still buggy,
>but we did nothing to get the bug fixed.

That ram/2 seems to be arbitrary, but I thought just ram, not ram/2, is 
reasonble as a safeguard in kernel side even after the userspace process or
common library check the abnormal size.

I thought the userspace context would get -ENOMEM either in ram case or in
__GFP_RETRY_MAYFAIL case. And -ENOMEM is enough to inform. The usespace
may not fully recognize that it is buggy though.

I'm not sure that all the userspace context use the Android libdmabufheap
library and there is no other route directly using kernel dma-buf heap
ioctl. Anyway do you think size checking code should be added to the
libdmabufheap rather than kernel side?