[PATCH v4 0/2] x86/fpu: Fix dynamic fpstate leak on exec()

[PATCH v4 0/2] x86/fpu: Fix dynamic fpstate leak on exec()

[Guixiong Wei]

An exec() after using an XFD-controlled xfeature leaks the dynamically
allocated fpstate. Fix the cleanup in fpstate_reset() and add an AMX
regression selftest.

The selftest reports FAIL on an unfixed kernel (10/10 FAILs on 7.1.3),
SKIP when vmallocinfo addresses are hidden, and OK with the fix applied.

Changes since v3:
- Close the use-after-free that freeing in fpstate_reset() exposed.
  fpu_flush_thread() called fpstate_reset() before
  fpu_reset_fpstate_regs() set TIF_NEED_FPU_LOAD, so a context switch
  or a kernel FPU user in that window could still see live register
  state and save it into the freed buffer. Set TIF_NEED_FPU_LOAD before
  the reset and document on fpstate_reset() that callers operating on
  the current task must prevent such a save. The race was caught by
  the Sashiko review and the fix was suggested by Chang S. Bae.

v3: https://lore.kernel.org/r/cover.1793072271.git.weiguixiong@bytedance.com
v2: https://lore.kernel.org/r/cover.1791428412.git.weiguixiong@bytedance.com
v1: https://lore.kernel.org/r/20260929151013.81562-2-weiguixiong@bytedance.com

Guixiong Wei (2):
  x86/fpu: Fix memory leak with dynamic fpstate and exec()
  selftests/x86/amx: Test dynamic fpstate cleanup across exec()

 arch/x86/kernel/fpu/core.c        |  12 ++
 tools/testing/selftests/x86/amx.c | 251 +++++++++++++++++++++++++++++-
 2 files changed, 261 insertions(+), 2 deletions(-)


base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.55.0

[PATCH v4 2/2] selftests/x86/amx: Test dynamic fpstate cleanup across exec()

[Guixiong Wei]

Add a regression test for dynamic fpstate leaks across exec(). Repeatedly
fork a child, load XTILEDATA to allocate dynamic state, and exec the test
binary. Snapshot the start addresses of matching /proc/vmallocinfo entries
before and after the loop; addresses present only in the "after" snapshot
count as new allocations that outlived their task.

Before the first exec(), verify that the child's allocation is visible.
Skip the check if vmallocinfo is inaccessible, if a matching entry's
address is hidden by kptr_restrict, or if the allocation cannot be
observed; address-set diff cannot distinguish allocations in those cases.

The test assumes no concurrent workload creates matching allocations.
A set difference equal to the iteration count indicates a leak; an empty
difference indicates success. Report other counts as inconclusive.

Assisted-by: LLM
Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
---
 tools/testing/selftests/x86/amx.c | 251 +++++++++++++++++++++++++++++-
 1 file changed, 249 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
index 40769c16de1bb..ea35d5332dc3f 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -5,6 +5,7 @@
 #include <errno.h>
 #include <setjmp.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <stdbool.h>
 #include <unistd.h>
@@ -30,6 +31,9 @@
 #define XFEATURE_MASK_XTILEDATA	(1 << XFEATURE_XTILEDATA)
 #define XFEATURE_MASK_XTILE	(XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
 
+#define EXEC_TEST_ARG		"--exec-test-child"
+#define EXEC_TEST_ITERS		10
+
 struct xstate_info xtiledata;
 
 /* The helpers for managing XSAVE buffer and tile states: */
@@ -478,11 +482,253 @@ static void test_fork(void)
 	_exit(0);
 }
 
