Messages in this thread Patch in this message | | | Date | Thu, 28 Jul 2022 16:32:13 -0700 | Subject | Re: [patch V4 09/65] x86/fpu: Sanitize xstateregs_set() | From | "Chang S. Bae" <> |
| |
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); }
| |