Re: [PATCH] mm/mempolicy.c: Fix out of bounds write in mpol_parse_str()

[Dmitry Vyukov <dvyukov@google.com>]

On Wed, Jan 15, 2020 at 8:05 PM Michal Hocko <mhocko@kernel.org> wrote:
> > > > On Wed, Jan 15, 2020 at 1:54 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > > > >
> > > > > On 1/15/20 6:54 AM, Dan Carpenter wrote:
> > > > > > What we are trying to do is change the '=' character to a NUL terminator
> > > > > > and then at the end of the function we restore it back to an '='.  The
> > > > > > problem is there are two error paths where we jump to the end of the
> > > > > > function before we have replaced the '=' with NUL.  We end up putting
> > > > > > the '=' in the wrong place (possibly one element before the start of
> > > > > > the buffer).
> > > > >
> > > > > Bleh.
> > > > >
> > > > > > Reported-by: syzbot+e64a13c5369a194d67df@syzkaller.appspotmail.com
> > > > > > Fixes: 095f1fc4ebf3 ("mempolicy: rework shmem mpol parsing and display")
> > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > >
> > > > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > > > >
> > > > > CC stable perhaps? Can this (tmpfs mount options parsing AFAICS?) become
> > > > > part of unprivileged operation in some scenarios?
> > > >
> > > > Yes, tmpfs can be mounted by any user inside of a user namespace.
> > >
> > > Huh, is there any restriction though? It is certainly not nice to have
> > > an arbitrary memory allocated without a way of reclaiming it and OOM
> > > killer wouldn't help for shmem.
> >
> > The last time I checked there were hundreds of ways to allocate
> > arbitrary amounts of memory without any restrictions by any user. The
> > example at hand was setting up GB-sized netfilter tables in netns
> > under userns. It's not subject to ulimit/memcg.
>
> That's bad!
>
> > Most kmalloc/vmalloc's are not accounted and can be abused.
>
> Many of those should be bound to some objects and if those are directly
> controllable by userspace then we should account at least. And if they
> are not bound to a process life time then restricted.

I see you actually added one GFP_ACCOUNT in netfilter in "netfilter:
x_tables: do not fail xt_alloc_table_info too easilly". But it seems
there are more:

$ grep vmalloc\( net/netfilter/*.c
net/netfilter/nf_tables_api.c: return kvmalloc(alloc, GFP_KERNEL);
net/netfilter/x_tables.c: xt[af].compat_tab = vmalloc(mem);
net/netfilter/x_tables.c: mem = vmalloc(len);
net/netfilter/x_tables.c: info = kvmalloc(sz, GFP_KERNEL_ACCOUNT);
net/netfilter/xt_hashlimit.c: /* FIXME: don't use vmalloc() here or
anywhere else -HW */
net/netfilter/xt_hashlimit.c: hinfo = vmalloc(struct_size(hinfo, hash, size));

These are not bound to processes/threads as namespaces are orthogonal to tasks.

Somebody told me that it's not good to use GFP_ACCOUNT if the
allocation is not tied to the lifetime of the process. Is it still
true?

In the end if user controls either size or number allocations, they
should be accounted, and it seems we still have thousands of
unaccounted ones. There are dozens of kmalloc's in netfilter code and
none of them use GFP_ACCOUNT...



> > Is tmpfs even worse than these?
>
> Well, tmpfs is accounted and restricted by memcg at least. The problem
> is that it the memory is not really bound to a process life time which
> makes it effectively unreclaimable once the swap space is depleted.
> Still bad.

I see. If I understand it correctly, this one is actually better than
all these non-GFP_ACCOUNT allocations.
If I would DoS a box (intentionally or unintentionally, just a bug in
my program) I would probably go for one of these easier ones without
GFP_ACCOUNT.