[RESEND,v6,1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace.

  From: Kyle Huey <me@kylehuey.com>

When management of the PKRU register was moved away from XSTATE, emulation
of PKRU's existence in XSTATE was added for reading PKRU through ptrace,
but not for writing PKRU through ptrace. This can be seen by running gdb
and executing `p $pkru`, `set $pkru = 42`, and `p $pkru`. On affected
kernels (5.14+) the write to the PKRU register (which gdb performs through
ptrace) is ignored.

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.

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.

Changelog since v5:
- Avoids a second copy from the uabi buffer as suggested.
- Preserves old KVM_SET_XSAVE behavior where leaving the PKRU bit in the
  XSTATE header results in PKRU remaining unchanged instead of
  reinitializing it.
- Fixed up patch metadata as requested.

Changelog since v4:
- Selftest additionally checks PKRU readbacks through ptrace.
- Selftest flips all PKRU bits (except the default key).

Changelog since v3:
- The v3 patch is now part 1 of 2.
- Adds a selftest in part 2 of 2.

Changelog since v2:
- Removed now unused variables in fpu_copy_uabi_to_guest_fpstate

Changelog since v1:
- Handles the error case of copy_to_buffer().

Fixes: e84ba47e313d ("x86/fpu: Hook up PKRU into ptrace()")
Signed-off-by: Kyle Huey <me@kylehuey.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: stable@vger.kernel.org # 5.14+
---
 arch/x86/kernel/fpu/core.c   | 20 +++++++++-----------
 arch/x86/kernel/fpu/regset.c |  2 +-
 arch/x86/kernel/fpu/signal.c |  2 +-
 arch/x86/kernel/fpu/xstate.c | 25 ++++++++++++++++++++-----
 arch/x86/kernel/fpu/xstate.h |  4 ++--
 5 files changed, 33 insertions(+), 20 deletions(-)
  

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;
	}
  

On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> 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?

Well this depends on one defines work. If you use my definition of
"behaves equivalently to a hardware XRSTOR instruction" then #1
works, #2 is totally broken, and #3 is subtly broken. But #3 has
been the way that it is for a long time and the KVM maintainers
don't want to change it, whereas #2 was broken recently (in the
commit this fixes).

> > sigreturn still uses XRSTOR to write to PKRU.
>
> ... which means?  That sigreturn is fine and does not need to be touched
> in this patch?

Yes (modulo refactoring).

> > 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?

Largely but not entirely, because KVM's behavior is subtly different
from XRSTOR's.
KVM doesn't reinitialize PKRU to the hardware init value if the PKRU bit is
not set in the xfeatures mask, whereas XRSTOR does (and thus ptrace previously
did).

> > 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?

It's telling you that nothing really changed there. I can drop that.

> > 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?

I don't personally care about sigreturn that much but I would like the
ptrace ABI to behave like XRSTOR (which it did before 5.14), and the
sigreturn ABI behaves like XRSTOR because it *is* XRSTOR (both
before and after 5.14), so the ptrace ABI behaving like the sigreturn ABI
arises transitively.

> 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.

Ok. How about I break the first part into two pieces, one that changes the
signatures of copy_uabi_from_kernel_to_xstate() and
copy_sigframe_from_user_to_xstate(), and one that moves the relevant
KVM code from fpu_copy_uabi_to_guest_fpstate() to copy_uabi_to_xstate()
and deals with the edge case behavior of the mask?

> > 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.

This is necessary to avoid changing KVM's behavior at the same time
that we change
ptrace, since KVM doesn't want the same behavior as ptrace.

> > -     /* 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.

Right. Where we need to put PKRU to get the kernel to swap it in
varies depending
on whether this is a normal task or a VM.

> 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.

Right.

> 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.

Right.

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

Probably.

> >  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.

So that we can also access tsk->thread.pkru at the same time.

> >       } 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:

Ok.

> /*
>  * 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.
>  */

Or NULL, but yeah, sure.

> >  {
> >       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?

Probably not. I'll be more specific.

> > +     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.

Right.

> 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;
>         }

Yeah, Sean Christopherson suggested this (with the else and if
collapsed into a single level) when I submitted this previously.

Thanks,

- Kyle
  

On 11/10/22 16:03, Kyle Huey wrote:
> On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen <dave.hansen@intel.com> wrote:
...
>> 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.
> 
> Ok. How about I break the first part into two pieces, one that changes the
> signatures of copy_uabi_from_kernel_to_xstate() and
> copy_sigframe_from_user_to_xstate(), and one that moves the relevant
> KVM code from fpu_copy_uabi_to_guest_fpstate() to copy_uabi_to_xstate()
> and deals with the edge case behavior of the mask?

Sounds like a good start.  My gut says there's another patch or two that
could be broken out, but that sounds like a reasonable next step.

>>> 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.
> 
> This is necessary to avoid changing KVM's behavior at the same time
> that we change
> ptrace, since KVM doesn't want the same behavior as ptrace.

Your "This is necessary" doesn't really match with "Not clear that this
is actually necessary" from the comment, right?

Rather than claim whether it is necessary or not, maybe just say why
it's there: it's there to preserve wonky KVM behavior.

