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;
User-Agent: Cyrus-JMAP/3.5.0-alpha0-530-gd0c265785f-fm-20210616.002-gd0c26578
Mime-Version: 1.0
Message-Id: <9a943ade-cabe-40e0-992a-13f26ed58f69@www.fastmail.com>
In-Reply-To: <CAG48ez1=RPbsUos6yY1jMVU49U+GqSN8kFPosoFpKJZjFa4yvQ@mail.gmail.com>
References: <20210414055217.543246-1-avagin@gmail.com>
 <20210414055217.543246-3-avagin@gmail.com>
 <CAG48ez3UrzPE8rkucTgCu8ggcTEjx_h3Gj2FES1qM-uv2KD8bQ@mail.gmail.com>
 <YN6wlwFomEJ0LK1Y@gmail.com>
 <CAG48ez1=RPbsUos6yY1jMVU49U+GqSN8kFPosoFpKJZjFa4yvQ@mail.gmail.com>
Date:   Fri, 02 Jul 2021 13:40:26 -0700
From:   "Andy Lutomirski" <luto@kernel.org>
To:     "Jann Horn" <jannh@google.com>, "Andrei Vagin" <avagin@gmail.com>
Cc:     "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        "Linux API" <linux-api@vger.kernel.org>,
        linux-um@lists.infradead.org, criu@openvz.org, avagin@google.com,
        "Andrew Morton" <akpm@linux-foundation.org>,
        "Anton Ivanov" <anton.ivanov@cambridgegreys.com>,
        "Christian Brauner" <christian.brauner@ubuntu.com>,
        "Dmitry Safonov" <0x7f454c46@gmail.com>,
        "Ingo Molnar" <mingo@redhat.com>, "Jeff Dike" <jdike@addtoit.com>,
        "Mike Rapoport" <rppt@linux.ibm.com>,
        "Michael Kerrisk" <mtk.manpages@gmail.com>,
        "Oleg Nesterov" <oleg@redhat.com>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        "Richard Weinberger" <richard@nod.at>,
        "Thomas Gleixner" <tglx@linutronix.de>, linux-mm@kvack.org
Subject: Re: [PATCH 2/4] arch/x86: implement the process_vm_exec syscall
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Fri, Jul 2, 2021, at 4:51 AM, Jann Horn wrote:
> On Fri, Jul 2, 2021 at 8:25 AM Andrei Vagin <avagin@gmail.com> wrote:
> > On Mon, Jun 28, 2021 at 06:13:29PM +0200, Jann Horn wrote:
> > > On Wed, Apr 14, 2021 at 7:59 AM Andrei Vagin <avagin@gmail.com> wr=
ote:
> > > > +static void swap_mm(struct mm_struct *prev_mm, struct mm_struct=
 *target_mm)
> > > > +{
> > > > +       struct task_struct *tsk =3D current;
> > > > +       struct mm_struct *active_mm;
> > > > +
> > > > +       task_lock(tsk);
> > > > +       /* Hold off tlb flush IPIs while switching mm's */
> > > > +       local_irq_disable();
> > > > +
> > > > +       sync_mm_rss(prev_mm);
> > > > +
> > > > +       vmacache_flush(tsk);
> > > > +
> > > > +       active_mm =3D tsk->active_mm;
> > > > +       if (active_mm !=3D target_mm) {
> > > > +               mmgrab(target_mm);
> > > > +               tsk->active_mm =3D target_mm;
> > > > +       }
> > > > +       tsk->mm =3D target_mm;
> > >
> > > I'm pretty sure you're not currently allowed to overwrite the ->mm=

> > > pointer of a userspace thread. For example, zap_threads() assumes =
that
> > > all threads running under a process have the same ->mm. (And if yo=
u're
> > > fiddling with ->mm stuff, you should probably CC linux-mm@.)
> > >
> > > As far as I understand, only kthreads are allowed to do this (as
> > > implemented in kthread_use_mm()).
> >
> > kthread_use_mm() was renamed from use_mm in the v5.8 kernel. Before
> > that, it wasn't used for user processes in the kernel, but it was
> > exported for modules, and we used it without any visible problems. W=
e
> > understood that there could be some issues like zap_threads and it w=
as
> > one of reasons why we decided to introduce this system call.
> >
> > I understand that there are no places in the kernel where we change =
mm
> > of user threads back and forth, but are there any real concerns why =
we
> > should not do that? I agree that zap_threads should be fixed, but it=

> > will the easy one.
>=20
> My point is that if you break a preexisting assumption like this,
> you'll have to go through the kernel and search for places that rely
> on this assumption, and fix them up, which may potentially require
> thinking about what kinds of semantics would actually be appropriate
> there. Like the MCE killing logic (collect_procs_anon() and such). And=

> current_is_single_threaded(), in which the current patch probably
> leads to logic security bugs. And __uprobe_perf_filter(). Before my
> refactoring of the ELF coredump logic in kernel 5.10 (commit
> b2767d97f5ff75 and the ones before it), you'd have also probably
> created memory corruption bugs in races between elf_core_dump() and
> syscalls like mmap()/munmap(). (Note that this is not necessarily an
> exhaustive list.)
>=20

There=E2=80=99s nmi_uaccess_okay(), and its callers assume that, when a =
task is perf tracing itself, that an event on that task with nmi_uaccess=
_okay() means that uaccess will access that task=E2=80=99s memory.

Core dump code probably expects that dumping memory will access the corr=
ect mm.

I cannot fathom why any kind of remote vm access touched FPU state at al=
l.

What PKRU value is supposed to be used when doing mm swap shenanigans?  =
How about PASID?

What happens if one task attempts to issue a KVM ioctl while its mm is s=
wapped?