Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 Jan 2022 16:35:12 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Denys Vlasenko <vda.linux@googlemail.com>,
        Kees Cook <keescook@chromium.org>,
        Jann Horn <jannh@google.com>, Vlastimil Babka <vbabka@suse.cz>,
        "Liam R . Howlett" <Liam.Howlett@oracle.com>
Subject: Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list
Message-ID: <YfgPwPvopO1aqcVC@casper.infradead.org>
References: <20220131153740.2396974-1-willy@infradead.org>
 <871r0nriy4.fsf@email.froward.int.ebiederm.org>
 <YfgKw5z2uswzMVRQ@casper.infradead.org>
 <877dafq3bw.fsf@email.froward.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <877dafq3bw.fsf@email.froward.int.ebiederm.org>
Precedence: bulk

On Mon, Jan 31, 2022 at 10:26:11AM -0600, Eric W. Biederman wrote:
> Matthew Wilcox <willy@infradead.org> writes:
> 
> > On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
> >> "Matthew Wilcox (Oracle)" <willy@infradead.org> writes:
> >> 
> >> > I'm not sure if the VMA list can change under us, but dump_vma_snapshot()
> >> > is very careful to take the mmap_lock in write mode.  We only need to
> >> > take it in read mode here as we do not care if the size of the stack
> >> > VMA changes underneath us.
> >> >
> >> > If it can be changed underneath us, this is a potential use-after-free
> >> > for a multithreaded process which is dumping core.
> >> 
> >> The problem is not multi-threaded process so much as processes that
> >> share their mm.
> >
> > I don't understand the difference.  I appreciate that another process can
> > get read access to an mm through, eg, /proc, but how can another process
> > (that isn't a thread of this process) modify the VMAs?
> 
> There are a couple of ways.
> 
> A classic way is a multi-threads process can call vfork, and the
> mm_struct is shared with the child until exec is called.

While true, I thought the semantics of vfork() were that the parent
was suspended.  Given that, it can't core dump until the child execs
... right?

> A process can do this more deliberately by forking a child using
> clone(CLONE_VM) and not including CLONE_THREAD.   Supporting this case
> is a hold over from before CLONE_THREAD was supported in the kernel and
> such processes were used to simulate threads.

That is a multithreaded process then!  Maybe not in the strict POSIX
compliance sense, but the intent is to be a multithreaded process.
ie multiple threads of execution, sharing an address space.

> It also happens that there are subsystems in the kernel that do things
> like kthread_use_mm that can also be modifying the mm during a coredump.

Yikes.  That's terrifying.  It's really legitimate for a kthread to
attach to a process and start tearing down VMAs?

> > Uhh .. that seems like it needs a lot more understanding of binfmt_elf
> > than I currently possess.  I'd rather spend my time working on folios
> > than learning much more about binfmt_elf.  I was just trying to fix an
> > assertion failure with the maple tree patches (we now assert that you're
> > holding a lock when walking the list of VMAs).
> 
> Fair enough.  I will put it on my list of things to address.

Thanks.  Now that I've disclosed it's a UAF, I hope you're able to
get to it soon.  Otherwise we should put this band-aid in for now
and you can address it properly in the fullness of time.