Subject: Re: [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function

From: Michael Ellerman <[email-masked]>
Date: Tue, 07 Jul 2020 16:05:15 +1000

"Aneesh Kumar K.V" <[email-masked]> writes:
> UAMOR values are not application-specific.

It used to be, that's worth mentioning.

> The kernel initializes its value based on different reserved keys.
> Remove the thread-specific UAMOR value and don't switch the UAMOR on
> context switch.
>
> Move UAMOR initialization to key initialization code. Now that
> KUAP/KUEP feature depends on PPC_MEM_KEYS, we can start to consolidate
> all register initialization to keys init.
>
> Signed-off-by: Aneesh Kumar K.V <[email-masked]>
> ---
>  arch/powerpc/include/asm/book3s/64/kup.h |  2 ++
>  arch/powerpc/include/asm/processor.h     |  1 -
>  arch/powerpc/kernel/ptrace/ptrace-view.c | 17 ++++++++----
>  arch/powerpc/kernel/smp.c                |  5 ++++
>  arch/powerpc/mm/book3s64/pkeys.c         | 35 ++++++++++++++----------
>  5 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 3a0e138d2735..942594745dfa 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -67,6 +67,8 @@
>  #include <asm/mmu.h>
>  #include <asm/ptrace.h>
>  
> +extern u64 default_uamor;
> +
>  static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
>  {
>  	if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 52a67835057a..6ac12168f1fe 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -237,7 +237,6 @@ struct thread_struct {
>  #ifdef CONFIG_PPC_MEM_KEYS
>  	unsigned long	amr;
>  	unsigned long	iamr;
> -	unsigned long	uamor;
>  #endif
>  #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
>  	void*		kvm_shadow_vcpu; /* KVM internal data */
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index caeb5822a8f4..689711eb018a 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -488,14 +488,22 @@ static int pkey_active(struct task_struct *target, const struct user_regset *reg
>  static int pkey_get(struct task_struct *target, const struct user_regset *regset,
>  		    unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf)
>  {
> +	int ret;
> +
>  	BUILD_BUG_ON(TSO(amr) + sizeof(unsigned long) != TSO(iamr));
> -	BUILD_BUG_ON(TSO(iamr) + sizeof(unsigned long) != TSO(uamor));
>  
>  	if (!arch_pkeys_enabled())
>  		return -ENODEV;
>  
> -	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
> -				   0, ELF_NPKEY * sizeof(unsigned long));
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
> +				  0, 2 * sizeof(unsigned long));
> +	if (ret)
> +		goto err_out;

Why not just return?

> +
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &default_uamor,
> +				  2 * sizeof(unsigned long), 3 * sizeof(unsigned long));
> +err_out:
> +	return ret;
>  }
>  
>  static int pkey_set(struct task_struct *target, const struct user_regset *regset,
> @@ -518,8 +526,7 @@ static int pkey_set(struct task_struct *target, const struct user_regset *regset
>  		return ret;
>  
>  	/* UAMOR determines which bits of the AMR can be set from userspace. */
> -	target->thread.amr = (new_amr & target->thread.uamor) |
> -			     (target->thread.amr & ~target->thread.uamor);
> +	target->thread.amr = (new_amr & default_uamor) | (target->thread.amr & ~default_uamor);

That comment could explain better why we are bothering to mask with ~default_uamor.

>  	return 0;
>  }
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index c820c95162ff..eec40082599f 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -59,6 +59,7 @@
>  #include <asm/asm-prototypes.h>
>  #include <asm/cpu_has_feature.h>
>  #include <asm/ftrace.h>
> +#include <asm/kup.h>
>  
>  #ifdef DEBUG
>  #include <asm/udbg.h>
> @@ -1256,6 +1257,10 @@ void start_secondary(void *unused)
>  	mmgrab(&init_mm);
>  	current->active_mm = &init_mm;
>  
> +#ifdef CONFIG_PPC_MEM_KEYS
> +	mtspr(SPRN_UAMOR, default_uamor);
> +#endif

That's 1) not very pretty and 2) risks blowing up on other CPUs.

It should at least go in early_init_mmu_secondary().

>  	smp_store_cpu_info(cpu);
>  	set_dec(tb_ticks_per_jiffy);
>  	preempt_disable();
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index aeecc8b8e11c..3f3593f85358 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -24,7 +24,7 @@ static u32  initial_allocation_mask;   /* Bits set for the initially allocated k
>  static u64 default_amr;
>  static u64 default_iamr;
>  /* Allow all keys to be modified by default */
> -static u64 default_uamor = ~0x0UL;
> +u64 default_uamor = ~0x0UL;

__ro_after_init?

>  /*
>   * Key used to implement PROT_EXEC mmap. Denies READ/WRITE
>   * We pick key 2 because 0 is special key and 1 is reserved as per ISA.
> @@ -113,8 +113,16 @@ void __init pkey_early_init_devtree(void)
>  	/* scan the device tree for pkey feature */
>  	pkeys_total = scan_pkey_feature();
>  	if (!pkeys_total) {
> -		/* No support for pkey. Mark it disabled */
> -		return;
> +		/*
> +		 * No key support but on radix we can use key 0
> +		 * to implement kuap.
> +		 */
> +		if (early_radix_enabled())
> +			/*
> +			 * Make sure userspace can't change the AMR
> +			 */
> +			default_uamor = 0;
> +		goto err_out;

Would be cleaner if you inverted that. ie. initialise to 0 and then set
to ~0x0UL when you detect pkeys.

>  	}
>  
>  	cur_cpu_spec->mmu_features |= MMU_FTR_PKEY;
> @@ -197,6 +205,12 @@ void __init pkey_early_init_devtree(void)
>  	initial_allocation_mask |= reserved_allocation_mask;
>  
>  	pr_info("Enabling Memory keys with max key count %d", max_pkey);
> +err_out:

It's not "err" out if the OK path goes via here. That's just "out".

> +	/*
> +	 * Setup uamor on boot cpu
> +	 */
> +	mtspr(SPRN_UAMOR, default_uamor);
> +
>  	return;
>  }
>  
> @@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled)
>  		cur_cpu_spec->mmu_features |= MMU_FTR_KUAP;
>  	}
>  
> -	/* Make sure userspace can't change the AMR */
> -	mtspr(SPRN_UAMOR, 0);

