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

2020-07-07 Thread Aneesh Kumar K.V
Michael Ellerman  writes:

> "Aneesh Kumar K.V"  writes:
>  
.

>> @@ -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.

We can't se the value to 0, because with hash kuap it is derived
from what other keys are used for. Are you suggesting to keep it as

if (radix_enabled())
mtspr(SPRN_UAMOR, 0);

That would confuse w.r.t what happens with hash.

I can add a comment there explaining details? 

>
>> +/*
>> + * 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

-aneesh


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

2020-07-06 Thread Michael Ellerman
"Aneesh Kumar K.V"  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 
> ---
>  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 
>  #include 
>  
> +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 
>  #include 
>  #include 
> +#include 
>  
>  #ifdef DEBUG
>  #include 
> @@ -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 */
> -st