Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
Excerpts from Michael Ellerman's message of April 20, 2020 5:04 pm: > Nicholas Piggin writes: >> Excerpts from Nicholas Piggin's message of April 20, 2020 10:17 am: >>> Excerpts from Aneesh Kumar K.V's message of April 19, 2020 11:53 pm: As per the ISA, context synchronizing instructions is needed before and after SPRN_AMR update. Use isync before and the CSI after is implied by the rfid that we will use to switch to a new context. >>> >>> Not entirely sure if we need this. This will restore AMR to more >>> permissive, so if it executes ahead of a stray load from this >>> context, it won't make it fault. > > I thought we'd convinced ourselves it didn't matter in practice due to > the proximity of the entry/exit. I don't remember exactly. We can always drop the isync from the side that pairs with an entry or exit. If we drop it from the other side, what it means in theory is it could float past some of the accesses we're doing in the interrupt context that we thought were protected. So we won't take faults, but it's possible we would let through a user access. I think it's likey that we'd end up executing the mtspr before anything much can take advantage of it, but you never know, and I guess the problem is it becomes impossile to audit and be sure. Thanks, Nick
Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
Nicholas Piggin writes: > Excerpts from Nicholas Piggin's message of April 20, 2020 10:17 am: >> Excerpts from Aneesh Kumar K.V's message of April 19, 2020 11:53 pm: >>> As per the ISA, context synchronizing instructions is needed before and >>> after >>> SPRN_AMR update. Use isync before and the CSI after is implied by the rfid >>> that we will use to switch to a new context. >> >> Not entirely sure if we need this. This will restore AMR to more >> permissive, so if it executes ahead of a stray load from this >> context, it won't make it fault. I thought we'd convinced ourselves it didn't matter in practice due to the proximity of the entry/exit. >> That said, leaving this end open makes it harder to reason about >> user access protection I guess, so let's add it. > > We probably should test whether it needs updating, like the entry > code does. That will be the common case (no update), so yes I agree. cheers
Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
Excerpts from Nicholas Piggin's message of April 20, 2020 10:17 am: > Excerpts from Aneesh Kumar K.V's message of April 19, 2020 11:53 pm: >> As per the ISA, context synchronizing instructions is needed before and after >> SPRN_AMR update. Use isync before and the CSI after is implied by the rfid >> that we will use to switch to a new context. > > Not entirely sure if we need this. This will restore AMR to more > permissive, so if it executes ahead of a stray load from this > context, it won't make it fault. > > That said, leaving this end open makes it harder to reason about > user access protection I guess, so let's add it. We probably should test whether it needs updating, like the entry code does. Thanks, Nick
Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
Excerpts from Aneesh Kumar K.V's message of April 19, 2020 11:53 pm:
> As per the ISA, context synchronizing instructions is needed before and after
> SPRN_AMR update. Use isync before and the CSI after is implied by the rfid
> that we will use to switch to a new context.
Not entirely sure if we need this. This will restore AMR to more
permissive, so if it executes ahead of a stray load from this
context, it won't make it fault.
That said, leaving this end open makes it harder to reason about
user access protection I guess, so let's add it.
Thanks,
Nick
>
> Signed-off-by: Aneesh Kumar K.V
> ---
> arch/powerpc/include/asm/book3s/64/kup-radix.h | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h
> b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> index 3bcef989a35d..224658efe2fd 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> @@ -16,6 +16,7 @@
> #ifdef CONFIG_PPC_KUAP
> BEGIN_MMU_FTR_SECTION_NESTED(67)
> ld \gpr, STACK_REGS_KUAP(r1)
> + isync
> mtspr SPRN_AMR, \gpr
> END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
> #endif
> @@ -62,8 +63,14 @@
>
> static inline void kuap_restore_amr(struct pt_regs *regs)
> {
> - if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
> + isync();
> mtspr(SPRN_AMR, regs->kuap);
> + /*
> + * No following isync/CSI required because we will be
> + * returning to a different context using rfid
> + */
> + }
> }
>
> static inline void kuap_check_amr(void)
> --
> 2.25.2
>
>
[PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after
As per the ISA, context synchronizing instructions is needed before and after
SPRN_AMR update. Use isync before and the CSI after is implied by the rfid
that we will use to switch to a new context.
Signed-off-by: Aneesh Kumar K.V
---
arch/powerpc/include/asm/book3s/64/kup-radix.h | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h
b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 3bcef989a35d..224658efe2fd 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -16,6 +16,7 @@
#ifdef CONFIG_PPC_KUAP
BEGIN_MMU_FTR_SECTION_NESTED(67)
ld \gpr, STACK_REGS_KUAP(r1)
+ isync
mtspr SPRN_AMR, \gpr
END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
#endif
@@ -62,8 +63,14 @@
static inline void kuap_restore_amr(struct pt_regs *regs)
{
- if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
+ isync();
mtspr(SPRN_AMR, regs->kuap);
+ /*
+* No following isync/CSI required because we will be
+* returning to a different context using rfid
+*/
+ }
}
static inline void kuap_check_amr(void)
--
2.25.2
