Re: [PATCH] powerpc/fault: fix wrong KUAP fault for IO_URING

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

Nicholas Piggin <npiggin@gmail.com> writes:

> Excerpts from Michael Ellerman's message of January 30, 2021 9:22 pm:
>> Christophe Leroy <christophe.leroy@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

Can we try this?

commit 9a193d38b1a0a52364bc70ec953e0685993603b6
Author: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Date:   Tue Feb 2 09:23:38 2021 +0530

    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.kumar@linux.ibm.com>

diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index f50f72e535aa..7457d80ba0bb 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().
+	 */
+	if (!current->mm && current->active_mm != &init_mm)
+		thread_amr = 0;
+	else if (mmu_has_feature(MMU_FTR_PKEY))
 		thread_amr = current_thread_amr();
 
 	if (dir == KUAP_READ)