[PATCH v2 0/4] x86/fpu: Fix MXCSR handling and SSE component definition

[PATCH v2 0/4] x86/fpu: Fix MXCSR handling and SSE component definition

[Chang S. Bae]

Hi all,

Changes from v1:
* patch3: revert the function name change and add a warning. Then update
  the changelog and title.

Thanks Sean Christopherson for the feedback.

== Regression ==

Recently the XSTATE copy functions were unitized together [2]. At a glance,
this change appears to relapse the ptrace write on the MXCSR state when
the non-compacted format is used in the kernel.

But, this regression appears to root in the XSAVES-enabling code [3] that
introduced the XSTATE conversion along with the MXCSR mistreatment.

== MXCSR Hindsight ==

MXCSR is architecturally part of the SSE component. The MXCSR association
of XSTATE_BV depends on the XSAVE format.

The change [3], however, presumed MXCSR as part of the X87 component and
made the kernel referencing XSTATE_BV regardless of the format.

== Patches ==

* Fix the MXCSR conversion code along with adding the test case.
* Then, fixing MXCSR, one of the other call sites is also updated to
  exclude legacy states.
* The hard-coded legacy state offset and size are adjusted in the end.

These patches can be also found in this repository:
  git://github.com/intel/amx-linux.git mxcsr

Thanks,
Chang

[1] v1: https://lore.kernel.org/lkml/20220916201158.8072-1-chang.seok.bae@intel.com/
[2] Commit 43be46e89698 ("x86/fpu: Sanitize xstateregs_set()")
[3] Commit 91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")

Chang S. Bae (4):
  x86/fpu: Fix the MXCSR state reshuffling between userspace and kernel
    buffers
  selftests/x86/mxcsr: Test the MXCSR state write via ptrace
  x86/fpu: Disallow legacy states from fpstate_clear_xstate_component()
  x86/fpu: Correct the legacy state offset and size information

 arch/x86/kernel/fpu/xstate.c         | 100 ++++++++++----
 tools/testing/selftests/x86/Makefile |   2 +-
 tools/testing/selftests/x86/mxcsr.c  | 200 +++++++++++++++++++++++++++
 3 files changed, 274 insertions(+), 28 deletions(-)
 create mode 100644 tools/testing/selftests/x86/mxcsr.c


base-commit: 08ed00508bc1fa0a0bc6dd2420f982b55051de23
-- 
2.17.1

[PATCH v2 1/4] x86/fpu: Fix the MXCSR state reshuffling between userspace and kernel buffers

[Chang S. Bae]

== Hardware Background ==

The MXCSR state, as part of the SSE component, has different treatments
with XSAVE*/XRSTOR* between the two XSAVE formats:

- When the MXCSR state is XSAVEd in the non-compacted format, the feature
bit in XSTATE_BV pertains to the XMM registers. XRSTOR restores the MXCSR
state without referencing XSTATE_BV.

- But, on XSAVE* with the compacted format, the SSE bit is set in XSTATE_BV
if MXCSR is not in the init state on XSAVE*. Then, on XRSTOR*, the state is
restored only when the SSE bit is set in XSTATE_BV.

== Regression ==

