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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 07 May 2024 18:47:41 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>,
 linux-kernel@...r.kernel.org
Cc: x86@...nel.org, dave.hansen@...ux.intel.com, mingo@...nel.org,
 keith.lucas@...cle.com, aruna.ramakrishna@...cle.com
Subject: Re: [PATCH v3 3/4] x86/pkeys: Update PKRU to enable all pkeys
 before XSAVE

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