Date:   Mon, 31 Jan 2022 11:38:49 -0600
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Denys Vlasenko <vda.linux@...glemail.com>,
        Kees Cook <keescook@...omium.org>,
        Jann Horn <jannh@...gle.com>, Vlastimil Babka <vbabka@...e.cz>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>
Subject: Re: [PATCH] binfmt_elf: Take the mmap lock when walking the VMA list

Matthew Wilcox <willy@...radead.org> writes:

> On Mon, Jan 31, 2022 at 10:26:11AM -0600, Eric W. Biederman wrote:
>> Matthew Wilcox <willy@...radead.org> writes:
>> 
>> > On Mon, Jan 31, 2022 at 10:03:31AM -0600, Eric W. Biederman wrote:
>> >> "Matthew Wilcox (Oracle)" <willy@...radead.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?

The thread that called vfork is suspended.  The other threads can
continue to execute.

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

Sometimes.  From a coredump perspective it is just another process
that happens to share the mm.  Like the vfork process.

For a while the coredump code was trying to kill and possibly dump all
of these ``threads'' that shared a vm.  The practical problem was that
a failing exec after vfork could trigger a coredump that would kill
it's parent process.

So when I look at these from a coredump or signal perspective I just
treat them as weird processes that happen to share an mm_struct.

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

I don't know how much VMA manipulation makes sense but it is legitimate
to attach to an mm and do those things as Jann pointed out.

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

Working on it now.

Eric

Powered by blists - more mailing lists