Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Jann Horn <jannh@google.com>
Cc:     Andrei Vagin <avagin@gmail.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] time/namespace: Forbid timens page faults under
 kthread_use_mm()
In-Reply-To: <CAG48ez3UBb3Aq7+AFSmRj5a9czmew5b0PEdhWQ9qvQdeejnJZg@mail.gmail.com>
References: <20221129191839.2471308-1-jannh@google.com>
 <20221129191839.2471308-2-jannh@google.com> <87fse1v4rf.ffs@tglx>
 <CAG48ez3UBb3Aq7+AFSmRj5a9czmew5b0PEdhWQ9qvQdeejnJZg@mail.gmail.com>
Date:   Wed, 30 Nov 2022 01:07:37 +0100
Message-ID: <87y1rttid2.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Tue, Nov 29 2022 at 23:28, Jann Horn wrote:
> On Tue, Nov 29, 2022 at 10:18 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> But the way more interesting question is:
>>
>> >  struct page *find_timens_vvar_page(struct vm_area_struct *vma)
>> >  {
>> > +     /*
>> > +      * We can't handle faults where current's timens does not match the
>> > +      * timens associated with the mm_struct. This can happen if a page fault
>> > +      * occurs in a kthread that is using kthread_use_mm().
>> > +      */
>>
>> How does a kthread, which obvioulsy did kthread_use_mm(), end up trying to
>> fault in the time namespace vvar page?
>
> By doing copy_from_user()? That's what kthread_use_mm() is for, right?
> If you look through the users of kthread_use_mm(), most of them use it
> to be able to use the normal usercopy functions. See the users in usb
> gadget code, and the VFIO code, and the AMD GPU code. And if you're
> doing usercopy on userspace addresses, then you can basically always
> hit a vvar page - even if you had somehow checked beforehand what the
> address points to, userspace could have moved a vvar region in that
> spot in the meantime.
>
> That said, I haven't actually tried it. But I don't think there's
> anything in the page fault handling path that distinguishes between
> copy_from_user() faults in kthread context and other userspace faults
> in a relevant way?

True.

>> It neither answers the obvious question why this is a problem of the
>> time namespace vvar page and not a general issue versus a kthread, which
>> borrowed a user mm, ending up in vdso_fault() in the first place?
>
> Is it a problem if a kthread ends up in the other parts of
> vdso_fault() or vvar_fault()? From what I can tell, nothing in there
> except for the timens stuff is going to care whether it's hit from a
> userspace fault or from a kthread.
>
> Though, looking at it again now, I guess the `sym_offset ==
> image->sym_vvar_page` path is also going to misbehave, so I guess we
> could try to instead make the vdso/vvar fault handlers bail out in
> kthread context for all the architectures, since we're only going to
> hit that if userspace is deliberately doing something bad...

Deliberately or stupdily, does not matter. But squashing the problem
right at the entry point is definitely the better than making it a
special case of timens.

>> None of those VDSO (user space) addresses are subject to be faulted in
>> by anything else than the associated user space task(s).
>
> Are you saying that it's not possible or that it doesn't happen when
> userspace is well-behaved?

My subconcious self told me that a kthread won't do that unless it's
buggered which makes the vdso fault path the least of our problems, but
thinking more about it: You are right, that there are ways that the
kthread ends up with a vdso page address.... Bah!

Still my point stands that this is not a timens VDSO issue, but an issue
of: kthread tries to fault in a VDSO page of whatever nature.

Thanks,

        tglx