Why not just leave it there. It's extra insurance and it's good
documentation.

> +	/*
> +	 * Set the default kernel AMR values on all cpus.
> +	 */
>  	mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
>  	isync();
>  }
> @@ -278,11 +293,6 @@ static inline u64 read_uamor(void)
>  	return mfspr(SPRN_UAMOR);
>  }
>  
> -static inline void write_uamor(u64 value)
> -{
> -	mtspr(SPRN_UAMOR, value);
> -}
> -
>  static bool is_pkey_enabled(int pkey)
>  {
>  	u64 uamor = read_uamor();
> @@ -353,7 +363,6 @@ void thread_pkey_regs_save(struct thread_struct *thread)
>  	 */
>  	thread->amr = read_amr();
>  	thread->iamr = read_iamr();
> -	thread->uamor = read_uamor();
>  }
>  
>  void thread_pkey_regs_restore(struct thread_struct *new_thread,
> @@ -366,8 +375,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
>  		write_amr(new_thread->amr);
>  	if (old_thread->iamr != new_thread->iamr)
>  		write_iamr(new_thread->iamr);
> -	if (old_thread->uamor != new_thread->uamor)
> -		write_uamor(new_thread->uamor);
>  }
>  
>  void thread_pkey_regs_init(struct thread_struct *thread)
> @@ -377,11 +384,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>  
>  	thread->amr   = default_amr;
>  	thread->iamr  = default_iamr;
> -	thread->uamor = default_uamor;
>  
>  	write_amr(default_amr);
>  	write_iamr(default_iamr);
> -	write_uamor(default_uamor);
>  }
>  
>  int execute_only_pkey(struct mm_struct *mm)

cheers