Re: [PATCH v3 3/4] x86/pkeys: Update PKRU to enable all pkeys before XSAVE

[Thomas Gleixner]

On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:

> If the alternate signal stack is protected by a different pkey than the
> current execution stack, copying xsave data to the altsigstack will fail
> if its pkey is not enabled. This commit enables all pkeys before
> xsave,

This commit (patch) ....

Also this lacks any justification why this enables all pkeys and how
that is the right thing to do instead of using init_pkru_value which
is what is set by fpu__clear_user_states() before going back to user
space. For signal handling this can be the only valid PKEY state unless
I'm missing something here.

>  static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf,
>  					  u32 pkru)
>  {
> -	if (use_xsave())
> -		return xsave_to_user_sigframe(buf);
> +	int err = 0;
> +
> +	if (use_xsave()) {
> +		err = xsave_to_user_sigframe(buf);
> +		if (!err && cpu_feature_enabled(X86_FEATURE_OSPKE))

The CPU feature check really wants to be in update_pkru_in_sigframe()

> @@ -278,6 +278,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  	if (stepping)
>  		user_disable_single_step(current);
>  
> +	pkru = sig_prepare_pkru();

pkru is defined in the first patch:

> +       u32 pkru = read_pkru();

Why do we need a read and then another read in sig_prepare_pkru()?

Also this lacks a comment what the sig_prepare_pkru() invocation is for ...

>  	failed = (setup_rt_frame(ksig, regs, pkru) < 0);
>  	if (!failed) {
>  		/*
> @@ -295,6 +296,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  		 * Ensure the signal handler starts with the new fpu state.
>  		 */
>  		fpu__clear_user_states(fpu);
> +	} else {
> +		write_pkru(pkru);

.. and a corresponding comment why this needs to be restored here.

>  	}
>  	signal_setup_done(failed, ksig, stepping);
>  }

Thanks,

        tglx