From 617a11eab6337693eae9d160453adf1943ab6a37 Mon Sep 17 00:00:00 2001 From: Konstantin Belousov Date: Tue, 18 Apr 2023 18:50:26 +0300 Subject: [PATCH] x86: initialize use_xsave once The explanation from https://reviews.freebsd.org/D39637 by stevek: The "use_xsave" variable is a global and that is only supposed to be initialized early before scheduling gets started. However, with the way the ifuncs for "fpusave" and "fpurestore" are implemented, the value could be changed at runtime when scheduling is active if "use_xsave" was set to 0 by the tunable. This leaves a window of opportunity where "use_xsave" gets re-initialized to 1 and a context switch could occur with a thread that was not set up to be able to use xsave functionality. This can lead to an "privileged instruction fault". The fix is to protect "use_xsave" from being initialized more than once. Reported and reviewed by: stevek Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D39660 --- sys/amd64/amd64/fpu.c | 16 ---------------- sys/amd64/amd64/machdep.c | 5 +++++ sys/i386/i386/machdep.c | 6 ++++++ sys/i386/i386/npx.c | 14 -------------- 4 files changed, 11 insertions(+), 30 deletions(-) diff --git a/sys/amd64/amd64/fpu.c b/sys/amd64/amd64/fpu.c index 64974a7210a9..a3421cf0d496 100644 --- a/sys/amd64/amd64/fpu.c +++ b/sys/amd64/amd64/fpu.c @@ -238,22 +238,8 @@ fpurestore_fxrstor(void *addr) fxrstor((char *)addr); } -static void -init_xsave(void) -{ - - if (use_xsave) - return; - if ((cpu_feature2 & CPUID2_XSAVE) == 0) - return; - use_xsave = 1; - TUNABLE_INT_FETCH("hw.use_xsave", &use_xsave); -} - DEFINE_IFUNC(, void, fpusave, (void *)) { - - init_xsave(); if (!use_xsave) return (fpusave_fxsave); if ((cpu_stdext_feature & CPUID_EXTSTATE_XSAVEOPT) != 0) { @@ -266,8 +252,6 @@ DEFINE_IFUNC(, void, fpusave, (void *)) DEFINE_IFUNC(, void, fpurestore, (void *)) { - - init_xsave(); if (!use_xsave) return (fpurestore_fxrstor); return ((cpu_stdext_feature & CPUID_STDEXT_NFPUSG) != 0 ? diff --git a/sys/amd64/amd64/machdep.c b/sys/amd64/amd64/machdep.c index 480db1ed2c31..29a0bd6f9db4 100644 --- a/sys/amd64/amd64/machdep.c +++ b/sys/amd64/amd64/machdep.c @@ -1345,6 +1345,11 @@ hammer_time(u_int64_t modulep, u_int64_t physfree) &pmap_pcid_invlpg_workaround_uena); cpu_init_small_core(); + if ((cpu_feature2 & CPUID2_XSAVE) != 0) { + use_xsave = 1; + TUNABLE_INT_FETCH("hw.use_xsave", &use_xsave); + } + link_elf_ireloc(kmdp); /* diff --git a/sys/i386/i386/machdep.c b/sys/i386/i386/machdep.c index d59e95ed1a99..1003dbe539c2 100644 --- a/sys/i386/i386/machdep.c +++ b/sys/i386/i386/machdep.c @@ -1545,6 +1545,11 @@ init386(int first) i386_kdb_init(); } + if (cpu_fxsr && (cpu_feature2 & CPUID2_XSAVE) != 0) { + use_xsave = 1; + TUNABLE_INT_FETCH("hw.use_xsave", &use_xsave); + } + kmdp = preload_search_by_type("elf kernel"); link_elf_ireloc(kmdp); @@ -1565,6 +1570,7 @@ init386(int first) msgbufinit(msgbufp, msgbufsize); npxinit(true); + /* * Set up thread0 pcb after npxinit calculated pcb + fpu save * area size. Zero out the extended state header in fpu save diff --git a/sys/i386/i386/npx.c b/sys/i386/i386/npx.c index 3d4f2f2a60c8..91967b4be843 100644 --- a/sys/i386/i386/npx.c +++ b/sys/i386/i386/npx.c @@ -320,22 +320,8 @@ fpusave_fnsave(union savefpu *addr) fnsave((char *)addr); } -static void -init_xsave(void) -{ - - if (use_xsave) - return; - if (!cpu_fxsr || (cpu_feature2 & CPUID2_XSAVE) == 0) - return; - use_xsave = 1; - TUNABLE_INT_FETCH("hw.use_xsave", &use_xsave); -} - DEFINE_IFUNC(, void, fpusave, (union savefpu *)) { - - init_xsave(); if (use_xsave) return ((cpu_stdext_feature & CPUID_EXTSTATE_XSAVEOPT) != 0 ? fpusave_xsaveopt : fpusave_xsave);