On Thu, Jul 18, 2024 at 7:55 PM Michal Hocko <mhocko@xxxxxxxx> wrote: > > On Thu 18-07-24 19:41:33, Barry Song wrote: > > On Thu, Jul 18, 2024 at 7:27 PM Michal Hocko <mhocko@xxxxxxxx> 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
