[PATCH v2 0/4] x86/fpu: Restore and reinforce signal frame portability

[PATCH v2 0/4] x86/fpu: Restore and reinforce signal frame portability

[Andrei Vagin]

The x86 signal frame is designed to be self-describing. The xstate_size
field in the software-reserved bytes indicates the actual size of the
xstate context and is used by the kernel to locate the FP_XSTATE_MAGIC2
marker during signal return.

This design is required to provide portability of signal frames across
different machines. For example, a process checkpointed on a system with
fewer xstate features and restored on a system with more features will
have a signal frame on its stack that is smaller than the destination
host's default. By relying on the frame's internal xstate_size, the
kernel can correctly validate and restore such frames.

This series addresses a regression introduced in commit dc8aa31a7ac2
("x86/fpu: Refine and simplify the magic number check during signal
return").

v2: Address sashiko comments.
    44eeff9bc467 ("Revert "x86/fpu: Refine and simplify the magic number
    check during signal return"") has been merged.

Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Chang S. Bae" <chang.seok.bae@intel.com>

Andrei Vagin (5):
  Revert "x86/fpu: Refine and simplify the magic number check during
    signal return"
  x86/fpu: Document signal frame portability
  selftests/x86: Add a test for signal frame portability
  x86/fpu: Add consistency check between xstate_size and xfeatures
  selftests/x86: Add a consistency test for signal frames

 Documentation/arch/x86/xstate.rst             |  14 +-
 arch/x86/include/uapi/asm/sigcontext.h        |  10 +
 arch/x86/kernel/fpu/signal.c                  |  29 ++-
 arch/x86/kernel/fpu/xstate.c                  |   2 +-
 arch/x86/kernel/fpu/xstate.h                  |   2 +
 tools/testing/selftests/x86/Makefile          |   5 +-
 .../selftests/x86/sigframe_portability.c      | 205 ++++++++++++++++++
 tools/testing/selftests/x86/xstate.c          |   5 -
 tools/testing/selftests/x86/xstate.h          |  12 +
 9 files changed, 269 insertions(+), 15 deletions(-)
 create mode 100644 tools/testing/selftests/x86/sigframe_portability.c

-- 
2.54.0.746.g67dd491aae-goog

[PATCH 1/4] x86/fpu: Document signal frame portability

[Andrei Vagin]

The x86 signal frame is designed to be self-describing, with the
'xstate_size' field in the software-reserved bytes indicating the actual
size of the context. This design is required for portability, allowing a
signal frame created on a system with a specific set of xstate features
to be restored on a machine with a different (larger) set of features.

Document this contract in the uabi headers and Documentation/. This
requirement is critical for checkpoint/restore tools like CRIU, which
should be able to migrate processes across machines with heterogeneous
FPU capabilities. Note that portability is generally limited to CPUs
from the same vendor (e.g., Intel vs. AMD) due to potential differences
in xstate layouts.

Signed-off-by: Andrei Vagin <avagin@google.com>
---
 Documentation/arch/x86/xstate.rst      | 14 ++++++++++++--
 arch/x86/include/uapi/asm/sigcontext.h | 10 ++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/arch/x86/xstate.rst b/Documentation/arch/x86/xstate.rst
index cec05ac464c1..e27e779bae44 100644
--- a/Documentation/arch/x86/xstate.rst
+++ b/Documentation/arch/x86/xstate.rst
@@ -170,5 +170,15 @@ are extended to control the guest permission:
  is going to be rejected. So, the permission has to be requested before the
  first VCPU creation.
 
-Note that some VMMs may have already established a set of supported state
-components. These options are not presumed to support any particular VMM.
+Signal Frame Portability
+------------------------
+
+The signal frame is designed to be self-describing and portable. This is
+especially important for checkpoint/restore tools like CRIU, which may restore
+a process on a different host than where it was checkpointed. A signal frame
+created on a machine with fewer CPU features can be successfully restored on a
+machine with more CPU features.
+
+Note that signal frame portability is generally guaranteed only between CPUs
+from the same vendor. Different vendors may use different offsets for the same
+xstate features in the xsave area, making frames incompatible between them.
diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
index d0d9b331d3a1..3ea63c29c3d7 100644
--- a/arch/x86/include/uapi/asm/sigcontext.h
+++ b/arch/x86/include/uapi/asm/sigcontext.h
@@ -34,6 +34,16 @@
  * fpstate+extended_size-FP_XSTATE_MAGIC2_SIZE address) is set to
  * FP_XSTATE_MAGIC2 so that you can sanity check your size calculations.)
  *
