Kyle!
On Mon, Aug 08 2022 at 07:15, Kyle Huey wrote:
When management of the PKRU register was moved away from XSTATE, emulation of PKRU's existence in XSTATE was added for APIs that read XSTATE, but not for APIs that write XSTATE. 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 relevant APIs: PTRACE_SETREGSET with NT_X86_XSTATE, sigreturn, and KVM_SET_XSAVE. 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 and sigreturn pass in pointers to the appropriate PKRU value.
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 to match XRSTOR. This is a change to the current KVM_SET_XSAVE behavior.
You are stating a fact here, but provide 0 justification why this is correct.
Changelog since v4:
Can you please put the change log past the --- seperator line, so it gets stripped off when the patch is applied? That spares manual fixups.
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: kvm@vger.kernel.org # For edge case behavior of KVM_SET_XSAVE Cc: stable@vger.kernel.org # 5.14+ Fixes: e84ba47e313d ("x86/fpu: Hook up PKRU into ptrace()")
Can you please use the documented tag ordering?
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-sub...
@@ -1235,6 +1235,24 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf, for (i = 0; i < XFEATURE_MAX; i++) { mask = BIT_ULL(i);
if (i == XFEATURE_PKRU) {/** Retrieve PKRU if not in init state, otherwise* initialize it.*/if (hdr.xfeatures & mask) {struct pkru_state xpkru = {0};if (copy_from_buffer(&xpkru, xstate_offsets[i],sizeof(xpkru), kbuf, ubuf))return -EFAULT;*pkru = xpkru.pkru;} else {*pkru = 0;}}
That's really horrible and there is no point in copying the stuff from the buffer twice:
@@ -1246,6 +1246,15 @@ static int copy_uabi_to_xstate(struct fp } }
+ /* Update the user protection key storage */ + *pkru = 0; + if (hdr.xfeatures & XFEATURE_MASK_PKRU) { + struct pkru_state *xpkru; + + xpkru = get_xsave_addr(xsave, XFEATURE_PKRU); + *pkru = xpkru->pkru; + } +
Hmm?
Thanks,
tglx