On Thu, Feb 04, 2021 at 10:31:59PM +0530, Aneesh Kumar K.V wrote:
> On 2/4/21 10:23 PM, Jens Axboe wrote:
> > On 2/1/21 11:30 PM, Aneesh Kumar K.V wrote:
> > > On 2/2/21 11:50 AM, Christophe Leroy wrote:
> > > >
> > > >
> > > > Le 02/02/2021 à 07:16, Aneesh Kumar K.V a écrit :
> > > > > On 2/2/21 11:32 AM, Christophe Leroy wrote:
> > > > > >
> > > > > >
> > > > > > Le 02/02/2021 à 06:55, Aneesh Kumar K.V a écrit :
> > > > > > > Aneesh Kumar K.V <aneesh.ku...@linux.ibm.com> writes:
> > > > > > >
> > > > > > > > Nicholas Piggin <npig...@gmail.com> writes:
> > > > > > > >
> > > > > > > > > Excerpts from Michael Ellerman's message of January 30, 2021
> > > > > > > > > 9:22 pm:
> > > > > > > > > > Christophe Leroy <christophe.le...@csgroup.eu> writes:
> > > > > > > > > > > +Aneesh
> > > > > > > > > > >
> > > > > > > > > > > Le 29/01/2021 à 07:52, Zorro Lang a écrit :
> > > > > > > > > > ..
> > > > > > > > > > > > [ 96.200296] ------------[ cut here ]------------
> > > > > > > > > > > > [ 96.200304] Bug: Read fault blocked by KUAP!
> > > > > > > > > > > > [ 96.200309] WARNING: CPU: 3 PID: 1876 at
> > > > > > > > > > > > arch/powerpc/mm/fault.c:229 bad_kernel_fault+0x180/0x310
> > > > > > > > > > >
> > > > > > > > > > > > [ 96.200734] NIP [c000000000849424]
> > > > > > > > > > > > fault_in_pages_readable+0x104/0x350
> > > > > > > > > > > > [ 96.200741] LR [c00000000084952c]
> > > > > > > > > > > > fault_in_pages_readable+0x20c/0x350
> > > > > > > > > > > > [ 96.200747] --- interrupt: 300
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Problem happens in a section where userspace access is
> > > > > > > > > > > supposed
> > > > > > > > > > > to be granted, so the patch you
> > > > > > > > > > > proposed is definitely not the right fix.
> > > > > > > > > > >
> > > > > > > > > > > c000000000849408: 2c 01 00 4c isync
> > > > > > > > > > > c00000000084940c: a6 03 3d 7d mtspr 29,r9 <==
> > > > > > > > > > > granting
> > > > > > > > > > > userspace access permission
> > > > > > > > > > > c000000000849410: 2c 01 00 4c isync
> > > > > > > > > > > c000000000849414: 00 00 36 e9 ld r9,0(r22)
> > > > > > > > > > > c000000000849418: 20 00 29 81 lwz r9,32(r9)
> > > > > > > > > > > c00000000084941c: 00 02 29 71 andi. r9,r9,512
> > > > > > > > > > > c000000000849420: 78 d3 5e 7f mr r30,r26
> > > > > > > > > > > ==> c000000000849424: 00 00 bf 8b lbz
> > > > > > > > > > > r29,0(r31) <==
> > > > > > > > > > > accessing userspace
> > > > > > > > > > > c000000000849428: 10 00 82 41 beq
> > > > > > > > > > > c000000000849438
> > > > > > > > > > > <fault_in_pages_readable+0x118>
> > > > > > > > > > > c00000000084942c: 2c 01 00 4c isync
> > > > > > > > > > > c000000000849430: a6 03 bd 7e mtspr 29,r21 <==
> > > > > > > > > > > clearing userspace access permission
> > > > > > > > > > > c000000000849434: 2c 01 00 4c isync
> > > > > > > > > > >
> > > > > > > > > > > My first guess is that the problem is linked to the
> > > > > > > > > > > following
> > > > > > > > > > > function, see the comment
> > > > > > > > > > >
> > > > > > > > > > > /*
> > > > > > > > > > > * For kernel thread that doesn't have thread.regs
> > > > > > > > > > > return
> > > > > > > > > > > * default AMR/IAMR values.
> > > > > > > > > > > */
> > > > > > > > > > > static inline u64 current_thread_amr(void)
> > > > > > > > > > > {
> > > > > > > > > > > if (current->thread.regs)
> > > > > > > > > > > return current->thread.regs->amr;
> > > > > > > > > > > return AMR_KUAP_BLOCKED;
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > Above function was introduced by commit 48a8ab4eeb82
> > > > > > > > > > > ("powerpc/book3s64/pkeys: Don't update SPRN_AMR
> > > > > > > > > > > when in kernel mode")
> > > > > > > > > >
> > > > > > > > > > Yeah that's a bit of a curly one.
> > > > > > > > > >
> > > > > > > > > > At some point io_uring did kthread_use_mm(), which is
> > > > > > > > > > supposed to
> > > > > > > > > > mean
> > > > > > > > > > the kthread can operate on behalf of the original process
> > > > > > > > > > that
> > > > > > > > > > submitted
> > > > > > > > > > the IO.
> > > > > > > > > >
> > > > > > > > > > But because KUAP is implemented using memory protection
> > > > > > > > > > keys, it
> > > > > > > > > > depends
> > > > > > > > > > on the value of the AMR register, which is not part of the
> > > > > > > > > > mm,
> > > > > > > > > > it's in
> > > > > > > > > > thread.regs->amr.
> > > > > > > > > >
> > > > > > > > > > And what's worse by the time we're in kthread_use_mm() we no
> > > > > > > > > > longer have
> > > > > > > > > > access to the thread.regs->amr of the original process that
> > > > > > > > > > submitted
> > > > > > > > > > the IO.
> > > > > > > > > >
> > > > > > > > > > We also can't simply move the AMR into the mm, precisely
> > > > > > > > > > because
> > > > > > > > > > it's
> > > > > > > > > > per thread, not per mm.
> > > > > > > > > >
> > > > > > > > > > So TBH I don't know how we're going to fix this.
> > > > > > > > > >
> > > > > > > > > > I guess we could return AMR=unblocked for kernel threads,
> > > > > > > > > > but that's
> > > > > > > > > > arguably a bug because it allows a process to circumvent
> > > > > > > > > > memory
> > > > > > > > > > keys by
> > > > > > > > > > asking the kernel to do the access.
> > > > > > > > >
> > > > > > > > > We shouldn't need to inherit AMR should we? We only need it
> > > > > > > > > to be
> > > > > > > > > locked
> > > > > > > > > for kernel threads until it's explicitly unlocked -- nothing
> > > > > > > > > mm
> > > > > > > > > specific
> > > > > > > > > there. I think current_thread_amr could return 0 for kernel
> > > > > > > > > threads? Or
> > > > > > > > > I would even avoid using that function for allow_user_access
> > > > > > > > > and open
> > > > > > > > > code the kthread case and remove it from current_thread_amr().
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Nick
> > > > > > > >
> > > > > > >
> > > > > > > updated one
> > > > > > >
> > > > > > > From 8fdb0680f983940d61f91da8252b13c8d3e8ebee Mon Sep 17
> > > > > > > 00:00:00 2001
> > > > > > > From: "Aneesh Kumar K.V" <aneesh.ku...@linux.ibm.com>
> > > > > > > Date: Tue, 2 Feb 2021 09:23:38 +0530
> > > > > > > Subject: [PATCH v2] powerpc/kuap: Allow kernel thread to access
> > > > > > > userspace
> > > > > > > after kthread_use_mm
> > > > > > >
> > > > > > > This fix the bad fault reported by KUAP when io_wqe_worker access
> > > > > > > userspace.
> > > > > > >
> > > > > > > Bug: Read fault blocked by KUAP!
> > > > > > > WARNING: CPU: 1 PID: 101841 at arch/powerpc/mm/fault.c:229
> > > > > > > __do_page_fault+0x6b4/0xcd0
> > > > > > > NIP [c00000000009e7e4] __do_page_fault+0x6b4/0xcd0
> > > > > > > LR [c00000000009e7e0] __do_page_fault+0x6b0/0xcd0
> > > > > > > ..........
> > > > > > > Call Trace:
> > > > > > > [c000000016367330] [c00000000009e7e0]
> > > > > > > __do_page_fault+0x6b0/0xcd0
> > > > > > > (unreliable)
> > > > > > > [c0000000163673e0] [c00000000009ee3c] do_page_fault+0x3c/0x120
> > > > > > > [c000000016367430] [c00000000000c848]
> > > > > > > handle_page_fault+0x10/0x2c
> > > > > > > --- interrupt: 300 at iov_iter_fault_in_readable+0x148/0x6f0
> > > > > > > ..........
> > > > > > > NIP [c0000000008e8228] iov_iter_fault_in_readable+0x148/0x6f0
> > > > > > > LR [c0000000008e834c] iov_iter_fault_in_readable+0x26c/0x6f0
> > > > > > > interrupt: 300
> > > > > > > [c0000000163677e0] [c0000000007154a0]
> > > > > > > iomap_write_actor+0xc0/0x280
> > > > > > > [c000000016367880] [c00000000070fc94] iomap_apply+0x1c4/0x780
> > > > > > > [c000000016367990] [c000000000710330]
> > > > > > > iomap_file_buffered_write+0xa0/0x120
> > > > > > > [c0000000163679e0] [c00800000040791c]
> > > > > > > xfs_file_buffered_aio_write+0x314/0x5e0 [xfs]
> > > > > > > [c000000016367a90] [c0000000006d74bc] io_write+0x10c/0x460
> > > > > > > [c000000016367bb0] [c0000000006d80e4] io_issue_sqe+0x8d4/0x1200
> > > > > > > [c000000016367c70] [c0000000006d8ad0]
> > > > > > > io_wq_submit_work+0xc0/0x250
> > > > > > > [c000000016367cb0] [c0000000006e2578]
> > > > > > > io_worker_handle_work+0x498/0x800
> > > > > > > [c000000016367d40] [c0000000006e2cdc] io_wqe_worker+0x3fc/0x4f0
> > > > > > > [c000000016367da0] [c0000000001cb0a4] kthread+0x1c4/0x1d0
> > > > > > > [c000000016367e10] [c00000000000dbf0]
> > > > > > > ret_from_kernel_thread+0x5c/0x6c
> > > > > > >
> > > > > > > The kernel consider thread AMR value for kernel thread to be
> > > > > > > AMR_KUAP_BLOCKED. Hence access to userspace is denied. This
> > > > > > > of course not correct and we should allow userspace access after
> > > > > > > kthread_use_mm(). To be precise, kthread_use_mm() should inherit
> > > > > > > the
> > > > > > > AMR value of the operating address space. But, the AMR value is
> > > > > > > thread-specific and we inherit the address space and not thread
> > > > > > > access restrictions. Because of this ignore AMR value when
> > > > > > > accessing
> > > > > > > userspace via kernel thread.
> > > > > > >
> > > > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.ku...@linux.ibm.com>
> > > > > > > ---
> > > > > > > Changes from v1:
> > > > > > > * Address review feedback from Nick
> > > > > > >
> > > > > > > arch/powerpc/include/asm/book3s/64/kup.h | 8 +++++++-
> > > > > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/arch/powerpc/include/asm/book3s/64/kup.h
> > > > > > > b/arch/powerpc/include/asm/book3s/64/kup.h
> > > > > > > index f50f72e535aa..95f4df99249e 100644
> > > > > > > --- a/arch/powerpc/include/asm/book3s/64/kup.h
> > > > > > > +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> > > > > > > @@ -384,7 +384,13 @@ static __always_inline void
> > > > > > > allow_user_access(void __user *to, const void __user
> > > > > > > // This is written so we can resolve to a single case at
> > > > > > > build
> > > > > > > time
> > > > > > > BUILD_BUG_ON(!__builtin_constant_p(dir));
> > > > > > > - if (mmu_has_feature(MMU_FTR_PKEY))
> > > > > > > + /*
> > > > > > > + * if it is a kthread that did kthread_use_mm() don't
> > > > > > > + * use current_thread_amr().
> > > > > >
> > > > > > According to include/linux/sched.h, PF_KTHREAD means /* I am a
> > > > > > kernel
> > > > > > thread */
> > > > > > It doesn't seem to be related to kthread_use_mm()
> > > > >
> > > > > That should be a sufficient check here. if we did reach here without
> > > > > calling kthread_user_mm, we will crash on access because we don't have
> > > > > a mm attached to the current process. a kernel thread with
> > > > > kthread_use_mm has
> > > >
> > > > Ok but then the comment doesn't match the check.
> > >
> > >
> > > I was trying to be explict in the comment that we expect the thread to
> > > have done kthread_use_mm().
> > >
> > > >
> > > > And also the comment in current_thread_amr() is then misleading.
> > > >
> > > > Why not do the current->flags & PF_KTHREAD check in current_thread_amr()
> > > > and return 0 in that case instead of BLOCKED ?
> > >
> > > In my view currrent_thread_amr() is more generic and we want to be
> > > explicit there that a kernel thread AMR is KUAP_BLOCKED. Only when we
> > > call allow user access, we relax the AMR value.
> >
> > Just following up on this, as I'd hate to have 5.11 released with this
> > bug in it for powerpc. It'll obviously also affect other cases of a
> > kernel thread faulting after having done kthread_use_mm(), though I'm
> > not sure how widespread that is. In any case, it'll leave io_uring
> > mostly broken on powerpc if this isn't patched for release.
> >
>
> I am waiting for test feedback on the change I posted earlier. I am also
> running a regression run myself. Once that is complete i will post the patch
> as a separate email.
Are you waiting a test from me? Or someone else who test PPC? Although I'm
the "Reported-by" of this bug, I just can help to verify this bug itself,
I don't have enough test cases to do regression test from PPC side. Do you need
me to verify this bug itself.
Thanks,
Zorro
>
> -aneesh
>
