Re: [PATCH RFC] mm: warn potential return NULL for kmalloc_array and kvmalloc_array with __GFP_NOFAIL

[Barry Song <21cnbao@gmail.com>]

On Thu, Jul 18, 2024 at 6:58 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 18-07-24 11:00:25, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > 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@suse.com>
> > Cc: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Lorenzo Stoakes <lstoakes@gmail.com>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: Pekka Enberg <penberg@kernel.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > Cc: Vlastimil Babka <vbabka@suse.cz>
> > Cc: Roman Gushchin <roman.gushchin@linux.dev>
> > Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> > ---
> >  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