Date:   Tue, 8 Nov 2022 10:23:02 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     Kyle Huey <me@...ehuey.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
        x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        Robert O'Callahan <robert@...llahan.org>,
        David Manouchehri <david.manouchehri@...eup.net>,
        Borislav Petkov <bp@...e.de>, stable@...r.kernel.org
Subject: Re: [RESEND PATCH v6 1/2] x86/fpu: Allow PKRU to be (once again)
 written by ptrace.

Kyle, sorry this took so long to get to.  I do really appreciate the fix
and the selftest.  This seems like something we want to get merged
sooner rather than later, so please bear with me.

On 11/6/22 22:38, Kyle Huey wrote:
> There are three APIs that write PKRU: sigreturn, PTRACE_SETREGSET with
> NT_X86_XSTATE, and KVM_SET_XSAVE. sigreturn still uses XRSTOR to write to
> PKRU. KVM_SET_XSAVE has its own special handling to make PKRU writes take
> effect (in fpu_copy_uabi_to_guest_fpstate). Push that down into
> copy_uabi_to_xstate and have PTRACE_SETREGSET with NT_X86_XSTATE pass in
> a pointer to the appropriate PKRU slot. copy_sigframe_from_user_to_xstate
> depends on copy_uabi_to_xstate populating the PKRU field in the task's
> XSTATE so that __fpu_restore_sig can do a XRSTOR from it, so continue doing
> that.

Please always write functions() with parenthesis to make it clear what
you're talking about.  Also, there are much better ways to format this
paragraph.  I probably would have said:

There are three APIs that write PKRU:
1. sigreturn
2. PTRACE_SETREGSET with NT_X86_XSTATE
3. KVM_SET_XSAVE

Then broken it up into three follow-on paragraphs.  I actually kinda had
to do this to even make sense of what you were trying to say above.  It
would also be nice to have a clear problem statement paired with the
mention of these three ABIs.

#1 and #3 work OK, right?  It's #2 that's broken?

> sigreturn still uses XRSTOR to write to PKRU.

... which means?  That sigreturn is fine and does not need to be touched
in this patch?

> KVM_SET_XSAVE has its own special handling to make PKRU writes take
> effect (in fpu_copy_uabi_to_guest_fpstate()).  Push that down into
> copy_uabi_to_xstate() and have PTRACE_SETREGSET with NT_X86_XSTATE pass in
> a pointer to the appropriate PKRU slot. 

So this is the bugfix?  KVM already does it right, and we just need to
make ptrace() share the KVM code?

> copy_sigframe_from_user_to_xstate() depends on copy_uabi_to_xstate()
> populating the PKRU field in the task's XSTATE so that
> __fpu_restore_sig() can do a XRSTOR from it, so continue doing that.

I'm not quite sure what this chunk of the changelog is trying to tell
me.  Isn't this the sigreturn path?  Why did the paragraph above go from
talking about sigreturn to KVM then back to sigreturn?

> This also adds code to initialize the PKRU value to the hardware init value
> (namely 0) if the PKRU bit is not set in the XSTATE header provided to
> ptrace, to match XRSTOR.

The implication here is that we would like the sigreturn ABI and the
ptrace ABI to behave in a similar fashion, right?

At a high level, this patch does a *LOT*.  Generally, it's nice when
bugfixes can be encapsulted in one patch, but I think there's too much
going on here for one patch.

> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 3b28c5b25e12..c273669e8a00 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -391,8 +391,6 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
>  {
>  	struct fpstate *kstate = gfpu->fpstate;
>  	const union fpregs_state *ustate = buf;
> -	struct pkru_state *xpkru;
> -	int ret;
>  
>  	if (!cpu_feature_enabled(X86_FEATURE_XSAVE)) {
>  		if (ustate->xsave.header.xfeatures & ~XFEATURE_MASK_FPSSE)
> @@ -406,16 +404,16 @@ int fpu_copy_uabi_to_guest_fpstate(struct fpu_guest *gfpu, const void *buf,
>  	if (ustate->xsave.header.xfeatures & ~xcr0)
>  		return -EINVAL;
>  
> -	ret = copy_uabi_from_kernel_to_xstate(kstate, ustate);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * Nullify @vpkru to preserve its current value if PKRU's bit isn't set
> +	 * in the header.  KVM's odd ABI is to leave PKRU untouched in this
> +	 * case (all other components are eventually re-initialized).
> +	 * (Not clear that this is actually necessary for compat).
> +	 */
> +	if (!(ustate->xsave.header.xfeatures & XFEATURE_MASK_PKRU))
> +		vpkru = NULL;

I'm not a big fan of hunks that are part of bugfixes where it is not
clear that the hunk is necessary.

> -	/* Retrieve PKRU if not in init state */
> -	if (kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU) {
> -		xpkru = get_xsave_addr(&kstate->regs.xsave, XFEATURE_PKRU);
> -		*vpkru = xpkru->pkru;
> -	}
> -	return 0;
> +	return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);
>  }
>  EXPORT_SYMBOL_GPL(fpu_copy_uabi_to_guest_fpstate);
>  #endif /* CONFIG_KVM */
> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
> index 75ffaef8c299..6d056b68f4ed 100644
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -167,7 +167,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
>  	}
>  
>  	fpu_force_restore(fpu);
> -	ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf);
> +	ret = copy_uabi_from_kernel_to_xstate(fpu->fpstate, kbuf ?: tmpbuf, &target->thread.pkru);

