Re [PATCH v5 1/5] x86/pkeys: Add PKRU as a parameter in signal handling functions

[jeffxu]

On Thu, Jun 06, 2024 at 10:40:31PM +0000, Aruna Ramakrishna wrote:

> diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
> index e770c4fc47f4..de458354a3ea 100644
> --- a/arch/x86/include/asm/sighandling.h
> +++ b/arch/x86/include/asm/sighandling.h
> @@ -17,11 +17,11 @@ void signal_fault(struct pt_regs *regs, void __user *frame, char *where);
>  
>  void __user *
>  get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> -	     void __user **fpstate);
> +	     void __user **fpstate, u32 pkru);
>  
> -int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs);
> -int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> -int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> -int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs);
> +int ia32_setup_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
> +int ia32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
> +int x64_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);
> +int x32_setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru);

Does ia32 support PKEY ? I thought it is X64 only.
It might be possible to not to change any of ia32 code.

>  
>  #endif /* _ASM_X86_SIGHANDLING_H */
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 247f2225aa9f..2b3b9e140dd4 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -156,7 +156,7 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
>  	return !err;
>  }
>  
> -static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
> +static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf, u32 pkru)
>  {
>  	if (use_xsave())
>  		return xsave_to_user_sigframe(buf);
> @@ -185,7 +185,7 @@ static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
>   * For [f]xsave state, update the SW reserved fields in the [f]xsave frame
>   * indicating the absence/presence of the extended state to the user.
>   */
> -bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
> +bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size, u32 pkru)
>  {
>  	struct task_struct *tsk = current;
>  	struct fpstate *fpstate = tsk->thread.fpu.fpstate;
> @@ -228,7 +228,7 @@ bool copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  		fpregs_restore_userregs();
>  
>  	pagefault_disable();
> -	ret = copy_fpregs_to_sigframe(buf_fx);
> +	ret = copy_fpregs_to_sigframe(buf_fx, pkru);
>  	pagefault_enable();
>  	fpregs_unlock();
>  
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index 31b6f5dddfc2..94b894437327 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -74,7 +74,7 @@ static inline int is_x32_frame(struct ksignal *ksig)
>   */
>  void __user *
>  get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
> -	     void __user **fpstate)
> +	     void __user **fpstate, u32 pkru)
>  {
>  	struct k_sigaction *ka = &ksig->ka;
>  	int ia32_frame = is_ia32_frame(ksig);
> @@ -139,7 +139,7 @@ get_sigframe(struct ksignal *ksig, struct pt_regs *regs, size_t frame_size,
>  	}
>  
>  	/* save i387 and extended state */
> -	if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size))
> +	if (!copy_fpstate_to_sigframe(*fpstate, (void __user *)buf_fx, math_size, pkru))

You might find that we only need to update PKRU right before copy_fpstate_to_sigframe,
so no need to pass pkru all the way from top.

-Jeff