On 19.08.24 14:48, Barry Song wrote:
On Tue, Aug 20, 2024 at 12:33 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
On 19.08.24 12:02, Barry Song wrote:
On Mon, Aug 19, 2024 at 9:55 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
On 19.08.24 11:47, Barry Song wrote:
On Mon, Aug 19, 2024 at 9:43 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
On 17.08.24 08:24, Barry Song wrote:
From: Barry Song <v-songbaohua@xxxxxxxx>
We have cases we still fail though callers might have __GFP_NOFAIL. Since
they don't check the return, we are exposed to the security risks for NULL
deference.
Though BUG_ON() is not encouraged by Linus, this is an unrecoverable
situation.
Christoph Hellwig:
The whole freaking point of __GFP_NOFAIL is that callers don't handle
allocation failures. So in fact a straight BUG is the right thing
here.
Vlastimil Babka:
It's just not a recoverable situation (WARN_ON is for recoverable
situations). The caller cannot handle allocation failure and at the same
time asked for an impossible allocation. BUG_ON() is a guaranteed oops
with stracktrace etc. We don't need to hope for the later NULL pointer
dereference (which might if really unlucky happen from a different
context where it's no longer obvious what lead to the allocation failing).
Michal Hocko:
Linus tends to be against adding new BUG() calls unless the failure is
absolutely unrecoverable (e.g. corrupted data structures etc.). I am
not sure how he would look at simply incorrect memory allocator usage to
blow up the kernel. Now the argument could be made that those failures
could cause subtle memory corruptions or even be exploitable which might
be a sufficient reason to stop them early.
Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
Cc: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
Cc: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
Cc: Christoph Lameter <cl@xxxxxxxxx>
Cc: Pekka Enberg <penberg@xxxxxxxxxx>
Cc: David Rientjes <rientjes@xxxxxxxxxx>
Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
Cc: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Cc: Kees Cook <kees@xxxxxxxxxx>
Cc: "Eugenio Pérez" <eperezma@xxxxxxxxxx>
Cc: Hailong.Liu <hailong.liu@xxxxxxxx>
Cc: Jason Wang <jasowang@xxxxxxxxxx>
Cc: Maxime Coquelin <maxime.coquelin@xxxxxxxxxx>
Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
Cc: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
---
include/linux/slab.h | 4 +++-
mm/page_alloc.c | 4 +++-
mm/util.c | 1 +
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index c9cb42203183..4a4d1fdc2afe 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -827,8 +827,10 @@ kvmalloc_array_node_noprof(size_t n, size_t size, gfp_t flags, int node)
{
size_t bytes;
- if (unlikely(check_mul_overflow(n, size, &bytes)))
+ if (unlikely(check_mul_overflow(n, size, &bytes))) {
+ BUG_ON(flags & __GFP_NOFAIL);
return NULL;
+ }
return kvmalloc_node_noprof(bytes, flags, node);
}
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;
+ }
gfp &= gfp_allowed_mask;
/*
diff --git a/mm/util.c b/mm/util.c
index ac01925a4179..678c647b778f 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -667,6 +667,7 @@ void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node)
/* Don't even allow crazy sizes */
if (unlikely(size > INT_MAX)) {
+ BUG_ON(flags & __GFP_NOFAIL);
No new BUG_ON please. WARN_ON_ONCE() + recovery code might be suitable here.
Hi David,
WARN_ON_ONCE() might be fine but I don't see how it is possible to recover.
Just return NULL? "shit in shit out" :) ?
Returning NULL is perfectly right if gfp doesn't include __GFP_NOFAIL,
as it's the caller's responsibility to check the return value. However, with
__GFP_NOFAIL, users will directly dereference *(p + offset) even when
p == NULL. It is how __GFP_NOFAIL is supposed to work.
If the caller is not supposed to pass that flag combination (shit in),
we are not obligated to give a reasonable result (shit out).
My point is that we should let the caller (possibly?) crash -- the one
that did something that is wrong -- instead of forcing a crash using
BUG_ON in this code here.
It should all be caught during testing either way. And if some OOT
module does something nasty, that's not our responsibility.
BUG_ON is not a way to write assertions into the code.
It seems there was a misunderstanding regarding the purpose of
this change. we actually have many details in changelog.
Its aim is not to write an assertion, but rather to prevent exposing
a security vulnerability.
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?
That might result in an issue elsewhere, but likely no security
vulnerability?
I really hate each and every BUG_ON I have to stare at.
--
Cheers,
David / dhildenb