+ * The xstate_size field indicates the actual size of the xstate context
+ * (including the fxregs_state and xstate_header). This size is used in
+ * conjunction with the fpstate pointer to locate FP_XSTATE_MAGIC2.  This makes
+ * the signal frame self-describing and portable: a signal frame created on a
+ * machine with a certain set of xstate features can be restored on a machine
+ * with a different (larger) set of features, as long as the latter supports
+ * all features present in the frame. Note that this portability is generally
+ * limited to CPUs of the same vendor, as different vendors may use different
+ * xstate layouts.
+ *
  * This extended area typically grows with newer CPUs that have larger and
  * larger XSAVE areas.
  */
-- 
2.54.0.1032.g2f8565e1d1-goog

Re: [PATCH 1/4] x86/fpu: Document signal frame portability

[Andrei Vagin]

On Thu, Jun 4, 2026 at 10:04 AM Andrei Vagin <avagin@google.com> wrote:
>
> The x86 signal frame is designed to be self-describing, with the
> 'xstate_size' field in the software-reserved bytes indicating the actual
> size of the context. This design is required for portability, allowing a
> signal frame created on a system with a specific set of xstate features
> to be restored on a machine with a different (larger) set of features.
>
> Document this contract in the uabi headers and Documentation/. This
> requirement is critical for checkpoint/restore tools like CRIU, which
> should be able to migrate processes across machines with heterogeneous
> FPU capabilities. Note that portability is generally limited to CPUs
> from the same vendor (e.g., Intel vs. AMD) due to potential differences
> in xstate layouts.
>
> Signed-off-by: Andrei Vagin <avagin@google.com>
> ---
>  Documentation/arch/x86/xstate.rst      | 14 ++++++++++++--
>  arch/x86/include/uapi/asm/sigcontext.h | 10 ++++++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/arch/x86/xstate.rst b/Documentation/arch/x86/xstate.rst
> index cec05ac464c1..e27e779bae44 100644
> --- a/Documentation/arch/x86/xstate.rst
> +++ b/Documentation/arch/x86/xstate.rst
> @@ -170,5 +170,15 @@ are extended to control the guest permission:
>   is going to be rejected. So, the permission has to be requested before the
>   first VCPU creation.
>
> -Note that some VMMs may have already established a set of supported state
> -components. These options are not presumed to support any particular VMM.

My bad, I accidentally deleted those lines. Please ignore that and review
the other changes.

>
> +Signal Frame Portability
> +------------------------
> +
> +The signal frame is designed to be self-describing and portable. This is
> +especially important for checkpoint/restore tools like CRIU, which may restore
> +a process on a different host than where it was checkpointed. A signal frame
> +created on a machine with fewer CPU features can be successfully restored on a
> +machine with more CPU features.
> +
> +Note that signal frame portability is generally guaranteed only between CPUs
> +from the same vendor. Different vendors may use different offsets for the same
> +xstate features in the xsave area, making frames incompatible between them.
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index d0d9b331d3a1..3ea63c29c3d7 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -34,6 +34,16 @@
>   * fpstate+extended_size-FP_XSTATE_MAGIC2_SIZE address) is set to
>   * FP_XSTATE_MAGIC2 so that you can sanity check your size calculations.)
>   *
> + * The xstate_size field indicates the actual size of the xstate context
> + * (including the fxregs_state and xstate_header). This size is used in
> + * conjunction with the fpstate pointer to locate FP_XSTATE_MAGIC2.  This makes
> + * the signal frame self-describing and portable: a signal frame created on a
> + * machine with a certain set of xstate features can be restored on a machine
> + * with a different (larger) set of features, as long as the latter supports
> + * all features present in the frame. Note that this portability is generally
> + * limited to CPUs of the same vendor, as different vendors may use different
> + * xstate layouts.
> + *
>   * This extended area typically grows with newer CPUs that have larger and
>   * larger XSAVE areas.
>   */
> --
> 2.54.0.1032.g2f8565e1d1-goog
>
>

