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

[Nicholas Piggin]

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