[PATCH v4 24/41] powerpc/book3s64/pkeys: Store/restore userspace AMR correctly on entry and exit from kernel
Nicholas Piggin
npiggin at gmail.com
Fri Jul 3 19:18:42 AEST 2020
Excerpts from Aneesh Kumar K.V's message of June 15, 2020 4:14 pm:
> This prepare kernel to operate with a different value than userspace AMR.
> For this, AMR needs to be saved and restored on entry and return from the
> kernel.
>
> With KUAP we modify kernel AMR when accessing user address from the kernel
> via copy_to/from_user interfaces.
>
> If MMU_FTR_KEY is enabled we always use the key mechanism to implement KUAP
> feature. If MMU_FTR_KEY is not supported and if we support MMU_FTR_KUAP
> (radix translation on POWER9), we can skip restoring AMR on return
> to userspace. Userspace won't be using AMR in that specific config.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar at linux.ibm.com>
> ---
> arch/powerpc/include/asm/book3s/64/kup.h | 141 ++++++++++++++++++-----
> arch/powerpc/kernel/entry_64.S | 6 +-
> arch/powerpc/kernel/exceptions-64s.S | 4 +-
> arch/powerpc/kernel/syscall_64.c | 26 ++++-
> 4 files changed, 144 insertions(+), 33 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index e6ee1c34842f..6979cd1a0003 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -13,18 +13,47 @@
>
> #ifdef __ASSEMBLY__
>
> -.macro kuap_restore_amr gpr1, gpr2
> -#ifdef CONFIG_PPC_KUAP
> +.macro kuap_restore_user_amr gpr1
> +#if defined(CONFIG_PPC_MEM_KEYS)
> BEGIN_MMU_FTR_SECTION_NESTED(67)
> - mfspr \gpr1, SPRN_AMR
> + /*
> + * AMR is going to be different when
> + * returning to userspace.
> + */
> + ld \gpr1, STACK_REGS_KUAP(r1)
> + isync
> + mtspr SPRN_AMR, \gpr1
> +
> + /* No isync required, see kuap_restore_user_amr() */
> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_PKEY , 67)
> +#endif
> +.endm
> +
> +.macro kuap_restore_kernel_amr gpr1, gpr2
> +#if defined(CONFIG_PPC_MEM_KEYS)
> + BEGIN_MMU_FTR_SECTION_NESTED(67)
> + b 99f // handle_pkey_restore_amr
> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_PKEY , 67)
> +
> + BEGIN_MMU_FTR_SECTION_NESTED(68)
> + b 99f // handle_kuap_restore_amr
> + MMU_FTR_SECTION_ELSE_NESTED(68)
> + b 100f // skip_restore_amr
> + ALT_MMU_FTR_SECTION_END_NESTED_IFSET(MMU_FTR_KUAP, 68)
> +
> +99:
> + /*
> + * AMR is going to be mostly the same since we are
> + * returning to the kernel. Compare and do a mtspr.
> + */
> ld \gpr2, STACK_REGS_KUAP(r1)
> + mfspr \gpr1, SPRN_AMR
> cmpd \gpr1, \gpr2
> - beq 998f
> + beq 100f
> isync
> mtspr SPRN_AMR, \gpr2
> /* No isync required, see kuap_restore_amr() */
> -998:
> - END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_KUAP, 67)
> +100: // skip_restore_amr
Can't you code it like this? (_IFCLR requires none of the bits to be
set)
BEGIN_MMU_FTR_SECTION_NESTED(67)
b 99f // nothing using AMR, no need to restore
END_MMU_FTR_SECTION_NESTED_IFCLR(MMU_FTR_PKEY | MMU_FTR_KUAP, 67)
That saves you a branch in the common case of using AMR. Similar
for others.
> @@ -69,22 +133,40 @@
>
> extern u64 default_uamor;
>
> -static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
> +static inline void kuap_restore_user_amr(struct pt_regs *regs)
> {
> - if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
> - isync();
> - mtspr(SPRN_AMR, regs->kuap);
> - /*
> - * No isync required here because we are about to RFI back to
> - * previous context before any user accesses would be made,
> - * which is a CSI.
> - */
> + if (!mmu_has_feature(MMU_FTR_PKEY))
> + return;
If you have PKEY but not KUAP, do you still have to restore?
> +
> + isync();
> + mtspr(SPRN_AMR, regs->kuap);
> + /*
> + * No isync required here because we are about to rfi
> + * back to previous context before any user accesses
> + * would be made, which is a CSI.
> + */
> +}
> +
> +static inline void kuap_restore_kernel_amr(struct pt_regs *regs,
> + unsigned long amr)
> +{
> + if (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)) {
> +
> + if (unlikely(regs->kuap != amr)) {
> + isync();
> + mtspr(SPRN_AMR, regs->kuap);
> + /*
> + * No isync required here because we are about to rfi
> + * back to previous context before any user accesses
> + * would be made, which is a CSI.
> + */
> + }
> }
> }
>
> static inline unsigned long kuap_get_and_check_amr(void)
> {
> - if (mmu_has_feature(MMU_FTR_KUAP)) {
> + if (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)) {
> unsigned long amr = mfspr(SPRN_AMR);
> if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG)) /* kuap_check_amr() */
> WARN_ON_ONCE(amr != AMR_KUAP_BLOCKED);
We could do a static key that's based on this condition, but that can
wait for another day.
>
> static inline void kuap_check_amr(void)
> {
> - if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && mmu_has_feature(MMU_FTR_KUAP))
> + if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) &&
> + (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)))
> WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
> }
>
> #else /* CONFIG_PPC_MEM_KEYS */
>
> -static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
> +static inline void kuap_restore_user_amr(struct pt_regs *regs)
> +{
> +}
> +
> +static inline void kuap_restore_kernel_amr(struct pt_regs *regs, unsigned long amr)
> {
> }
>
> @@ -113,6 +200,7 @@ static inline unsigned long kuap_get_and_check_amr(void)
> {
> return 0;
> }
> +
> #endif /* CONFIG_PPC_MEM_KEYS */
>
>
> @@ -187,7 +275,6 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
> "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
> }
> #endif /* CONFIG_PPC_KUAP */
> -
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_BOOK3S_64_KUP_H */
Unnecessary hunks.
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 9d49338e0c85..a087cbe0b17d 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -481,8 +481,8 @@ _ASM_NOKPROBE_SYMBOL(fast_interrupt_return)
> kuap_check_amr r3, r4
> ld r5,_MSR(r1)
> andi. r0,r5,MSR_PR
> - bne .Lfast_user_interrupt_return
> - kuap_restore_amr r3, r4
> + bne .Lfast_user_interrupt_return_amr
> + kuap_restore_kernel_amr r3, r4
> andi. r0,r5,MSR_RI
> li r3,0 /* 0 return value, no EMULATE_STACK_STORE */
> bne+ .Lfast_kernel_interrupt_return
> @@ -502,6 +502,8 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return)
> cmpdi r3,0
> bne- .Lrestore_nvgprs
>
> +.Lfast_user_interrupt_return_amr:
> + kuap_restore_user_amr r3
> .Lfast_user_interrupt_return:
> ld r11,_NIP(r1)
> ld r12,_MSR(r1)
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index e70ebb5c318c..8226af444d77 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -971,7 +971,7 @@ EXC_COMMON_BEGIN(system_reset_common)
> ld r10,SOFTE(r1)
> stb r10,PACAIRQSOFTMASK(r13)
>
> - kuap_restore_amr r9, r10
> + kuap_restore_kernel_amr r9, r10
> EXCEPTION_RESTORE_REGS
> RFI_TO_USER_OR_KERNEL
>
> @@ -2784,7 +2784,7 @@ EXC_COMMON_BEGIN(soft_nmi_common)
> ld r10,SOFTE(r1)
> stb r10,PACAIRQSOFTMASK(r13)
>
> - kuap_restore_amr r9, r10
> + kuap_restore_kernel_amr r9, r10
> EXCEPTION_RESTORE_REGS hsrr=0
> RFI_TO_KERNEL
>
> diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
> index 7e560a01afa4..fded67982fbe 100644
> --- a/arch/powerpc/kernel/syscall_64.c
> +++ b/arch/powerpc/kernel/syscall_64.c
> @@ -35,7 +35,21 @@ notrace long system_call_exception(long r3, long r4, long r5,
> BUG_ON(!FULL_REGS(regs));
> BUG_ON(regs->softe != IRQS_ENABLED);
>
> - kuap_check_amr();
> +#ifdef CONFIG_PPC_MEM_KEYS
> + if (mmu_has_feature(MMU_FTR_PKEY)) {
> + unsigned long amr;
> + /*
> + * When entering from userspace we mostly have the AMR
> + * different from kernel default values. Hence don't compare.
> + */
> + amr = mfspr(SPRN_AMR);
> + regs->kuap = amr;
> + if (mmu_has_feature(MMU_FTR_KUAP))
> + mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
> + isync();
isync should be inside the if(). Again do pkeys need to save this if
KUAP is not being used? I haven't really looked at how all that works,
but what's changing for the PKEY && !KUAP case?
This would be nice if it could all go into a wrapper function rather
than ifdef.
> + } else
> +#endif
> + kuap_check_amr();
>
> account_cpu_user_entry();
>
> @@ -222,6 +236,10 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>
> account_cpu_user_exit();
>
> + /*
> + * We do this at the end so that we do context switch with KERNEL AMR
> + */
> + kuap_restore_user_amr(regs);
> return ret;
Comment doesn't make sense, newline required before return.
> }
>
> @@ -306,6 +324,10 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>
> account_cpu_user_exit();
>
> + /*
> + * We do this at the end so that we do context switch with KERNEL AMR
> + */
> + kuap_restore_user_amr(regs);
Duplicated comments I prefer to just have like this instead of trying to
keep them in sync. Can complete the circular reference by having a
* similarly in interrupt_exit_user_prepare
in the main comment, but if they come close to one another in the same
file it's not so important to keep them together.
+ kuap_restore_user_amr(regs); /* see syscall_exit_prepare */
> return ret;
> }
>
> @@ -376,7 +398,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
> * which would cause Read-After-Write stalls. Hence, we take the AMR
> * value from the check above.
> */
> - kuap_restore_amr(regs, amr);
> + kuap_restore_kernel_amr(regs, amr);
>
> return ret;
> }
> --
> 2.26.2
>
>
More information about the Linuxppc-dev
mailing list