The XSTATE copy routine between userspace and kernel buffers used to be
separate for different XSAVE formats. Commit 43be46e89698 ("x86/fpu:
Sanitize xstateregs_set()") combined them together.

This change appears to be a regression on XSAVES-less systems. But, the
merged code is based on the original conversion code with commit
91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES").

That has such oversight as:
- Mistreating MXCSR as part of the FP state instead of the SSE component.
- Taking the SSE bit in XSTATE_BV always for all the SSE states.

== Correction ==

Update the XSTATE conversion code:
- Refactor the copy routine for legacy states. Treat MXCSR as part of SSE.
- Copying MXCSR, reference XSTATE_BV only with the compacted format.
- Also, flip the SSE bit in XSTATE_BV accordingly to the format.

Reported-by: Andrei Vagin <avagin@gmail.com>
Fixes: 91c3dba7dbc1 ("x86/fpu/xstate: Fix PTRACE frames for XSAVES")
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Link: https://lore.kernel.org/lkml/CANaxB-wkcNKWjyNGFuMn6f6H2DQSGwwQjUgg1eATdUgmM-Kg+A@mail.gmail.com/
---
 arch/x86/kernel/fpu/xstate.c | 70 +++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8340156bfd2..d7676cfc32eb 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1064,6 +1064,7 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
 			       u32 pkru_val, enum xstate_copy_mode copy_mode)
 {
 	const unsigned int off_mxcsr = offsetof(struct fxregs_state, mxcsr);
+	bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
 	struct xregs_state *xinit = &init_fpstate.regs.xsave;
 	struct xregs_state *xsave = &fpstate->regs.xsave;
 	struct xstate_header header;
@@ -1093,8 +1094,13 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
 	copy_feature(header.xfeatures & XFEATURE_MASK_FP, &to, &xsave->i387,
 		     &xinit->i387, off_mxcsr);
 
-	/* Copy MXCSR when SSE or YMM are set in the feature mask */
-	copy_feature(header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
+	/*
+	 * Copy MXCSR depending on the XSAVE format. If compacted,
+	 * reference the feature mask. Otherwise, check if any of related
+	 * features is valid.
+	 */
+	copy_feature(compacted ? header.xfeatures & XFEATURE_MASK_SSE :
+		     fpstate->user_xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
 		     &to, &xsave->i387.mxcsr, &xinit->i387.mxcsr,
 		     MXCSR_AND_FLAGS_SIZE);
 
@@ -1199,6 +1205,11 @@ 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 unsigned int off_stspace = offsetof(struct fxregs_state, st_space);
+	const unsigned int off_xmm = offsetof(struct fxregs_state, xmm_space);
+	const unsigned int off_mxcsr = offsetof(struct fxregs_state, mxcsr);
+	bool compacted = cpu_feature_enabled(X86_FEATURE_XCOMPACTED);
+	struct fxregs_state *fxsave = &fpstate->regs.fxsave;
 	struct xregs_state *xsave = &fpstate->regs.xsave;
 	unsigned int offset, size;
 	struct xstate_header hdr;
@@ -1212,38 +1223,48 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
 	if (validate_user_xstate_header(&hdr, fpstate))
 		return -EINVAL;
 
-	/* Validate MXCSR when any of the related features is in use */
-	mask = XFEATURE_MASK_FP | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
-	if (hdr.xfeatures & mask) {
+	if (hdr.xfeatures & XFEATURE_MASK_FP) {
+		if (copy_from_buffer(fxsave, 0, off_mxcsr, kbuf, ubuf))
+			return -EINVAL;
+		if (copy_from_buffer(fxsave->st_space, off_stspace, sizeof(fxsave->st_space),
+				     kbuf, ubuf))
+			return -EINVAL;
+	}
+
+	/* Validate MXCSR when any of the related features is valid. */
+	mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
+	if (fpstate->user_xfeatures & mask) {
 		u32 mxcsr[2];
 
-		offset = offsetof(struct fxregs_state, mxcsr);
-		if (copy_from_buffer(mxcsr, offset, sizeof(mxcsr), kbuf, ubuf))
+		if (copy_from_buffer(mxcsr, off_mxcsr, sizeof(mxcsr), kbuf, ubuf))
 			return -EFAULT;
 
 		/* Reserved bits in MXCSR must be zero. */
 		if (mxcsr[0] & ~mxcsr_feature_mask)
 			return -EINVAL;
 
-		/* SSE and YMM require MXCSR even when FP is not in use. */
-		if (!(hdr.xfeatures & XFEATURE_MASK_FP)) {
-			xsave->i387.mxcsr = mxcsr[0];
-			xsave->i387.mxcsr_mask = mxcsr[1];
-		}
+		/*
+		 * Copy MXCSR regardless of the feature mask as userspace
+		 * uses the uncompacted format.
+		 */
+		fxsave->mxcsr = mxcsr[0];
+		fxsave->mxcsr_mask = mxcsr[1];
 	}
 
-	for (i = 0; i < XFEATURE_MAX; i++) {
-		mask = BIT_ULL(i);
+	if (hdr.xfeatures & XFEATURE_MASK_SSE) {
+		if (copy_from_buffer(fxsave->xmm_space, off_xmm, sizeof(fxsave->xmm_space),
+				     kbuf, ubuf))
+			return -EINVAL;
+	}
 
-		if (hdr.xfeatures & mask) {
-			void *dst = __raw_xsave_addr(xsave, i);
+	for_each_extended_xfeature(i, hdr.xfeatures) {
+		void *dst = __raw_xsave_addr(xsave, i);
 
-			offset = xstate_offsets[i];
-			size = xstate_sizes[i];
+		offset = xstate_offsets[i];
+		size = xstate_sizes[i];
 
-			if (copy_from_buffer(dst, offset, size, kbuf, ubuf))
-				return -EFAULT;
-		}
+		if (copy_from_buffer(dst, offset, size, kbuf, ubuf))
+			return -EFAULT;
 	}
 
 	/*
@@ -1256,6 +1277,13 @@ static int copy_uabi_to_xstate(struct fpstate *fpstate, const void *kbuf,
 	 * Add back in the features that came in from userspace:
 	 */
 	xsave->header.xfeatures |= hdr.xfeatures;
+	/*
+	 * Convert the SSE bit in the feature mask as it implies
+	 * differently between the formats. It indicates the MXCSR state
+	 * if compacted; otherwise, it pertains to XMM registers.
+	 */
+	if (compacted && fxsave->mxcsr != MXCSR_DEFAULT)
+		xsave->header.xfeatures |= XFEATURE_MASK_SSE;
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 2/4] selftests/x86/mxcsr: Test the MXCSR state write via ptrace

[Chang S. Bae]

The ptrace buffer is in the non-compacted format. The MXCSR state should be
written to the target thread when either SSE or AVX component is enabled.

Write an MXCSR value to the target and read back. Then it is validated with
the XRSTOR/XSAVE result on the current.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-kselftest@vger.kernel.org
---

If this is acceptable, I will also follow up to move some of the helper
functions to a .h file from this and other test cases because duplicating
what is shareable should be avoided.
---
 tools/testing/selftests/x86/Makefile |   2 +-
 tools/testing/selftests/x86/mxcsr.c  | 200 +++++++++++++++++++++++++++
 2 files changed, 201 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/x86/mxcsr.c

diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 0388c4d60af0..621c47960be3 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -13,7 +13,7 @@ CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh "$(CC)" trivial_program.c -no-pie)
 TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
 			check_initial_reg_state sigreturn iopl ioperm \
 			test_vsyscall mov_ss_trap \
-			syscall_arg_fault fsgsbase_restore sigaltstack
+			syscall_arg_fault fsgsbase_restore sigaltstack mxcsr
 TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
diff --git a/tools/testing/selftests/x86/mxcsr.c b/tools/testing/selftests/x86/mxcsr.c
new file mode 100644
index 000000000000..7c318c48b4be
--- /dev/null
+++ b/tools/testing/selftests/x86/mxcsr.c
@@ -0,0 +1,200 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <err.h>
+#include <elf.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <x86intrin.h>
+
+#include <sys/ptrace.h>
+#include <sys/syscall.h>
+#include <sys/wait.h>
+#include <sys/uio.h>
+
+#include "../kselftest.h" /* For __cpuid_count() */
+
+#define LEGACY_STATE_SIZE	24
+#define MXCSR_SIZE		8
+#define STSTATE_SIZE		8*16
+#define XMM_SIZE		16*16
+#define PADDING_SIZE		96
+#define XSAVE_HDR_SIZE		64
+
+struct xsave_buffer {
+	uint8_t		legacy_state[LEGACY_STATE_SIZE];
+	uint8_t		mxcsr[MXCSR_SIZE];
+	uint8_t		st_state[STSTATE_SIZE];
+	uint8_t		xmm_state[XMM_SIZE];
+	uint8_t		padding[PADDING_SIZE];
+	uint8_t		header[XSAVE_HDR_SIZE];
+	uint8_t		extended[0];
+};
+
+#ifdef __x86_64__
+#define REX_PREFIX	"0x48, "
+#else
+#define REX_PREFIX
+#endif
+
+#define XSAVE		".byte " REX_PREFIX "0x0f,0xae,0x27"
+#define XRSTOR		".byte " REX_PREFIX "0x0f,0xae,0x2f"
+
+static inline uint64_t xgetbv(uint32_t index)
+{
+	uint32_t eax, edx;
+
+	asm volatile("xgetbv"
+		     : "=a" (eax), "=d" (edx)
+		     : "c" (index));
+	return eax + ((uint64_t)edx << 32);
+}
+
+static inline void xsave(struct xsave_buffer *xbuf, uint64_t rfbm)
+{
+	uint32_t rfbm_lo = rfbm;
+	uint32_t rfbm_hi = rfbm >> 32;
+
+	asm volatile(XSAVE :: "D" (xbuf), "a" (rfbm_lo), "d" (rfbm_hi) : "memory");
+}
+
+static inline void xrstor(struct xsave_buffer *xbuf, uint64_t rfbm)
+{
+	uint32_t rfbm_lo = rfbm;
+	uint32_t rfbm_hi = rfbm >> 32;
+
+	asm volatile(XRSTOR :: "D" (xbuf), "a" (rfbm_lo), "d" (rfbm_hi));
+}
+
+static inline void clear_xstate_header(struct xsave_buffer *xbuf)
+{
+	memset(&xbuf->header, 0, sizeof(xbuf->header));
+}
+
+static inline uint32_t get_mxcsr(struct xsave_buffer *xbuf)
+{
+	return *((uint32_t *)xbuf->mxcsr);
+}
+
+static inline void set_mxcsr(struct xsave_buffer *xbuf, uint32_t val)
+{
+	*((uint32_t *)xbuf->mxcsr) = val;
+}
+
+#define XFEATURE_MASK_SSE		0x2
+#define XFEATURE_MASK_YMM		0x4
+
+#define CPUID_LEAF1_ECX_XSAVE_MASK	(1 << 26)
+#define CPUID_LEAF1_ECX_OSXSAVE_MASK	(1 << 27)
+#define CPUID_LEAF_XSTATE		0xd
+#define CPUID_SUBLEAF_XSTATE_USER	0x0
+#define CPUID_SUBLEAF_XSTATE_EXT	0x1
+
+static bool xsave_availability(void)
+{
+	uint32_t eax, ebx, ecx, edx;
+
+	__cpuid_count(1, 0, eax, ebx, ecx, edx);
+	if (!(ecx & CPUID_LEAF1_ECX_XSAVE_MASK))
+		return false;
+	if (!(ecx & CPUID_LEAF1_ECX_OSXSAVE_MASK))
+		return false;
+	return true;
+}
+
+static uint32_t get_xbuf_size(void)
+{
+	uint32_t eax, ebx, ecx, edx;
+
+	__cpuid_count(CPUID_LEAF_XSTATE, CPUID_SUBLEAF_XSTATE_USER,
+		      eax, ebx, ecx, edx);
+	return ebx;
+}
+
+static void ptrace_get(pid_t pid, struct iovec *iov)
+{
+	memset(iov->iov_base, 0, iov->iov_len);
+
+	if (ptrace(PTRACE_GETREGSET, pid, (uint32_t)NT_X86_XSTATE, iov))
+		err(1, "TRACE_GETREGSET");
+}
+
+static void ptrace_set(pid_t pid, struct iovec *iov)
+{
+	if (ptrace(PTRACE_SETREGSET, pid, (uint32_t)NT_X86_XSTATE, iov))
+		err(1, "TRACE_SETREGSET");
+}
+
+int main(void)
+{
+	struct xsave_buffer *xbuf;
+	uint32_t xbuf_size;
+	struct iovec iov;
+	uint32_t mxcsr;
+	pid_t child;
+	int status;
+
+	if (!xsave_availability())
+		printf("[SKIP]\tSkip as XSAVE not available.\n");
+
+	xbuf_size = get_xbuf_size();
+	if (!xbuf_size)
+		printf("[SKIP]\tSkip as XSAVE not available.\n");
+
+	if (!(xgetbv(0) & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM)))
+		printf("[SKIP]\tSkip as SSE state not available.\n");
+
+	xbuf = aligned_alloc(64, xbuf_size);
+	if (!xbuf)
+		err(1, "aligned_alloc()");
+
+	iov.iov_base = xbuf;
+	iov.iov_len = xbuf_size;
+
+	child = fork();
+	if (child < 0) {
+		err(1, "fork()");
+	} else if (!child) {
+		if (ptrace(PTRACE_TRACEME, 0, NULL, NULL))
+			err(1, "PTRACE_TRACEME");
+
+		raise(SIGTRAP);
+		_exit(0);
+	}
+
+	wait(&status);
+
+	if (WSTOPSIG(status) != SIGTRAP)
+		err(1, "raise(SIGTRAP)");
+
+	printf("[RUN]\tTest the MXCSR state write via ptrace().\n");
+
+	/* Set a benign value */
+	set_mxcsr(xbuf, 0xabc);
+	/* The MXCSR state should be loaded regardless of XSTATE_BV */
+	clear_xstate_header(xbuf);
+
+	/* Write the MXCSR state both locally and remotely. */
+	xrstor(xbuf, XFEATURE_MASK_SSE);
+	ptrace_set(child, &iov);
+
+	/* Read the MXCSR state back for both */
+	xsave(xbuf, XFEATURE_MASK_SSE);
+	mxcsr = get_mxcsr(xbuf);
+	ptrace_get(child, &iov);
+
+	/* Cross-check with each other */
+	if (mxcsr == get_mxcsr(xbuf))
+		printf("[OK]\tThe written state was read back correctly.\n");
+	else
+		printf("[FAIL]\tThe write (or read) was incorrect.\n");
+
+	ptrace(PTRACE_DETACH, child, NULL, NULL);
+	wait(&status);
+	if (!WIFEXITED(status) || WEXITSTATUS(status))
+		err(1, "PTRACE_DETACH");
+
+	free(xbuf);
+}
-- 
2.17.1

[PATCH v2 3/4] x86/fpu: Disallow legacy states from fpstate_clear_xstate_component()

[Chang S. Bae]

Commit 087df48c298c ("x86/fpu: Replace KVMs xstate component clearing")
refactored the MPX state clearing code.

But, legacy states are not warranted in this routine:
- It presumes every state is contiguous but that's not true for the legacy
  states. While MXCSR belongs to SSE, the state is located in the XSAVE
  buffer as surrounded by FP states.
- Also, zeroing out legacy states is not meaningful as their init state is
  non-zero.

It is possible to adjust the code to support them. Then, there is no use
for clearing legacy states yet. To make it simple, explicitly disallow
legacy states.

Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
Changes from v1 (Sean Christopherson):
* Revert the name change.
* Add a warning.
* Update title/changelog.
---
 arch/x86/kernel/fpu/xstate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d7676cfc32eb..a3f7045d1f8e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1375,6 +1375,15 @@ void fpstate_clear_xstate_component(struct fpstate *fps, unsigned int xfeature)
 {
 	void *addr = get_xsave_addr(&fps->regs.xsave, xfeature);
 
+	/*
+	 * Allow extended states only, because:
+	 * (1) Each legacy state is not contiguously located in the buffer.
+	 * (2) Zeroing those states is not meaningful as their init states
+	 *     are not zero.
+	 */
+	if (WARN_ON_ONCE(xfeature <= XFEATURE_SSE))
+		return;
+
 	if (addr)
 		memset(addr, 0, xstate_sizes[xfeature]);
 }
-- 
2.17.1

[PATCH v2 4/4] x86/fpu: Correct the legacy state offset and size information

[Chang S. Bae]

MXCSR is architecturally part of the SSE state. But, the kernel code
presumes it as part of the FP component. Adjust the offset and size for
these legacy states.

Notably, each legacy component area is not contiguous, unlike extended
components. Add a warning message when these offset and size are
referenced.

Fixes: ac73b27aea4e ("x86/fpu/xstate: Fix xstate_offsets, xstate_sizes for non-extended xstates")
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/fpu/xstate.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index a3f7045d1f8e..ac2ec5d6e7e4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
 	 * offsets.
 	 */
 	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
-	    xfeature <= XFEATURE_SSE)
+	    xfeature <= XFEATURE_SSE) {
+		if (xfeature <= XFEATURE_SSE)
+			pr_warn("The legacy state (%d) is discontiguously located.\n",
+				xfeature);
+
 		return xstate_offsets[xfeature];
+	}
 
 	/*
 	 * Compacted format offsets depend on the actual content of the
@@ -217,14 +222,18 @@ static void __init setup_xstate_cache(void)
 	 * The FP xstates and SSE xstates are legacy states. They are always
 	 * in the fixed offsets in the xsave area in either compacted form
 	 * or standard form.
+	 *
+	 * But, while MXCSR is part of the SSE state, it is located in
+	 * between the FP states. Note that it is erroneous assuming that
+	 * each legacy area is contiguous.
 	 */
 	xstate_offsets[XFEATURE_FP]	= 0;
