Re: [PATCH v3 3/4] mm: BUG_ON to avoid NULL deference while __GFP_NOFAIL fails

[David Hildenbrand <david@redhat.com>]

On 19.08.24 19:12, Michal Hocko wrote:
> On Mon 19-08-24 14:49:55, David Hildenbrand wrote:
>> On 19.08.24 14:48, Barry Song wrote:
> [...]
>>>>>>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>>>>>>> index 60742d057b05..d2c37f8f8d09 100644
>>>>>>>>> --- a/mm/page_alloc.c
>>>>>>>>> +++ b/mm/page_alloc.c
>>>>>>>>> @@ -4668,8 +4668,10 @@ struct page *__alloc_pages_noprof(gfp_t gfp, unsigned int order,
>>>>>>>>>            * There are several places where we assume that the order value is sane
>>>>>>>>>            * so bail out early if the request is out of bound.
>>>>>>>>>            */
>>>>>>>>> -     if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp))
>>>>>>>>> +     if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp)) {
>>>>>>>>> +             BUG_ON(gfp & __GFP_NOFAIL);
>>>>>>>>>                   return NULL;
>>>>>>>>> +     }
> [...]
>>> Returning NULL doesn't necessarily crash the caller's process, p->field,
>>> *(p + offset) deference could be used by hackers to exploit the system.
>>
>> See my other reply to Michal: why do we even allow to specify them
>> separately and not simply let one enforce the other?
> 
> Are you replying to this patch? This is not about a combination of
> flags. This is about the above (and other similar) boundary checks which
> return NULL if the size is deemed incorrect. 
> I think those are potential
> problems because it could be a lack of input check which could be turned
> into a potentially malicious code. Because unchecked (return value
> because NOFAIL never fails, right?) return value might even not OOPs and
> become a silent read/write into memory.

Right, that's a different kind of issue than the simple "don't pass 
stupid flag combinations" thing where "we'll fix that up for you" is 
more reasonable.

Possibly NOFAIL allocations with an allocation sizes that is not a small 
compile-time constant already has a bad smell to it, but I'm sure there 
are reasonable exceptions ...

> 
> Whether to BUG_ON or simply loop for ever in the allocator if somebody
> requests non-sleeping NOFAIL allocation is a different story.

Right, "warn + loop forever" is one alternative where you could at least 
keep the system alive to some degree. Satisfying a large allocation 
might take a long time, satisfying a "too large" allocation would take 
forever.

But as Linus says, it's all workarounds for other buggy code, to make 
buggy code less exploitable, maybe, ...

-- 
Cheers,

David / dhildenb