I actually hadn't dug into the KVM code around this before.  It seems
like the PKRU pointer (&target->thread.pkru) here can also be
&vcpu->arch.pkru if it comes from the KVM side.

I was missing why PKRU is so special here.  I think in *both* cases,
we're copying a potential PKRU value from userspace.  But, the fpstate
target is a useless place to write PKRU because the kernel doesn't
update from there.  So, the copy-in function
(copy_uabi_from_kernel_to_xstate()) needs a place to stash PKRU where
the kernel will see it.  The place that the kernel wants to stash it is
either the task PKRU field or the KVM vcpu field.  That's what the
pointer provides.

Also, is this getting a wee bit over 80 columns?

>  out:
>  	vfree(tmpbuf);
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 91d4b6de58ab..558076dbde5b 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -396,7 +396,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
>  
>  	fpregs = &fpu->fpstate->regs;
>  	if (use_xsave() && !fx_only) {
> -		if (copy_sigframe_from_user_to_xstate(fpu->fpstate, buf_fx))
> +		if (copy_sigframe_from_user_to_xstate(tsk, buf_fx))
>  			return false;

This is also changing copy_sigframe_from_user_to_xstate() to take a
'task_struct' instead of an 'fpstate'.  Why?  That function just turns
it right back into an fpstate with: tsk->thread.fpu.fpstate.

>  	} else {
>  		if (__copy_from_user(&fpregs->fxsave, buf_fx,
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index c8340156bfd2..8f14981a3936 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1197,7 +1197,7 @@ static int copy_from_buffer(void *dst, unsigned int offset, unsigned int size,
>  
>  
>  static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
> -			       const void __user *ubuf)
> +			       const void __user *ubuf, u32 *pkru)

I think this function deserves a little comment about what it expects
from 'pkru' here.  Maybe:

/*
 * The kernel will not update the actual PKRU register from the PKRU
 * space in @fpstate.  Allow callers to pass in an alternate destination
 * for PKRU.  This is currently either the pkru field from the
 * task_struct or vcpu.
 */

>  {
>  	struct xregs_state *xsave = &fpstate->regs.xsave;
>  	unsigned int offset, size;
> @@ -1246,6 +1246,21 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
>  		}
>  	}
>  
> +	/*
> +	 * Update the user protection key storage. Allow KVM to
> +	 * pass in a NULL pkru pointer if the mask bit is unset
> +	 * for its legacy ABI behavior.
> +	 */

If I read this in 5 years, do I know what the "KVM legacy ABI behavior" is?

> +	if (pkru)
> +		*pkru = 0;
> +
> +	if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
> +		struct pkru_state *xpkru;
> +
> +		xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU);
> +		*pkru = xpkru->pkru;
> +	}

This is a bit wonky.  The code kinda pretends that XFEATURE_MASK_PKRU
and 'pkru' are independent.  But, it's actually impossible to have a
pkru==NULL and have XFEATURE_MASK_PKRU set.  The code would oops in that
case.

Would something like this be more clear?

	if (hdr.xfeatures & XFEATURE_MASK_PKRU) {
		struct pkru_state *xpkru;

		xpkru = __raw_xsave_addr(xsave, XFEATURE_PKRU);
		*pkru = xpkru->pkru;
	} else {
		/*
		 * KVM may pass a NULL 'pkru' to indicate
		 * that it does not need PKRU updated.
		 */
		if (pkru)
			*pkru = 0;
	}

Powered by blists - more mailing lists