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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux