linux-kernel - Re: [PATCH] mm/mempolicy.c: Fix out of bounds write in mpol_parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200116073922.GL19428@dhcp22.suse.cz>
Date:   Thu, 16 Jan 2020 08:39:22 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     Vlastimil Babka <vbabka@...e.cz>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Lee Schermerhorn <lee.schermerhorn@...com>,
        Linux-MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>,
        syzbot <syzbot+e64a13c5369a194d67df@...kaller.appspotmail.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Hugh Dickins <hughd@...gle.com>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Al Viro <viro@...iv.linux.org.uk>, yang.shi@...ux.alibaba.com
Subject: Re: [PATCH] mm/mempolicy.c: Fix out of bounds write in
 mpol_parse_str()

On Thu 16-01-20 06:41:46, Dmitry Vyukov wrote:
> On Wed, Jan 15, 2020 at 8:05 PM Michal Hocko <mhocko@...nel.org> wrote:
> > > > > On Wed, Jan 15, 2020 at 1:54 PM Vlastimil Babka <vbabka@...e.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@...kaller.appspotmail.com
> > > > > > > Fixes: 095f1fc4ebf3 ("mempolicy: rework shmem mpol parsing and display")
> > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> > > > > >
> > > > > > Acked-by: Vlastimil Babka <vbabka@...e.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.

I cannot really comment on those. This is for networking people to
examine and find out whether they allow an untrusted user to runaway.

> 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?

Those are more tricky. Mostly because there is no way to reclaim the
memory once the hard limit is hit. Even the memcg oom killer will not
help much. So a care should be taken when adding GFP_ACCOUNT for those.
On the other hand it would prevent an unbounded allocations at least
so the DoS would be reduced to the hard limited memcg.
-- 
Michal Hocko
SUSE Labs