[PATCH 2/4] selftests/x86: Add a test for signal frame portability

[Andrei Vagin]

Add a new selftest tools/testing/selftests/x86/sigframe_portability.c
that verifies that the kernel correctly restores the xstate context even
if the frame size has been manually reduced, as long as the
FP_XSTATE_MAGIC2 marker is correctly placed at the end of the specified
xstate_size.

This test simulates a scenario where a signal frame is created on a
system with fewer xstate features and restored on a system with more
features.

Signed-off-by: Andrei Vagin <avagin@google.com>
---
 tools/testing/selftests/x86/Makefile          |   5 +-
 .../selftests/x86/sigframe_portability.c      | 156 ++++++++++++++++++
 tools/testing/selftests/x86/xstate.c          |   5 -
 tools/testing/selftests/x86/xstate.h          |  12 ++
 4 files changed, 172 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/x86/sigframe_portability.c

diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
index 434065215d12..bd59ee3df61d 100644
--- a/tools/testing/selftests/x86/Makefile
+++ b/tools/testing/selftests/x86/Makefile
@@ -19,7 +19,8 @@ TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
 			test_FCMOV test_FCOMI test_FISTTP \
 			vdso_restorer
 TARGETS_C_64BIT_ONLY := fsgsbase sysret_rip syscall_numbering \
-			corrupt_xstate_header amx lam test_shadow_stack avx apx
+			corrupt_xstate_header amx lam test_shadow_stack avx apx \
+			sigframe_portability
 # Some selftests require 32bit support enabled also on 64bit systems
 TARGETS_C_32BIT_NEEDED := ldt_gdt ptrace_syscall
 
@@ -138,3 +139,5 @@ $(OUTPUT)/avx_64: CFLAGS += -mno-avx -mno-avx512f
 $(OUTPUT)/amx_64: EXTRA_FILES += xstate.c
 $(OUTPUT)/avx_64: EXTRA_FILES += xstate.c
 $(OUTPUT)/apx_64: EXTRA_FILES += xstate.c
