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

["Chang S. Bae" <chang.seok.bae@intel.com>]

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);
}