Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
To:     ira.weiny@intel.com, Michael Ellerman <mpe@ellerman.id.au>,
        Dave Hansen <dave.hansen@linux.intel.com>
Cc:     Ira Weiny <ira.weiny@intel.com>, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pkeys: Make pkey unsigned in arch_set_user_pkey_access()
In-Reply-To: <20220304210543.3490880-1-ira.weiny@intel.com>
References: <20220304210543.3490880-1-ira.weiny@intel.com>
Date:   Mon, 07 Mar 2022 12:30:03 +0530
Message-ID: <878rtmtfgs.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

ira.weiny@intel.com writes:

> From: Ira Weiny <ira.weiny@intel.com>
>
> The WARN_ON check in arch_set_user_pkey_access() in the x86 architecture
> fails to check for an invalid negative value.
>
> A simple check for less than 0 would fix this issue however, in the call
> stack below arch_set_user_pkey_access() the pkey should never be
> negative on any architecture.  It is always best to use correct types
> when possible.  x86 only supports 16 keys while ppc supports 32, u8 is
> therefore large enough for all current architectures and likely those in
> the future.

Should we do that as a separate patch? ie, now convert the variable to
unsigned int and later switch all the variables to u8? because what we
now have is confusing.

static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
		unsigned long pkey)
static inline u64 pkey_to_vmflag_bits(u16 pkey)


>
> Change the type of the pkey passed to arch_set_user_pkey_access() to u8.
>
> To: Dave Hansen <dave.hansen@linux.intel.com>
> To: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  arch/powerpc/include/asm/pkeys.h | 4 ++--
>  arch/powerpc/mm/book3s64/pkeys.c | 2 +-
>  arch/x86/include/asm/pkeys.h     | 4 ++--
>  arch/x86/kernel/fpu/xstate.c     | 2 +-
>  include/linux/pkeys.h            | 2 +-
>  5 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 59a2c7dbc78f..e70615a1da9b 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -143,9 +143,9 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
>  	return __arch_override_mprotect_pkey(vma, prot, pkey);
>  }
>  
> -extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +extern int __arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  				       unsigned long init_val);


> -static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +static inline int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  					    unsigned long init_val)
>  {
>  	if (!mmu_has_feature(MMU_FTR_PKEY))
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index 753e62ba67af..c048467669df 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -333,7 +333,7 @@ static inline void init_iamr(int pkey, u8 init_bits)
>   * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
>   * specified in @init_val.
>   */
> -int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +int __arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  				unsigned long init_val)
>  {
>  	u64 new_amr_bits = 0x0ul;
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 5292e6dfe2a7..48efb81f6cc6 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -9,7 +9,7 @@
>   */
>  #define arch_max_pkey() (cpu_feature_enabled(X86_FEATURE_OSPKE) ? 16 : 1)
>  
> -extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +extern int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  		unsigned long init_val);
>  
>  static inline bool arch_pkeys_enabled(void)
> @@ -115,7 +115,7 @@ int mm_pkey_free(struct mm_struct *mm, int pkey)
>  	return 0;
>  }
>  
> -extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +extern int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  		unsigned long init_val);
>  
>  static inline int vma_pkey(struct vm_area_struct *vma)
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 7c7824ae7862..db511bec57e5 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1068,7 +1068,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
>   * This will go out and modify PKRU register to set the access
>   * rights for @pkey to @init_val.
>   */
> -int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  			      unsigned long init_val)
>  {
>  	u32 old_pkru, new_pkru_bits = 0;
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 86be8bf27b41..aa40ed2fb0fc 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -35,7 +35,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
>  	return -EINVAL;
>  }
>  
> -static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> +static inline int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
>  			unsigned long init_val)
>  {
>  	return 0;
> -- 
> 2.35.1