+
+$(OUTPUT)/sigframe_portability_64: CFLAGS += -mno-avx -mno-avx512f
diff --git a/tools/testing/selftests/x86/sigframe_portability.c b/tools/testing/selftests/x86/sigframe_portability.c
new file mode 100644
index 000000000000..54ba182a792f
--- /dev/null
+++ b/tools/testing/selftests/x86/sigframe_portability.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <signal.h>
+#include <string.h>
+#include <sys/ucontext.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <cpuid.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <asm/prctl.h>
+#include <stddef.h>
+
+#include "helpers.h"
+#include "xstate.h"
+
+/*
+ * This test verifies the portability of the signal frame.
+ * It simulates a scenario where a signal frame is created on a system with
+ * fewer xstate features and restored on a system with more features.
+ */
+
+#define SIGFRAME_XSTATE_HDR_OFFSET	512
+
+#define XSTATE_SSE_ONLY_SIZE	(SIGFRAME_XSTATE_HDR_OFFSET + XSAVE_HDR_SIZE)
+#define XFEATURE_MASK_FPSSE	((1 << XFEATURE_FP) | (1 << XFEATURE_SSE))
+
+static uint32_t ymm_offset;
+static uint32_t xstate_size_ymm;
+
+/*
+ * Avoid using printf() in signal handlers as it is not
+ * async-signal-safe.
+ */
+#define SIGNAL_BUF_LEN 1024
+static char sig_err_buf[SIGNAL_BUF_LEN];
+
+static void sig_print(const char *msg)
+{
+	int left = SIGNAL_BUF_LEN - strlen(sig_err_buf) - 1;
+
+	strncat(sig_err_buf, msg, left);
+}
+
+static void check_avx_support(void)
+{
+	struct xstate_info xstate;
+	unsigned long features;
+	long rc;
+
+	/*
+	 * Check if the kernel supports AVX (XFEATURE_YMM).
+	 * This also confirms that the OS has enabled XSAVE.
+	 */
+	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_SUPP, &features);
+	if (rc != 0)
+		ksft_exit_skip("ARCH_GET_XCOMP_SUPP not supported\n");
+
+	if (!(features & (1 << XFEATURE_YMM)))
+		ksft_exit_skip("AVX not supported by kernel/hardware\n");
+
+	xstate = get_xstate_info(XFEATURE_YMM);
+	if (!xstate.size)
+		ksft_exit_skip("AVX not supported by hardware\n");
+
+	ymm_offset = xstate.xbuf_offset;
+	xstate_size_ymm = xstate.xbuf_offset + xstate.size;
+}
+
+#define TEST_YMMH_VAL (0x5656565656565656UL)
+
+__attribute__((target("avx")))
+static void read_ymm0(uint64_t *v)
+{
+	asm volatile ("vmovdqu %%ymm0, %0" : "=m"  (*(char (*)[32])v));
+}
+
+__attribute__((target("avx")))
+static void write_ymm0(uint64_t *v)
+{
+	asm volatile ("vmovdqu %0, %%ymm0" : : "m"  (*(char (*)[32])v));
+}
+
+
+static void handle_signal(int sig, siginfo_t *si, void *ucp)
+{
+	ucontext_t *uc = ucp;
+	void *fp = uc->uc_mcontext.fpregs;
+	struct _fpx_sw_bytes *sw = get_fpx_sw_bytes(fp);
+	struct xsave_buffer *xbuf;
+	uint64_t xfeatures, *ymmh_p;
+
+	if (sw->magic1 != FP_XSTATE_MAGIC1) {
+		sig_print("magic1 is not valid\n");
+		return;
+	}
+
+	xbuf = (struct xsave_buffer *)fp;
+
+	/* Shrink the frame to just YMM size */
+	sw->xstate_size = xstate_size_ymm;
+
+	xfeatures = get_xstatebv(xbuf);
+	xfeatures &= XFEATURE_MASK_FPSSE | (1 << XFEATURE_YMM);
+	set_xstatebv(xbuf, xfeatures);
+	/* Also update sw->xfeatures as the kernel relies on it */
+	set_fpx_sw_bytes_features(fp, xfeatures);
+
+	*(uint32_t *)(fp + sw->xstate_size) = FP_XSTATE_MAGIC2;
+
+	ymmh_p = (uint64_t *)(fp + ymm_offset);
+	ymmh_p[0] = TEST_YMMH_VAL;
+	ymmh_p[1] = TEST_YMMH_VAL+1;
+
+	/* clear everything after MAGIC2. */
+	if (sw->xstate_size + 4 < sw->extended_size)
+		memset(fp + sw->xstate_size + 4, 0, sw->extended_size - sw->xstate_size - 4);
+}
+
+int main(void)
+{
+	uint64_t v[4] = {0, 0, 0, 0};
+
+	ksft_print_header();
+	ksft_set_plan(1);
+
+	check_avx_support();
+
+	sethandler(SIGUSR1, handle_signal, 0);
+
+	v[0] = 0x1111111111111111ULL;
+	v[1] = 0x2222222222222222ULL;
+	v[2] = 0x3333333333333333ULL;
+	v[3] = 0x4444444444444444ULL;
+	write_ymm0(v);
+
+	raise(SIGUSR1);
+	v[0] = v[1] = v[2] = v[3] = 0;
+	read_ymm0(v);
+
+	if (sig_err_buf[0])
+		ksft_test_result_fail("%s\n", sig_err_buf);
+	else if (v[2] == TEST_YMMH_VAL && v[3] == (TEST_YMMH_VAL + 1))
+		ksft_test_result_pass("YMM state restored correctly\n");
+	else
+		ksft_test_result_fail(
+				"Got upper bits: 0x%lx 0x%lx (expected %lx %lx)\n",
+			       v[2], v[3], TEST_YMMH_VAL, TEST_YMMH_VAL + 1);
+
+	clearhandler(SIGUSR1);
+
+	ksft_finished();
+	return 0;
+}
diff --git a/tools/testing/selftests/x86/xstate.c b/tools/testing/selftests/x86/xstate.c
index 97fe4bd8bc77..40062b28c001 100644
--- a/tools/testing/selftests/x86/xstate.c
+++ b/tools/testing/selftests/x86/xstate.c
@@ -42,11 +42,6 @@ static inline uint64_t xgetbv(uint32_t index)
 	return eax + ((uint64_t)edx << 32);
 }
 
