Re: [PATCH] powerpc: Do not restore KUAP in arch_exit_to_user_mode_prepare()
Venkat Rao Bagalkote <[email protected]>
| Newsgroups | gmane.linux.ports.ppc.embedded |
|---|---|
| Message-ID | <[email protected]> |
On 30/08/26 8:24 pm, Ritesh Harjani (IBM) wrote:
> KUAP means kernel cannot touch user memory unless it explicitly is
> enabled. In the kernel it should stay AMR_KUAP_BLOCKED. While returning
> to userspace just before RFI, kernel should restore the user AMR value
> back.
>
> Looks like GENERIC_ENTRY might be treating arch_exit_to_user_mode_prepare()
> as the last architecture step before returning to userspace.
> commit bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature")
> therefore called kuap_user_restore() from that hook. But on PowerPC that
> is too early. After irqentry_exit() / syscall_exit_to_user_mode() we
> still run platform specific exit routines.
>
> e.g. code snippets showing both exception handling and system call
> handling as the callers of function arch_exit_to_user_mode_prepare()
> which does kuap_user_restore(). The below path shows that calling
> kuap_user_restore() is too early when called from
> arch_exit_to_user_mode_prepare().
>
> Exception handling in exceptions-64s.S
> =======================================
>
> bl CFUNC(do_page_fault)
> ..DEFINE_INTERRUPT_HANDLER_ASYNC(do_page_fault)
> arch_interrupt_async_enter_prepare(regs);
> state = irqentry_enter(regs);
> instrumentation_begin();
> irq_enter_rcu();
> handler(regs);
> nap_adjust_return(regs);
> irq_exit_rcu();
> instrumentation_end();
> arch_interrupt_async_exit_prepare(regs);
> irqentry_exit(regs, state); <<< too early
> irqentry_exit_to_user_mode()
> __exit_to_user_mode_prepare(regs, EXIT_TO_USER_MODE_WORK_IRQ);
> arch_exit_to_user_mode_prepare(regs, ti_work); <<< too early
> b interrupt_return_srr
> .. bl CFUNC(interrupt_exit_user_prepare) <<< already calls kuap_user_restore
>
> prep_irq_for_enabled_exit() retry can run kernel code with IRQs on. So
> only when that routine is fully finished is when the user KUAP should be
> fully restored which interrupt_exit_user_prepare() already takes care of
> before returning.
>
> Similarly for system call handling in interrupt_64.S
> ======================================================
>
> bl CFUNC(system_call_exception)
>
> .Lsyscall_exit:
> addi r4,r1,STACK_INT_FRAME_REGS
> li r5,0 /* !scv */
> bl CFUNC(syscall_exit_prepare)
> .. kuap_assert_locked();
> syscall_exit_to_user_mode(regs); <<< too early
> syscall_exit_to_user_mode_prepare(regs); <<< too early
> kuap_user_restore(regs); <<< already calls
>
> syscall_exit_prepare(), which can enable IRQs, replay a pending
> interrupt, and only then rfi. Those functions already restore KUAP
> immediately before rfi.
>
> Note that if we restore the user AMR too early like in the current code
> as shown from the code snippets above, then we get the following warning
> when CONFIG_PPC_KUAP_DEBUG is enabled:
> WARNING: arch/powerpc/include/asm/book3s/64/kup.h:293 at interrupt_exit_user_prepare+0x1a0/0x1c0
> Hardware name: IBM pSeries (emulated by qemu) POWER10 (architected)
> TRAP: 0700
> LR: c00000000000d8d4 CTR: c0000000021fe500
> MSR: <SF,EE,ME,IR,DR,RI,LE> CR: 44000804 XER: 20040000
> interrupt_exit_user_prepare+0x1a0/0x1c0
> interrupt_return_srr_user+0x8/0x12c
>
> Fixes: bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature")
> Fixes: 02565a782c1ee ("powerpc: Introduce syscall exit arch functions")
> Signed-off-by: Ritesh Harjani (IBM) <[email protected]>
> ---
Issue is fixed with this patch.
Tested-by: Venkat Rao Bagalkote <[email protected]>
Before this Patch:
[ 3.861094] ------------[ cut here ]------------
[ 3.861105] WARNING: ./arch/powerpc/include/asm/book3s/64/kup.h:293
at interrupt_exit_user_prepare+0x240/0x260, CPU#23: modprobe/348
[ 3.861124] Modules linked in:
[ 3.861133] CPU: 23 UID: 0 PID: 348 Comm: modprobe Not tainted 7.2.0+
#14 PREEMPTLAZY
[ 3.861145] Hardware name: IBM,8375-42A POWER9 (architected) 0x4e0202
0xf000005 of:IBM,FW950.80 (VL950_131) hv:phyp pSeries
[ 3.861154] NIP: c00000000003a730 LR: c00000000003a53c CTR:
000000000049f42c
[ 3.861162] REGS: c0000000a7967b70 TRAP: 0700 Not tainted (7.2.0+)
[ 3.861170] MSR: 8000000000029033 <SF,EE,ME,IR,DR,RI,LE> CR:
44000008 XER: 20040000
[ 3.861197] CFAR: c00000000003a56c IRQMASK: 1
GPR00: c00000000003a53c c0000000a7967e10
c000000002b7a400 c0000000a7967fb8
GPR04: 800000004000d033 0000000000000001
c0000000000a7460 ffffffffffffffbf
GPR08: 013ffffe00040038 fcffffffffffffff
3cffffffffffffff 0000000000000000
GPR12: c0000000a7967688 c000000017fe3700
0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR28: 0000000000000000 0000000000000000
0000000000000000 c0000000a7967e80
[ 3.861313] NIP [c00000000003a730]
interrupt_exit_user_prepare+0x240/0x260
[ 3.861325] LR [c00000000003a53c] interrupt_exit_user_prepare+0x4c/0x260
[ 3.861336] Call Trace:
[ 3.861340] [c0000000a7967e10] [c00000000003a53c]
interrupt_exit_user_prepare+0x4c/0x260 (unreliable)
[ 3.861356] [c0000000a7967e50] [c00000000000d8d4]
interrupt_return_srr_user+0x8/0x12c
[ 3.861370] ---- interrupt: 400 at 0x7fff8cf341c0
[ 3.861379] NIP: 00007fff8cf341c0 LR: 0000000000000000 CTR:
0000000000000000
[ 3.861388] REGS: c0000000a7967e80 TRAP: 0400 Not tainted (7.2.0+)
[ 3.861395] MSR: 800000004000d033 <SF,EE,PR,ME,IR,DR,RI,LE> CR:
00000000 XER: 00000000
[ 3.861424] CFAR: c00000000000d7bc IRQMASK: 0
GPR00: 0000000000000000 00007fffc7b04020
0000000000000000 0000000000000000
GPR04: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR08: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR12: 00007fff8cf341c0 0000000000000000
0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
GPR28: 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[ 3.861536] NIP [00007fff8cf341c0] 0x7fff8cf341c0
[ 3.861543] LR [0000000000000000] 0x0
[ 3.861550] ---- interrupt: 400
[ 3.861555] Code: 7fbd0aa6 387f0178 488befad 60000000 e93f0178
7c3d4800 4082febc 73de00ff 4182fef8 3ba00000 4bfffeb4 60000000
<0fe00000> 4bfffe3c 60000000 60000000
[ 3.861599] ---[ end trace 0000000000000000 ]---
Regards,
Venkat.
> arch/powerpc/include/asm/entry-common.h | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/entry-common.h b/arch/powerpc/include/asm/entry-common.h
> index c5adb5006361..94083516df57 100644
> --- a/arch/powerpc/include/asm/entry-common.h
> +++ b/arch/powerpc/include/asm/entry-common.h
> @@ -515,8 +515,14 @@ static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> local_paca->tm_scratch = regs->msr;
> #endif
> - /* Restore user access locks last */
> - kuap_user_restore(regs);
> + /*
> + * Do not restore KUAP here. Generic entry might treat this as the last
> + * arch step before userspace but PowerPC still has kernel work after
> + * irqentry_exit()/syscall_exit_to_user_mode() i.e. in
> + * interrupt_exit_user_prepare() / syscall_exit_prepare() may enable
> + * IRQs and retry. Those functions restore KUAP immediately before rfi,
> + * which is where it should belong.
> + */
> }
>
> #define arch_exit_to_user_mode_prepare arch_exit_to_user_mode_prepare
> --
> 2.39.5
>