[PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after

[PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after

[Aneesh Kumar K.V]

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

Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after

[Nicholas Piggin]

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

Re: [PATCH] powerpc/book3s64/kuap: SPRN_AMR modification need CSI instructions before and after

[Nicholas Piggin]

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> 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

[Nicholas Piggin]

Excerpts from Michael Ellerman's message of April 20, 2020 5:04 pm:
> Nicholas Piggin <npiggin@gmail.com> 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