-	xstate_sizes[XFEATURE_FP]	= offsetof(struct fxregs_state,
-						   xmm_space);
+	xstate_sizes[XFEATURE_FP]	= offsetof(struct fxregs_state, mxcsr) +
+					  sizeof_field(struct fxregs_state, st_space);
 
-	xstate_offsets[XFEATURE_SSE]	= xstate_sizes[XFEATURE_FP];
-	xstate_sizes[XFEATURE_SSE]	= sizeof_field(struct fxregs_state,
-						       xmm_space);
+	xstate_offsets[XFEATURE_SSE]	= offsetof(struct fxregs_state, mxcsr);
+	xstate_sizes[XFEATURE_SSE]	= MXCSR_AND_FLAGS_SIZE +
+					  sizeof_field(struct fxregs_state, xmm_space);
 
 	for_each_extended_xfeature(i, fpu_kernel_cfg.max_features) {
 		cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
-- 
2.17.1

Re: [PATCH v2 4/4] x86/fpu: Correct the legacy state offset and size information

[Sean Christopherson]

On Thu, Sep 22, 2022, Chang S. Bae wrote:
> MXCSR is architecturally part of the SSE state. But, the kernel code
> presumes it as part of the FP component. Adjust the offset and size for
> these legacy states.
> 
> Notably, each legacy component area is not contiguous, unlike extended
> components. Add a warning message when these offset and size are
> referenced.
> 
> Fixes: ac73b27aea4e ("x86/fpu/xstate: Fix xstate_offsets, xstate_sizes for non-extended xstates")
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  arch/x86/kernel/fpu/xstate.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index a3f7045d1f8e..ac2ec5d6e7e4 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
>  	 * offsets.
>  	 */
>  	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
> -	    xfeature <= XFEATURE_SSE)
> +	    xfeature <= XFEATURE_SSE) {
> +		if (xfeature <= XFEATURE_SSE)
> +			pr_warn("The legacy state (%d) is discontiguously located.\n",
> +				xfeature);

pr_warn() here isn't warranted.  copy_uabi_to_xstate() calls this with non-extended
features, which is perfectly fine since it manually handles MXCSR.  And that helper
is directly reachable by userspace, i.e. userspace can spam the pr_warn().

Re: [PATCH v2 4/4] x86/fpu: Correct the legacy state offset and size information

[Chang S. Bae]

On 9/28/2022 2:06 PM, Sean Christopherson wrote:
> On Thu, Sep 22, 2022, Chang S. Bae wrote:
>>
>> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
>> index a3f7045d1f8e..ac2ec5d6e7e4 100644
>> --- a/arch/x86/kernel/fpu/xstate.c
>> +++ b/arch/x86/kernel/fpu/xstate.c
>> @@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
>>   	 * offsets.
>>   	 */
>>   	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
>> -	    xfeature <= XFEATURE_SSE)
>> +	    xfeature <= XFEATURE_SSE) {
>> +		if (xfeature <= XFEATURE_SSE)
>> +			pr_warn("The legacy state (%d) is discontiguously located.\n",
>> +				xfeature);
> 
> pr_warn() here isn't warranted.  copy_uabi_to_xstate() calls this with non-extended
> features, 

I think patch1 makes changes not to call this for legacy features anymore.

> which is perfectly fine since it manually handles MXCSR.  And that helper
> is directly reachable by userspace, i.e. userspace can spam the pr_warn().

I don't think I get your point. I assume that helper is 
__raw_xsave_addr(). But then I'm missing how it can be directly reached 
by userspace.

Thanks,
Chang

Re: [PATCH v2 4/4] x86/fpu: Correct the legacy state offset and size information

[Sean Christopherson]

On Wed, Sep 28, 2022, Chang S. Bae wrote:
> On 9/28/2022 2:06 PM, Sean Christopherson wrote:
> > On Thu, Sep 22, 2022, Chang S. Bae wrote:
> > > 
> > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > > index a3f7045d1f8e..ac2ec5d6e7e4 100644
> > > --- a/arch/x86/kernel/fpu/xstate.c
> > > +++ b/arch/x86/kernel/fpu/xstate.c
> > > @@ -143,8 +143,13 @@ static unsigned int xfeature_get_offset(u64 xcomp_bv, int xfeature)
> > >   	 * offsets.
> > >   	 */
> > >   	if (!cpu_feature_enabled(X86_FEATURE_XCOMPACTED) ||
> > > -	    xfeature <= XFEATURE_SSE)
> > > +	    xfeature <= XFEATURE_SSE) {
> > > +		if (xfeature <= XFEATURE_SSE)
> > > +			pr_warn("The legacy state (%d) is discontiguously located.\n",
> > > +				xfeature);
> > 
> > pr_warn() here isn't warranted.  copy_uabi_to_xstate() calls this with non-extended
> > features,
> 
> I think patch1 makes changes not to call this for legacy features anymore.

Oh, even better!  In that case, drop patch 3 and WARN here, because with that
call site gone, all paths that lead to xfeature_get_offset() avoid calling it
with legacy features.

The comment above this probably needs to be updated too.

I was going to make that suggestion in my original response, but I didn't apply
the series and so didn't see that that last remaining wart was fixed.  Thanks,
and sorry for the runaround!

> > which is perfectly fine since it manually handles MXCSR.  And that helper
> > is directly reachable by userspace, i.e. userspace can spam the pr_warn().
> 
> I don't think I get your point. I assume that helper is __raw_xsave_addr().
> But then I'm missing how it can be directly reached by userspace.

ioctl(KVM_GET_XSAVE) can reach this code, i.e. if there were a bug then userspace
could invoke that ioctl() in a tight loop to spam the kernel log.