On Thu, Aug 18, 2022 at 3:57 AM Thomas Gleixner tglx@linutronix.de wrote:
Kyle!
Hi.
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.
Well, the justification is that this *is* the behavior we want for ptrace/sigreturn, and it's very likely the existing KVM_SET_XSAVE behavior in this edge case is an oversight rather than intentional, and in the absence of confirmation that KVM wants the existing behavior (the KVM mailing list and maintainer are CCd) one correct code path is better than one correct code path and one buggy code path.
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.
Ok.
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...
Ok.
@@ -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?
It took me a bit to figure out what this is actually trying to do. To work, it would need to come at the very end of copy_uabi_to_xstate after xsave->header.xfeatures is updated. If you just want to avoid two copies I would counter-propose this though:
@@ -1235,7 +1235,19 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf, for (i = 0; i < XFEATURE_MAX; i++) { mask = BIT_ULL(i);
- if (hdr.xfeatures & mask) { + if (i == XFEATURE_PKRU) { + /* Update the user protection key storage */ + *pkru = 0; + if (hdr.xfeatures & XFEATURE_MASK_PKRU) { + struct pkru_state xpkru = {0}; + + if (copy_from_buffer(&xpkru, xstate_offsets[i], + sizeof(xpkru), kbuf, ubuf)) + return -EFAULT; + + *pkru = xpkru.pkru; + } + } else if (hdr.xfeatures & mask) { void *dst = __raw_xsave_addr(xsave, i);
offset = xstate_offsets[i];
Thoughts? This avoids a second copy and avoids having to calculate the offset into the (now potentially compressed) XSTATE.
- Kyle
Thanks,
tglx