BTW, I'd love to know if KVM *REALLY* depends on this.  It'd be nice to
kill if not.
>> 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;
>>         }
> 
> Yeah, Sean Christopherson suggested this (with the else and if
> collapsed into a single level) when I submitted this previously.

I generally agree with Sean, but he's also been guilty of an atrocity or
two over the years.  :)  While I generally like low levels of
indentation I also think my version is much more clear in this case.
  

On Thu, Nov 10, 2022 at 5:38 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 11/10/22 16:03, Kyle Huey wrote:
> > On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen <dave.hansen@intel.com> wrote:
> ...
> >> 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.
> >
> > Ok. How about I break the first part into two pieces, one that changes the
> > signatures of copy_uabi_from_kernel_to_xstate() and
> > copy_sigframe_from_user_to_xstate(), and one that moves the relevant
> > KVM code from fpu_copy_uabi_to_guest_fpstate() to copy_uabi_to_xstate()
> > and deals with the edge case behavior of the mask?
>
> Sounds like a good start.  My gut says there's another patch or two that
> could be broken out, but that sounds like a reasonable next step.
>
> >>> 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.
> >
> > This is necessary to avoid changing KVM's behavior at the same time
> > that we change
> > ptrace, since KVM doesn't want the same behavior as ptrace.
>
> Your "This is necessary" doesn't really match with "Not clear that this
> is actually necessary" from the comment, right?
>
> Rather than claim whether it is necessary or not, maybe just say why
> it's there: it's there to preserve wonky KVM behavior.
>
> BTW, I'd love to know if KVM *REALLY* depends on this.  It'd be nice to
> kill if not.

qemu didn't appear to (it treats the KVM_GET_XSAVE2/KVM_SET_XSAVE
buffers as opaque blobs afaict) but it's of course not the only KVM
application out there.

> >> 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;
> >>         }
> >
> > Yeah, Sean Christopherson suggested this (with the else and if
> > collapsed into a single level) when I submitted this previously.
>
> I generally agree with Sean, but he's also been guilty of an atrocity or
> two over the years.  :)  While I generally like low levels of
> indentation I also think my version is much more clear in this case.
>

- Kyle
  

On Thu, Nov 10, 2022, Dave Hansen wrote:
> On 11/10/22 16:03, Kyle Huey wrote:
> > On Tue, Nov 8, 2022 at 10:23 AM Dave Hansen <dave.hansen@intel.com> wrote:
> BTW, I'd love to know if KVM *REALLY* depends on this.

Unlikely, but nearly impossible to know for sure.  Copy+pasting my response[1] to
an earlier version.

 : Hrm, the current behavior has been KVM ABI for a very long time.
 : 
 : It's definitely odd because all other components will be initialized due to their
 : bits being cleared in the header during kvm_load_guest_fpu(), and it probably
 : wouldn't cause problems in practice as most VMMs likely do "all or nothing" loads.
 : But, in theory, userspace could save/restore a subset of guest XSTATE and rely on
 : the kernel not overwriting guest PKRU when its bit is cleared in the header.
 : 
 : All that said, I don't see any reason to force KVM to change at this time, it's
 : trivial enough to handle KVM's oddities while providing sane behavior for others.
 : Nullify the pointer in the guest path and then update copy_uabi_to_xstate() to
 : play nice with a NULL pointer, e.g. 
 : 
 : 	/*
 : 	 * 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).
 : 	 */
 : 	if (!(kstate->regs.xsave.header.xfeatures & XFEATURE_MASK_PKRU))
 : 		vpkru = NULL;
 : 
 : 	return copy_uabi_from_kernel_to_xstate(kstate, ustate, vpkru);

> It'd be nice to kill if not.

I don't disagree, my hesitation is purely that doing so might subtly break
userspace.

That said, I'm 99.9% certain no traditional VMM, e.g. QEMU, is relying on this
behavior, as doing KVM_SET_XSAVE with anything except the guest's xfeatures mask
would corrupt guest XSAVE state for everything except PKRU.  I.e. for all intents
and purposes, a traditional VMM must do KVM_GET_SAVE => KVM_SET_XSAVE without
touching the xfeatures mask.

And for non-traditional usage of KVM, I would be quite surprised if any of those
use cases utilize PKRU in the guest, let alone play games with KVM_{G,S}SET_XSAVE.

So, I'm not completely opposed to "fixing" KVM's ABI, but it should be done as a
separate patch that is tagged "KVM: x86:" and clearly states that it's changing
KVM's ABI in a way that could theoretically break userspace.

> >> 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;
> >>         }
> > 
> > Yeah, Sean Christopherson suggested this (with the else and if
> > collapsed into a single level) when I submitted this previously.
> 
> I generally agree with Sean, but he's also been guilty of an atrocity or
> two over the years.  :)

Heh, just one or two?  I'll call that a win.

> While I generally like low levels of indentation I also think my version is
> much more clear in this case.

I've no objection to a standalone if.  My suggestion[2] was in response to code
that zeroed @pkru before the XFEATURE_MASK_PKRU check.

      if (pkru)
              *pkru = 0;

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

[1] https://lore.kernel.org/all/Yv6szXuKGv75wWmm@google.com
[2] https://lore.kernel.org/all/YxDP6jie4cwzZIHp@google.com