[v7,0/6] x86/fpu: Allow PKRU to be (once again) written by ptrace

Message

Kyle Huey Nov. 15, 2022, 11:09 p.m. UTC

Hi.

Following last week's discussion I've reorganized this patch. The goal
remains to restore the pre-5.14 behavior of ptrace(PTRACE_SET_REGSET,
NT_X86_XSTATE) for the PKRU register (which was equivalent to a hardware
XRSTOR instruction).

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

sigreturn restores PKRU from the fpstate and works as expected today.

PTRACE_SET_REGSET restores PKRU from the thread_struct's pkru member and
doesn't work at all.

KVM_SET_XSAVE restores PKRU from the vcpu's pkru member and honors
changes to the PKRU value in the XSAVE region but does not honor clearing
the PKRU bit in the xfeatures mask. The KVM maintainers do not want to
change the KVM behavior at the current time, however, so this quirk
survives after this patch set.

All three APIs ultimately call into copy_uabi_to_xstate(). Part 3 adds
an argument to that function that is used to pass in a pointer to either
the thread_struct's pkru or the vcpu's PKRU, for sigreturn/PTRACE_SET_REGSET
or KVM_SET_XSAVE respectively. While this isn't strictly necessary for
sigreturn, it makes part 5 easier. Parts 1 and 2 refactor the various
callers of copy_uabi_to_xstate() to make that possible.

Part 4 moves the existing KVM-specific PKRU handling in
fpu_copy_uabi_to_guest_fpstate() to copy_uabi_to_xstate() where it is now
shared amongst all three APIs. This is a no-op for sigreturn (which restores
PKRU from the fpstate anyways) and KVM but it changes the PTRACE_SET_REGSET
behavior to match KVM_SET_XSAVE.

Part 5 emulates the hardware XRSTOR behavior where PKRU is reset to the
hardware init value if the PKRU bit in the xfeatures mask is clear. KVM is
excluded from this emulation by passing a NULL pkru slot pointer to
copy_uabi_to_xstate() in this case. Passing in a pointer to the
thread_struct's PKRU slot for sigreturn (even though sigreturn won't restore
PKRU from that location) allows distinguishing KVM here. This changes
the PTRACE_SET_REGSET behavior to fully match sigreturn.

Part 6 is the self test that remains unchanged from v3 of this patchset.

At no point in this patch set is the user-visible behavior of sigreturn
or KVM_SET_XSAVE changed.

Changelog since v6:
- v6's part 1/2 is now split into parts 1 through 5.
- v6's part 2/2 is now part 6.
- Various style comments addressed.

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

Comments

Dave Hansen Nov. 16, 2022, 11:31 p.m. UTC | #1

On 11/15/22 15:09, Kyle Huey wrote:
> Following last week's discussion I've reorganized this patch. The goal
> remains to restore the pre-5.14 behavior of ptrace(PTRACE_SET_REGSET,
> NT_X86_XSTATE) for the PKRU register (which was equivalent to a hardware
> XRSTOR instruction).

The new version looks great.  I've applied it.

I did remove the stable@ tags for now.  There were a couple reasons for
that.  First, most of the x86 stuff marked for stable@ goes via our
tip/urgent branch and this doesn't seem super urgent.  It also touches
code that's exposed in at least three separate UABIs, so I want a bit
more soak time than x86/urgent normally provides.

I have zero objections if anyone wants to submit it to stable@ after it
hits Linus's tree.

Kyle Huey Nov. 17, 2022, 12:45 a.m. UTC | #2

On Wed, Nov 16, 2022 at 3:31 PM Dave Hansen <[e-mail]> wrote:
>
> On 11/15/22 15:09, Kyle Huey wrote:
> > Following last week's discussion I've reorganized this patch. The goal
> > remains to restore the pre-5.14 behavior of ptrace(PTRACE_SET_REGSET,
> > NT_X86_XSTATE) for the PKRU register (which was equivalent to a hardware
> > XRSTOR instruction).
>
> The new version looks great.  I've applied it.
>
> I did remove the stable@ tags for now.  There were a couple reasons for
> that.  First, most of the x86 stuff marked for stable@ goes via our
> tip/urgent branch and this doesn't seem super urgent.  It also touches
> code that's exposed in at least three separate UABIs, so I want a bit
> more soak time than x86/urgent normally provides.
>
> I have zero objections if anyone wants to submit it to stable@ after it
> hits Linus's tree.

Works for me, thanks.

- Kyle