-static inline uint64_t get_xstatebv(struct xsave_buffer *xbuf)
-{
-	return *(uint64_t *)(&xbuf->header);
-}
-
 static struct xstate_info xstate;
 
 struct futex_info {
diff --git a/tools/testing/selftests/x86/xstate.h b/tools/testing/selftests/x86/xstate.h
index 6ee816e7625a..c531667b66ad 100644
--- a/tools/testing/selftests/x86/xstate.h
+++ b/tools/testing/selftests/x86/xstate.h
@@ -3,6 +3,8 @@
 #define __SELFTESTS_X86_XSTATE_H
 
 #include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include "kselftest.h"
 
@@ -160,6 +162,11 @@ static inline void set_xstatebv(struct xsave_buffer *xbuf, uint64_t bv)
 	*(uint64_t *)(&xbuf->header) = bv;
 }
 
+static inline uint64_t get_xstatebv(struct xsave_buffer *xbuf)
+{
+	return *(uint64_t *)(&xbuf->header);
+}
+
 /* See 'struct _fpx_sw_bytes' at sigcontext.h */
 #define SW_BYTES_OFFSET		464
 /* N.B. The struct's field name varies so read from the offset. */
@@ -175,6 +182,11 @@ static inline uint64_t get_fpx_sw_bytes_features(void *buffer)
 	return *(uint64_t *)(buffer + SW_BYTES_BV_OFFSET);
 }
 
