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 7:55 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 18-07-24 19:41:33, Barry Song wrote:
> > On Thu, Jul 18, 2024 at 7:27 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Thu 18-07-24 19:22:37, Barry Song wrote:
> > > [...]
> > > > 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?
> > >
> > > Only if you allow to map zero page AFAIK. Nobody reasonable should be
> > > doing that.
> >
> > ptr->callback could be above /proc/sys/vm/mmap_min_addr ?
>
> Yes, it can of course but this would require quite a stretch to trigger,
> no?
>
> Look at this from a real life code POV. You are allocating an array of
> callbacks (or structure of callbacks). In order to have this exploitable
> you need to direct the first dereference above mmap_min_addr.
>
> If you really want to protect from a code like that then WARN_ON doesn't
> buy you anything because it will stop the exploit only when
> panic_on_warn. You would need BUG_ON as mentioned by Christoph.
>

I actually also mentioned BUG_ON in the changelog "Likely BUG_ON()
seems better as anyway we can't fix it?" though the code is WARN_ON.

> So the real question is, do you want to stop exploits or do you want to
> debug potentially incorrect but mostly harmless buggy code?

I want to ensure that GFP_NOFAIL has consistent semantics—we don't
check the return value, and it must succeed, although allocating a large
amount of NOFAIL(I mean a number less than overflow) memory might
make the system "unusable". While GFP_NOFAIL itself behaves correctly,
it's inappropriate for the caller.

So the purpose is making sure the semantics - NOFAIL means no failure
and we don't need to check ret.  If we can't really succeed, it should throw
a BUG to stop any potential exploits.

> --
> Michal Hocko
> SUSE Labs

Thanks
Barry