Received-SPF: pass (google.com: domain of linux-kernel+bounces-171869-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>,
 linux-kernel@vger.kernel.org
Cc: x86@kernel.org, dave.hansen@linux.intel.com, mingo@kernel.org,
 keith.lucas@oracle.com, aruna.ramakrishna@oracle.com
Subject: Re: [PATCH v3 3/4] x86/pkeys: Update PKRU to enable all pkeys
 before XSAVE
In-Reply-To: <20240425180542.1042933-4-aruna.ramakrishna@oracle.com>
References: <20240425180542.1042933-1-aruna.ramakrishna@oracle.com>
 <20240425180542.1042933-4-aruna.ramakrishna@oracle.com>
Date: Tue, 07 May 2024 18:47:41 +0200
Message-ID: <87wmo5po0i.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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