+static inline void set_fpx_sw_bytes_features(void *buffer, uint64_t features)
+{
+	*(uint64_t *)(buffer + SW_BYTES_BV_OFFSET) = features;
+}
+
 static inline void set_rand_data(struct xstate_info *xstate, struct xsave_buffer *xbuf)
 {
 	int *ptr = (int *)&xbuf->bytes[xstate->xbuf_offset];
-- 
2.54.0.1032.g2f8565e1d1-goog

[PATCH 3/4] x86/fpu: Add consistency check between xstate_size and xfeatures

[Andrei Vagin]

The signal frame is designed to be self-describing, where xstate_size
indicates the actual size of the xstate context. The kernel previously
lacked a check to ensure that the provided xstate_size was sufficient
for the features enabled in the xfeatures mask. Additionally,
restore_fpregs_from_user() always used the default xstate_size to fault
in the xstate user buffer.

These consistency checks have been added:
* Validate that xfeatures is a subset of the features enabled for the
  task.
* Calculate the required size for the validated xfeatures mask.
* Ensure the provided xstate_size is sufficient.

These checks prevent the kernel from attempting to fault in memory past
the end of a frame.

Signed-off-by: Andrei Vagin <avagin@google.com>
---
 arch/x86/kernel/fpu/signal.c | 29 +++++++++++++++++++++++------
 arch/x86/kernel/fpu/xstate.c |  2 +-
 arch/x86/kernel/fpu/xstate.h |  2 ++
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 20b638c507ca..4ddfa59a2851 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -29,16 +29,21 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
 {
 	int min_xstate_size = sizeof(struct fxregs_state) +
 			      sizeof(struct xstate_header);
-	void __user *fpstate = fxbuf;
+	struct fpstate *fpstate = x86_task_fpu(current)->fpstate;
+	void __user *sig_fpstate = fxbuf;
 	unsigned int magic2;
 
 	if (__copy_from_user(fx_sw, &fxbuf->sw_reserved[0], sizeof(*fx_sw)))
 		return false;
 
+	/* Enforce XFEATURE_MASK_FPSSE when XSAVE is enabled */
+	fx_sw->xfeatures |= XFEATURE_MASK_FPSSE;
+
 	/* Check for the first magic field and other error scenarios. */
 	if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
+	    (fx_sw->xfeatures & ~fpstate->user_xfeatures) ||
 	    fx_sw->xstate_size < min_xstate_size ||
-	    fx_sw->xstate_size > x86_task_fpu(current)->fpstate->user_size ||
+	    fx_sw->xstate_size > fpstate->user_size ||
 	    fx_sw->xstate_size > fx_sw->extended_size)
 		goto setfx;
 
@@ -48,9 +53,17 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
 	 * fpstate layout with out copying the extended state information
 	 * in the memory layout.
 	 */
-	if (__get_user(magic2, (__u32 __user *)(fpstate + fx_sw->xstate_size)))
+	if (__get_user(magic2, (__u32 __user *)(sig_fpstate + fx_sw->xstate_size)))
 		return false;
 
+	if (fx_sw->xstate_size != fpstate->user_size) {
+		unsigned int calculated_size;
+
+		calculated_size = xstate_calculate_size(fx_sw->xfeatures, false);
+		if (fx_sw->xstate_size < calculated_size)
+			goto setfx;
+	}
+
 	if (likely(magic2 == FP_XSTATE_MAGIC2))
 		return true;
 setfx:
@@ -266,7 +279,8 @@ static int __restore_fpregs_from_user(void __user *buf, u64 ufeatures,
  * Attempt to restore the FPU registers directly from user memory.
  * Pagefaults are handled and any errors returned are fatal.
  */
-static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_only)
+static bool restore_fpregs_from_user(void __user *buf, u64 xrestore,
+				     bool fx_only, size_t xstate_size)
 {
 	struct fpu *fpu = x86_task_fpu(current);
 	int ret;
@@ -302,7 +316,7 @@ static bool restore_fpregs_from_user(void __user *buf, u64 xrestore, bool fx_onl
 		if (ret != X86_TRAP_PF)
 			return false;
 
-		if (!fault_in_readable(buf, fpu->fpstate->user_size))
+		if (!fault_in_readable(buf, xstate_size))
 			goto retry;
 		return false;
 	}
@@ -333,6 +347,7 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
 	bool success, fx_only = false;
 	union fpregs_state *fpregs;
 	u64 user_xfeatures = 0;
+	size_t xstate_size;
 
 	if (use_xsave()) {
 		struct _fpx_sw_bytes fx_sw_user;
@@ -342,13 +357,15 @@ static bool __fpu_restore_sig(void __user *buf, void __user *buf_fx,
 
 		fx_only = !fx_sw_user.magic1;
 		user_xfeatures = fx_sw_user.xfeatures;
+		xstate_size = fx_sw_user.xstate_size;
 	} else {
 		user_xfeatures = XFEATURE_MASK_FPSSE;
+		xstate_size = sizeof(struct fxregs_state);
 	}
 
 	if (likely(!ia32_fxstate)) {
 		/* Restore the FPU registers directly from user memory. */
-		return restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only);
+		return restore_fpregs_from_user(buf_fx, user_xfeatures, fx_only, xstate_size);
 	}
 
 	/*
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index a7b6524a9dea..371a3a4a4b4e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -587,7 +587,7 @@ static bool __init check_xstate_against_struct(int nr)
 	return true;
 }
 
-static unsigned int xstate_calculate_size(u64 xfeatures, bool compacted)
+unsigned int xstate_calculate_size(u64 xfeatures, bool compacted)
 {
 	unsigned int topmost = fls64(xfeatures) -  1;
 	unsigned int offset, i;
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 38a2862f09d3..c73cf2444de6 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -55,6 +55,8 @@ extern int copy_sigframe_from_user_to_xstate(struct task_struct *tsk, const void
 extern void fpu__init_cpu_xstate(void);
 extern void fpu__init_system_xstate(unsigned int legacy_size);
 
+extern unsigned int xstate_calculate_size(u64 xfeatures, bool compacted);
+
 extern void __user *get_xsave_addr_user(struct xregs_state __user *xsave, int xfeature_nr);
 
 static inline u64 xfeatures_mask_supervisor(void)
-- 
2.54.0.1032.g2f8565e1d1-goog

Re: [PATCH 3/4] x86/fpu: Add consistency check between xstate_size and xfeatures

[Ingo Molnar]

* Andrei Vagin <avagin@google.com> wrote:

> The signal frame is designed to be self-describing, where xstate_size
> indicates the actual size of the xstate context. The kernel previously
> lacked a check to ensure that the provided xstate_size was sufficient
> for the features enabled in the xfeatures mask. Additionally,
> restore_fpregs_from_user() always used the default xstate_size to fault
> in the xstate user buffer.
> 
> These consistency checks have been added:
> * Validate that xfeatures is a subset of the features enabled for the
>   task.
> * Calculate the required size for the validated xfeatures mask.
> * Ensure the provided xstate_size is sufficient.
> 
> These checks prevent the kernel from attempting to fault in memory past
> the end of a frame.
> 
> Signed-off-by: Andrei Vagin <avagin@google.com>
> ---
>  arch/x86/kernel/fpu/signal.c | 29 +++++++++++++++++++++++------
>  arch/x86/kernel/fpu/xstate.c |  2 +-
>  arch/x86/kernel/fpu/xstate.h |  2 ++
>  3 files changed, 26 insertions(+), 7 deletions(-)

So since this is a potentially invasive change, could you please split
it up into further incremental steps, with the behavioral changes at
the end:

  x86/fpu: Export xstate_calculate_size() internally
  x86/fpu: Extend restore_fpregs_from_user() with 'xstate_size'
  x86/fpu: Rename 'fpstate' to 'sig_fpstate' in check_xstate_in_sigframe()
  x86/fpu: Introduce 'fpstate' helper variable in check_xstate_in_sigframe()
  etc.

To make it all more reviewable & bisectable.

Also, could the check_xstate_in_sigframe() function get some TLC before
we modify it materially:

  1)

  arch/x86/kernel/fpu/signal.c:static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
  arch/x86/kernel/fpu/signal.c:           if (!check_xstate_in_sigframe(buf_fx, &fx_sw_user))

  Please harmonize the argument names: why is it 'buf_fx' in one
  function and 'fxbuf' in another? We should probably standardize
  on 'buf_fx' everywhere.

  2)

  Why is an error condition label called 'setfx'? How about
  'error_setfx' or so. Makes patches more straightforward to read.

etc., I'm sure there's more.

Plus regarding the behavioral changes, this one should probably
be its own patch as well:

+       /* Enforce XFEATURE_MASK_FPSSE when XSAVE is enabled */
+       fx_sw->xfeatures |= XFEATURE_MASK_FPSSE;

With the new size checks in another patch, right?

Thanks,

	Ingo

[PATCH 4/4] selftests/x86: Add a consistency test for signal frames

[Andrei Vagin]

Extend sigframe_portability test to include a consistency check.  Verify
that the kernel correctly rejects signal frames where the xstate_size is
too small for the enabled features in the xfeatures mask.

Signed-off-by: Andrei Vagin <avagin@google.com>
---
 .../selftests/x86/sigframe_portability.c      | 77 +++++++++++++++----
 1 file changed, 63 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/x86/sigframe_portability.c b/tools/testing/selftests/x86/sigframe_portability.c
index 54ba182a792f..04685a1c3aaa 100644
--- a/tools/testing/selftests/x86/sigframe_portability.c
+++ b/tools/testing/selftests/x86/sigframe_portability.c
@@ -17,13 +17,14 @@
 #include "xstate.h"
 
 /*
- * This test verifies the portability of the signal frame.
- * It simulates a scenario where a signal frame is created on a system with
- * fewer xstate features and restored on a system with more features.
+ * This test verifies the portability and consistency of the signal frame.
+ * Portability: A frame created on a system with fewer features can be
+ *              restored on a system with more features.
+ * Consistency: The kernel rejects frames where xstate_size is insufficient
+ *              for the features enabled in xfeatures.
  */
 
 #define SIGFRAME_XSTATE_HDR_OFFSET	512
-
 #define XSTATE_SSE_ONLY_SIZE	(SIGFRAME_XSTATE_HDR_OFFSET + XSAVE_HDR_SIZE)
 #define XFEATURE_MASK_FPSSE	((1 << XFEATURE_FP) | (1 << XFEATURE_SSE))
 
@@ -83,8 +84,7 @@ static void write_ymm0(uint64_t *v)
 	asm volatile ("vmovdqu %0, %%ymm0" : : "m"  (*(char (*)[32])v));
 }
 
-
-static void handle_signal(int sig, siginfo_t *si, void *ucp)
+static void handle_portability(int sig, siginfo_t *si, void *ucp)
 {
 	ucontext_t *uc = ucp;
 	void *fp = uc->uc_mcontext.fpregs;
@@ -119,16 +119,12 @@ static void handle_signal(int sig, siginfo_t *si, void *ucp)
 		memset(fp + sw->xstate_size + 4, 0, sw->extended_size - sw->xstate_size - 4);
 }
 
-int main(void)
+static void test_portability(void)
 {
 	uint64_t v[4] = {0, 0, 0, 0};
 
-	ksft_print_header();
-	ksft_set_plan(1);
-
-	check_avx_support();
-
-	sethandler(SIGUSR1, handle_signal, 0);
+	sig_err_buf[0] = 0;
+	sethandler(SIGUSR1, handle_portability, 0);
 
 	v[0] = 0x1111111111111111ULL;
 	v[1] = 0x2222222222222222ULL;
@@ -143,13 +139,66 @@ int main(void)
 	if (sig_err_buf[0])
 		ksft_test_result_fail("%s\n", sig_err_buf);
 	else if (v[2] == TEST_YMMH_VAL && v[3] == (TEST_YMMH_VAL + 1))
-		ksft_test_result_pass("YMM state restored correctly\n");
+		ksft_test_result_pass("YMM state restored correctly from shrunk frame\n");
 	else
 		ksft_test_result_fail(
 				"Got upper bits: 0x%lx 0x%lx (expected %lx %lx)\n",
 			       v[2], v[3], TEST_YMMH_VAL, TEST_YMMH_VAL + 1);
 
 	clearhandler(SIGUSR1);
+}
+
+static void handle_consistency(int sig, siginfo_t *si, void *ucp)
+{
+	ucontext_t *uc = ucp;
+	void *fp = uc->uc_mcontext.fpregs;
+	struct _fpx_sw_bytes *sw = get_fpx_sw_bytes(fp);
+
+	/* The origin frame contains an AVX state. */
+	sw->xstate_size = XSTATE_SSE_ONLY_SIZE;
+
+	*(uint32_t *)(fp + sw->xstate_size) = FP_XSTATE_MAGIC2;
+}
+
+static void test_consistency(void)
+{
+	uint64_t v[4] = {0, 0, 0, 0};
+
+	sig_err_buf[0] = 0;
+	sethandler(SIGUSR2, handle_consistency, 0);
+
+	v[0] = 0x1111111111111111ULL;
+	v[1] = 0x2222222222222222ULL;
+	v[2] = 0x3333333333333333ULL;
+	v[3] = 0x4444444444444444ULL;
+	write_ymm0(v);
+
+	raise(SIGUSR2);
+	v[0] = v[1] = v[2] = v[3] = 0;
+	read_ymm0(v);
+
+	/*
+	 * When inconsistent, the kernel should have fallen back to
+	 * FX-only mode, so YMM upper bits should be zero (init state).
+	 */
+	if (v[2] == 0 && v[3] == 0)
+		ksft_test_result_pass("Inconsistent size correctly rejected\n");
+	else
+		ksft_test_result_fail("Inconsistent size was NOT rejected: 0x%lx 0x%lx\n",
+				      v[2], v[3]);
+
+	clearhandler(SIGUSR2);
+}
+
+int main(void)
+{
+	ksft_print_header();
+	ksft_set_plan(2);
+
+	check_avx_support();
+
+	test_portability();
+	test_consistency();
 
 	ksft_finished();
 	return 0;
-- 
2.54.0.1032.g2f8565e1d1-goog