Re: [patch V4 09/65] x86/fpu: Sanitize xstateregs_set()

From: Chang S. Bae
Date: Thu Jul 28 2022 - 19:32:33 EST

Next message: Beau Belgrave: "[PATCH v3 1/6] tracing/user_events: Use NULL for strstr checks"
Previous message: Neel Natu: "[PATCH] lib/cmdline: avoid page fault in next_arg"
In reply to: Dave Hansen: "Re: [patch V4 09/65] x86/fpu: Sanitize xstateregs_set()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 7/25/2022 2:26 PM, Dave Hansen wrote:

Do you happen to have a quick reproducer for this, or at least the
contents of the buffer that you are trying to restore?

While not following this report, I think there is a regression along with the changes:

As looking into the spec, this state load does not depend on XSTATE_BV:

RFBM := XCR0 AND EDX:EAX;
COMPMASK := XCOMP_BV field from XSAVE header;

IF COMPMASK[63] = 0
THEN
...
IF RFBM[1] = 1 OR RFBM[2] = 1
THEN load MXCSR from legacy region of XSAVE area;
FI;
...
ELSE
...

But our upstream code does reference XSTATE_BV instead of RFBM [1,2].

My test case [3] fails with the upstream but works with 5.13, which is before the series. Then, this change looks to make it work at least for it:

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index c8340156bfd2..db4ab5c7ba8b 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1094,7 +1094,7 @@ void __copy_xstate_to_uabi_buf(struct membuf to,
struct fpstate *fpstate,
&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_feature(fpstate->user_xfeatures & (XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM),
&to, &xsave->i387.mxcsr, &xinit->i387.mxcsr,
MXCSR_AND_FLAGS_SIZE);

@@ -1214,7 +1214,7 @@ static int copy_uabi_to_xstate(struct fpstate
*fpstate, const void *kbuf,

/* 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 (fpstate->user_xfeatures & mask) {
u32 mxcsr[2];

offset = offsetof(struct fxregs_state, mxcsr);
@@ -1226,7 +1226,7 @@ static int copy_uabi_to_xstate(struct fpstate
*fpstate, const void *kbuf,
return -EINVAL;

/* SSE and YMM require MXCSR even when FP is not in use. */
- if (!(hdr.xfeatures & XFEATURE_MASK_FP)) {
+ if (fpstate->user_xfeatures & (XFEATURE_MASK_SSE |
XFEATURE_MASK_YMM)) {
xsave->i387.mxcsr = mxcsr[0];
xsave->i387.mxcsr_mask = mxcsr[1];
}

Thanks,
Chang

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n1097
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n1217
[3] test case:

#include <err.h>
#include <elf.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>

#define PAGE_SIZE 4096

typedef uint8_t u8;
typedef uint16_t u16;
typedef uint32_t u32;
typedef uint64_t u64;

/* The below struct and define are copied from arch/x86/include/asm/fpu/types.h */

struct fxregs_state {
u16 cwd; /* Control Word */
u16 swd; /* Status Word */
u16 twd; /* Tag Word */
u16 fop; /* Last Instruction Opcode */
union {
struct {
u64 rip; /* Instruction Pointer */
u64 rdp; /* Data Pointer */
};
struct {
u32 fip; /* FPU IP Offset */
u32 fcs; /* FPU IP Selector */
u32 foo; /* FPU Operand Offset */
u32 fos; /* FPU Operand Selector */
};
};
u32 mxcsr; /* MXCSR Register State */
u32 mxcsr_mask; /* MXCSR Mask */

/* 8*16 bytes for each FP-reg = 128 bytes: */
u32 st_space[32];

/* 16*16 bytes for each XMM-reg = 256 bytes: */
u32 xmm_space[64];

u32 padding[12];

union {
u32 padding1[12];
u32 sw_reserved[12];
};

} __attribute__((aligned(16)));

struct xstate_header {
u64 xfeatures;
u64 xcomp_bv;
u64 reserved[6];
} __attribute__((packed));

struct xregs_state {
struct fxregs_state i387;
struct xstate_header header;
u8 extended_state_area[0];
} __attribute__ ((packed, aligned (64)));

union fpregs_state {
struct xregs_state xsave;
u8 __padding[PAGE_SIZE];
};

/*
* List of XSAVE features Linux knows about:
*/
enum xfeature {
XFEATURE_FP,
XFEATURE_SSE,
};

#define XFEATURE_MASK_SSE (1 << XFEATURE_SSE)

/* Default value for fxregs_state.mxcsr: */
#define MXCSR_DEFAULT 0x1f80

void main(void)
{
union fpregs_state *xbuf;
struct iovec iov;
pid_t child;
int status;
u32 mxcsr;

xbuf = aligned_alloc(64, sizeof(union fpregs_state));
if (!xbuf)
err(1, "aligned_alloc()");
memset(xbuf, 0, sizeof(union fpregs_state));

iov.iov_base = xbuf;
iov.iov_len = sizeof(union fpregs_state);

child = fork();
if (!child) {
if (ptrace(PTRACE_TRACEME, 0, NULL, NULL))
err(1, "PTRACE_TRACEME");

raise(SIGTRAP);
_exit(0);
}

do {
wait(&status);
} while (WSTOPSIG(status) != SIGTRAP);

printf("[RUN]\tCheck the default MXCSR state.\n");

if (ptrace(PTRACE_GETREGSET, child, (uint32_t)NT_X86_XSTATE, &iov))
err(1, "PTRACE_GETREGSET");

printf("[%svalid init value.\n",
(xbuf->xsave.i387.mxcsr == MXCSR_DEFAULT) ?
"OK]\twith the " : "FAIL]\twith an in");

printf("[RUN]\tTest MXCSR state write.\n");
xbuf->xsave.i387.mxcsr = 0;
/* the MXCSR state should be loaded regardless of XSTATE_BV */
xbuf->xsave.header.xfeatures = 0;

if (ptrace(PTRACE_SETREGSET, child, (uint32_t)NT_X86_XSTATE, &iov))
err(1, "PTRACE_SETREGSET");

/* ditch the MXCSR state */
xbuf->xsave.i387.mxcsr = 0xff;
xbuf->xsave.i387.mxcsr_mask = 0xff;

if (ptrace(PTRACE_GETREGSET, child, (uint32_t)NT_X86_XSTATE, &iov))
err(1, "PTRACE_GETREGSET");

printf("[%s]\tthe state was %swritten correctly\n",
!xbuf->xsave.i387.mxcsr ? "OK" : "FAIL",
!xbuf->xsave.i387.mxcsr ? "" : "not ");

ptrace(PTRACE_DETACH, child, NULL, NULL);
wait(&status);
if (!WIFEXITED(status) || WEXITSTATUS(status))
err(1, "PTRACE_DETACH");

free(xbuf);
}

Next message: Beau Belgrave: "[PATCH v3 1/6] tracing/user_events: Use NULL for strstr checks"
Previous message: Neel Natu: "[PATCH] lib/cmdline: avoid page fault in next_arg"
In reply to: Dave Hansen: "Re: [patch V4 09/65] x86/fpu: Sanitize xstateregs_set()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]