-int main(void)
+/*
+ * Snapshot start addresses of /proc/vmallocinfo entries whose caller
+ * matches a dynamic fpstate allocation. Returns false if the file is
+ * not readable (default mode 0400), or if a matching entry's address
+ * cannot be used (kptr_restrict hides the address, or parsing fails);
+ * in those cases the caller cannot distinguish allocations and must
+ * SKIP rather than report a potentially false PASS.
+ */
+struct fpstate_snapshot {
+	unsigned long *addrs;
+	size_t len, cap;
+};
+
+static int cmp_ulong(const void *a, const void *b)
+{
+	unsigned long x = *(const unsigned long *)a, y = *(const unsigned long *)b;
+
+	return (x > y) - (x < y);
+}
+
+static bool snapshot_fpstates(struct fpstate_snapshot *s)
+{
+	char *line = NULL;
+	size_t line_size = 0;
+	bool ok = true;
+	FILE *fp;
+
+	s->addrs = NULL;
+	s->len = 0;
+	s->cap = 0;
+
+	fp = fopen("/proc/vmallocinfo", "r");
+	if (!fp) {
+		if (errno == EACCES || errno == EPERM || errno == ENOENT)
+			return false;
+		fatal_error("fopen /proc/vmallocinfo");
+	}
+
+	while (getline(&line, &line_size, fp) >= 0) {
+		unsigned long addr;
+
+		/*
+		 * fpstate_realloc() is static and may or may not be inlined
+		 * into __xfd_enable_feature() depending on build options;
+		 * match either caller to stay compiler-independent.
+		 */
+		if (!strstr(line, "__xfd_enable_feature") &&
+		    !strstr(line, "fpstate_realloc"))
+			continue;
+		/*
+		 * vmallocinfo prints the start address with %pK. If
+		 * kptr_restrict hides it (address shown as 0) or the
+		 * format is unexpected, address-set diff loses the
+		 * ability to distinguish allocations. Bail out so the
+		 * caller SKIPs instead of silently masking a leak.
+		 */
+		if (sscanf(line, "0x%lx", &addr) != 1 || !addr) {
+			ok = false;
+			break;
+		}
+		if (s->len == s->cap) {
+			unsigned long *tmp;
+
+			s->cap = s->cap ? s->cap * 2 : 64;
+			tmp = realloc(s->addrs, s->cap * sizeof(*s->addrs));
+			if (!tmp)
+				fatal_error("realloc snapshot");
+			s->addrs = tmp;
+		}
+		s->addrs[s->len++] = addr;
+	}
+
+	if (ferror(fp))
+		fatal_error("read /proc/vmallocinfo");
+
+	free(line);
+	fclose(fp);
+	if (!ok) {
+		free(s->addrs);
+		s->addrs = NULL;
+		s->len = 0;
+		s->cap = 0;
+		return false;
+	}
+	qsort(s->addrs, s->len, sizeof(*s->addrs), cmp_ulong);
+	return true;
+}
+
+static int count_new(const struct fpstate_snapshot *before,
+		     const struct fpstate_snapshot *after)
+{
+	int n = 0;
+	size_t i;
+
+	for (i = 0; i < after->len; i++)
+		if (!bsearch(&after->addrs[i], before->addrs, before->len,
+			     sizeof(*before->addrs), cmp_ulong))
+			n++;
+	return n;
+}
+
+static int exec_test_fails;
+
+static void check_exec_child_status(int status)
+{
+	if (WIFEXITED(status) && !WEXITSTATUS(status))
+		return;
+	if (WIFSIGNALED(status))
+		errx(1, "[FAIL]\texec test child killed by signal %d",
+		     WTERMSIG(status));
+	errx(1, "[FAIL]\texec test child exited with status %d",
+	     WEXITSTATUS(status));
+}
+
+/*
+ * Fork a child that loads tiledata and blocks on a pipe before exec().
+ * The parent samples /proc/vmallocinfo while the child is still suspended
+ * and verifies that the dynamic fpstate allocation is observable. Without
+ * that visibility the test cannot distinguish "released" from "never
+ * observed", so it reports SKIP instead of a potentially false PASS.
+ */
+static bool observe_dynamic_fpstate(const struct fpstate_snapshot *before)
+{
+	struct fpstate_snapshot mid;
+	int ready_pipe[2], go_pipe[2];
+	pid_t child;
+	int status;
+	bool observable;
+	char c = 0;
+
+	if (pipe(ready_pipe) || pipe(go_pipe))
+		fatal_error("pipe");
+
+	child = fork();
+	if (child < 0)
+		fatal_error("fork");
+	if (!child) {
+		close(ready_pipe[0]);
+		close(go_pipe[1]);
+		if (!load_rand_tiledata(stashed_xsave))
+			fatal_error("failed to load tiledata");
+		if (write(ready_pipe[1], &c, 1) != 1)
+			fatal_error("write ready");
+		if (read(go_pipe[0], &c, 1) != 1)
+			fatal_error("read go");
+		close(ready_pipe[1]);
+		close(go_pipe[0]);
+		execl("/proc/self/exe", "amx", EXEC_TEST_ARG, NULL);
+		fatal_error("exec");
+	}
+
+	close(ready_pipe[1]);
+	close(go_pipe[0]);
+
+	if (read(ready_pipe[0], &c, 1) != 1)
+		fatal_error("read ready");
+
+	observable = snapshot_fpstates(&mid) && count_new(before, &mid) > 0;
+	free(mid.addrs);
+
+	if (write(go_pipe[1], &c, 1) != 1)
+		fatal_error("write go");
+
+	close(ready_pipe[0]);
+	close(go_pipe[1]);
+
+	if (waitpid(child, &status, 0) != child)
+		fatal_error("waitpid");
+	check_exec_child_status(status);
+
+	return observable;
+}
+
+static void test_exec(void)
+{
+	struct fpstate_snapshot before, after;
+	int new_count, i;
+
+	printf("[RUN]\tCheck dynamic fpstate cleanup across exec().\n");
+
+	if (!snapshot_fpstates(&before)) {
+		printf("[SKIP]\tvmallocinfo inaccessible or addresses hidden by kptr_restrict.\n");
+		return;
+	}
+
+	if (!observe_dynamic_fpstate(&before)) {
+		free(before.addrs);
+		printf("[SKIP]\tCannot observe dynamic fpstate allocations via vmallocinfo.\n");
+		return;
+	}
+	free(before.addrs);
+
+	if (!snapshot_fpstates(&before))
+		fatal_error("snapshot before");
+
+	for (i = 0; i < EXEC_TEST_ITERS; i++) {
+		pid_t child;
+		int status;
+
+		child = fork();
+		if (child < 0)
+			fatal_error("fork");
+		if (!child) {
+			if (!load_rand_tiledata(stashed_xsave))
+				fatal_error("failed to load tiledata");
+			execl("/proc/self/exe", "amx", EXEC_TEST_ARG, NULL);
+			fatal_error("exec");
+		}
+
+		if (waitpid(child, &status, 0) != child)
+			fatal_error("waitpid");
+		check_exec_child_status(status);
+	}
+
+	if (!snapshot_fpstates(&after)) {
+		free(before.addrs);
+		printf("[SKIP]\tvmallocinfo inaccessible or addresses hidden by kptr_restrict.\n");
+		return;
+	}
+	new_count = count_new(&before, &after);
+	free(before.addrs);
+	free(after.addrs);
+
+	if (!new_count) {
+		printf("[OK]\tDynamic fpstate allocations were freed across exec().\n");
+		return;
+	}
+
+	if (new_count == EXEC_TEST_ITERS) {
+		printf("[FAIL]\texec() leaked %d dynamic fpstate allocations\n",
+		       new_count);
+		exec_test_fails++;
+		return;
+	}
+
+	printf("[SKIP]\tUnexpected new allocation count %d; result inconclusive.\n",
+	       new_count);
+}
+
+int main(int argc, char **argv)
 {
 	unsigned long features;
 	long rc;
 
+	if (argc == 2 && !strcmp(argv[1], EXEC_TEST_ARG))
+		return 0;
+
 	rc = syscall(SYS_arch_prctl, ARCH_GET_XCOMP_SUPP, &features);
 	if (rc || (features & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE) {
 		ksft_print_msg("no AMX support\n");
@@ -504,6 +750,7 @@ int main(void)
 	req_xtiledata_perm();
 
 	test_fork();
+	test_exec();
 
 	/*
 	 * Perform generic xstate tests for context switching, ptrace,
@@ -514,5 +761,5 @@ int main(void)
 	clearhandler(SIGILL);
 	free_stashed_xsave();
 
-	return 0;
+	return exec_test_fails ? 1 : 0;
 }
-- 
2.55.0

[PATCH v4 1/2] x86/fpu: Fix memory leak with dynamic fpstate and exec()

[Guixiong Wei]

Using an XFD-controlled xfeature can allocate a larger fpstate. On
exec(), fpstate_reset() replaces fpu->fpstate with the embedded buffer
without freeing the dynamic allocation. Once the pointer is overwritten,
the allocation cannot be freed on task exit.

Free dynamic state at the start of fpstate_reset(). In fpu_clone(),
clear the new task's fpstate pointer before resetting its state, so the
parent's allocation is not freed.

fpstate_reset() may now free the current task's buffer. fpu_flush_thread()
calls it before fpu_reset_fpstate_regs() sets TIF_NEED_FPU_LOAD. If a
context switch or a kernel FPU user happens in that window, the register
state still looks live and gets saved into the freed buffer. Set
TIF_NEED_FPU_LOAD before the reset so that nothing saves the hardware
state until it is reloaded from the reset in-memory state.

Fixes: 500afbf645a0 ("x86/fpu/xstate: Add fpstate_realloc()/free()")
Suggested-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
---
 arch/x86/kernel/fpu/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index d1aeecd57f5ed..af1c1688d69bf 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -592,8 +592,15 @@ static void __fpstate_reset(struct fpstate *fpstate)
 	fpstate->user_xfeatures	= fpu_user_cfg.default_features;
 }
 
+/*
+ * Callers resetting the current task must ensure that its FPU register
+ * state isn't saved on context switch as the buffer may be freed here.
+ */
 void fpstate_reset(struct fpu *fpu)
 {
+	/* Free dynamic fpstate (if any) before resetting pointer: */
+	fpstate_free(fpu);
+
 	/* Set the fpstate pointer to the default fpstate */
 	fpu->fpstate = &fpu->__fpstate;
 	__fpstate_reset(fpu->fpstate);
@@ -670,6 +677,8 @@ int fpu_clone(struct task_struct *dst, u64 clone_flags, bool minimal,
 	/* The new task's FPU state cannot be valid in the hardware. */
 	dst_fpu->last_cpu = -1;
 
+	/* Break the link to the other FPU's fpstate: */
+	dst_fpu->fpstate = NULL;
 	fpstate_reset(dst_fpu);
 
 	if (!cpu_feature_enabled(X86_FEATURE_FPU))
@@ -866,6 +875,9 @@ void fpu__clear_user_states(struct fpu *fpu)
 
 void fpu_flush_thread(void)
 {
+	/* Prevent the hardware state from being saved on context switch */
+	set_thread_flag(TIF_NEED_FPU_LOAD);
+
 	fpstate_reset(x86_task_fpu(current));
 	fpu_reset_fpstate_regs();
 }

base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.55.0