On Thu, Jul 18, 2024 at 6:58 PM Michal Hocko <mhocko@xxxxxxxx> wrote:
>
> On Thu 18-07-24 11:00:25, Barry Song wrote:
> > From: Barry Song <v-songbaohua@xxxxxxxx>
> >
> > Overflow in this context is highly unlikely. However, allocations using
> > GFP_NOFAIL are guaranteed to succeed, so checking the return value is
> > unnecessary. One option to fix this is allowing memory allocation with
> > an overflowed size, but it seems pointless. Let's at least issue a
> > warning. Likely BUG_ON() seems better as anyway we can't fix it?
>
> WARN_ON is effectively BUG_ON with panic_on_warn so if this happens to
> be in a user triggerable path then you would have an easy way to panic
> the whole machine. It is likely true that the kernel could oops just
> right after the failure but that could be recoverable at least.
>
> If anything I would just pr_warn with caller address or add dump_stack
> to capture the full trace. That would give us the caller that needs
> fixing without panicing the system with panic_on_warn.
>
> Btw. what has led you to create this patch? Have you encountered a
> misbehaving caller?
I didn't encounter any misbehaving callers in the in-tree code. I was
debugging another bug potentially related to kvmalloc in my out-of-tree
drivers, so I reviewed all the code and noticed the NULL return for
GFP_NOFAIL.
For future-proofing and security reasons, returning NULL for NOFAIL
still seems incorrect as the callers won't check the ret. If any future or
existing in-tree code has a potential bug which might be exploited by
hackers, for example
ptr = kvmalloc_array(NOFAIL);
ptr->callback(); //ptr=NULL;
callback could be a privilege escalation?
I'm not a security expert, so hopefully others can provide some
insights :-)
>
> Realistically speaking k*malloc(GFP_NOFAIL) with large values (still
> far from overflow) would be very dangerous as well. So I am not really
> sure this is buying us much if anything at all.
>
> > Cc: Michal Hocko <mhocko@xxxxxxxx>
> > Cc: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> > Cc: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
> > Cc: Christoph Lameter <cl@xxxxxxxxx>
> > Cc: Pekka Enberg <penberg@xxxxxxxxxx>
> > Cc: David Rientjes <rientjes@xxxxxxxxxx>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> > Cc: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> > Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
> > ---
> > include/linux/slab.h | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index a332dd2fa6cd..c6aec311864f 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -692,8 +692,10 @@ static inline __alloc_size(1, 2) void *kmalloc_array_noprof(size_t n, size_t siz
> > {
> > size_t bytes;
> >
> > - if (unlikely(check_mul_overflow(n, size, &bytes)))
> > + if (unlikely(check_mul_overflow(n, size, &bytes))) {
> > + WARN_ON(flags & __GFP_NOFAIL);
> > return NULL;
> > + }
> > if (__builtin_constant_p(n) && __builtin_constant_p(size))
> > return kmalloc_noprof(bytes, flags);
> > return kmalloc_noprof(bytes, flags);
> > @@ -794,8 +796,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))) {
> > + WARN_ON(flags & __GFP_NOFAIL);
> > return NULL;
> > + }
> >
> > return kvmalloc_node_noprof(bytes, flags, node);
> > }
> > --
> > 2.34.1
>
> --
> Michal Hocko
> SUSE Labs
Thanks
Barry
