linux-kernel - Re: [PATCH v5 1/2] x86/fpu: Allow PKRU to be (once again) written by ptrace.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ilmpzunz.ffs@tglx>
Date:   Thu, 18 Aug 2022 12:57:04 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Kyle Huey <me@...ehuey.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Borislav Petkov <bp@...en8.de>
Cc:     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>,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        Robert O'Callahan <robert@...llahan.org>,
        David Manouchehri <david.manouchehri@...eup.net>,
        Kyle Huey <me@...ehuey.com>, Borislav Petkov <bp@...e.de>,
        kvm@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v5 1/2] x86/fpu: Allow PKRU to be (once again) written
 by ptrace.

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@...ehuey.com>
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: kvm@...r.kernel.org # For edge case behavior of KVM_SET_XSAVE
> Cc: stable@...r.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-submission-notes

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