From: Thomas Gleixner <tglx@linutronix.de> To: Fenghua Yu <fenghua.yu@intel.com> Cc: linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>, iommu@lists.linux-foundation.org, Borislav Petkov <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>, H Peter Anvin <hpa@zytor.com>, Andy Lutomirski <luto@kernel.org>, Jean-Philippe Brucker <jean-philippe@linaro.org>, Christoph Hellwig <hch@infradead.org>, Peter Zijlstra <peterz@infradead.org>, David Woodhouse <dwmw2@infradead.org>, Lu Baolu <baolu.lu@linux.intel.com>, Dave Hansen <dave.hansen@intel.com>, Tony Luck <tony.luck@intel.com>, Randy Dunlap <rdunlap@infradead.org>, Ashok Raj <ashok.raj@intel.com>, Jacob Jun Pan <jacob.jun.pan@intel.com>, Dave Jiang <dave.jiang@intel.com>, Sohil Mehta <sohil.mehta@intel.com>, Ravi V Shankar <ravi.v.shankar@intel.com> Subject: [PATCH] x86/cpufeatures: Force disable X86_FEATURE_ENQCMD and remove update_pasid() Date: Sat, 29 May 2021 11:17:30 +0200 [thread overview] Message-ID: <87mtsd6gr9.ffs@nanos.tec.linutronix.de> (raw) In-Reply-To: <1600187413-163670-10-git-send-email-fenghua.yu@intel.com> While digesting the XSAVE related horrors, which got introduced with the supervisor/user split, the recent addition of ENQCMD related functionality got on the radar and turned out to be similarly broken. update_pasid(), which is only required when X86_FEATURE_ENQCMD is available, is invoked from two places: 1) From switch_to() for the incoming task 2) Via a SMP function call from the IOMMU/SMV code #1 is half-ways correct as it hacks around the brokenness of get_xsave_addr() by enforcing the state to be 'present', but all the conditionals in that code are completely pointless for that. Also the invocation is just useless overhead because at that point it's guaranteed that TIF_NEED_FPU_LOAD is set on the incoming task and all of this can be handled at return to user space. #2 is broken beyond repair. The comment in the code claims that it is safe to invoke this in an IPI, but that's just wishful thinking. FPU state of a running task is protected by fregs_lock() which is nothing else than a local_bh_disable(). As BH disabled regions run usually with interrupts enabled the IPI can hit a code section which modifies FPU state and there is absolutely no guarantee that any of the assumptions which are made for the IPI case is true. Also the IPI is sent to all CPUs in mm_cpumask(mm), but the IPI is invoked with a NULL pointer argument, so it can hit a completely unrelated task and unconditionally force an update for nothing. Worse it can hit a kernel thread which operates on a user space address space and set a random PASID for it. The offending commit does not cleanly revert, but it's sufficient to force disable X86_FEATURE_ENQCMD and to remove the broken update_pasid() code to make this dysfunctional all over the place. Anything more complex would require more surgery and none of the related functions outside of the x86 core code are blatantly wrong, so removing those would be overkill. As nothing enables the PASID bit in the IA32_XSS MSR yet, which is required to make this actually work, this cannot result in a regression except for related out of tree train-wrecks, but they are broken already today. Fixes: 20f0afd1fb3d ("x86/mmu: Allocate/free a PASID") Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: stable@vger.kernel.org --- See also: https://lore.kernel.org/lkml/874keo80bh.ffs@nanos.tec.linutronix.de --- arch/x86/include/asm/disabled-features.h | 7 +-- arch/x86/include/asm/fpu/api.h | 6 --- arch/x86/include/asm/fpu/internal.h | 7 --- arch/x86/kernel/fpu/xstate.c | 57 ------------------------------- 4 files changed, 3 insertions(+), 74 deletions(-) --- a/arch/x86/include/asm/disabled-features.h +++ b/arch/x86/include/asm/disabled-features.h @@ -56,11 +56,8 @@ # define DISABLE_PTI (1 << (X86_FEATURE_PTI & 31)) #endif -#ifdef CONFIG_IOMMU_SUPPORT -# define DISABLE_ENQCMD 0 -#else -# define DISABLE_ENQCMD (1 << (X86_FEATURE_ENQCMD & 31)) -#endif +/* Force disable because it's broken beyond repair */ +#define DISABLE_ENQCMD (1 << (X86_FEATURE_ENQCMD & 31)) #ifdef CONFIG_X86_SGX # define DISABLE_SGX 0 --- a/arch/x86/include/asm/fpu/api.h +++ b/arch/x86/include/asm/fpu/api.h @@ -106,10 +106,6 @@ extern int cpu_has_xfeatures(u64 xfeatur */ #define PASID_DISABLED 0 -#ifdef CONFIG_IOMMU_SUPPORT -/* Update current's PASID MSR/state by mm's PASID. */ -void update_pasid(void); -#else static inline void update_pasid(void) { } -#endif + #endif /* _ASM_X86_FPU_API_H */ --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -583,13 +583,6 @@ static inline void switch_fpu_finish(str pkru_val = pk->pkru; } __write_pkru(pkru_val); - - /* - * Expensive PASID MSR write will be avoided in update_pasid() because - * TIF_NEED_FPU_LOAD was set. And the PASID state won't be updated - * unless it's different from mm->pasid to reduce overhead. - */ - update_pasid(); } #endif /* _ASM_X86_FPU_INTERNAL_H */ --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -1402,60 +1402,3 @@ int proc_pid_arch_status(struct seq_file return 0; } #endif /* CONFIG_PROC_PID_ARCH_STATUS */ - -#ifdef CONFIG_IOMMU_SUPPORT -void update_pasid(void) -{ - u64 pasid_state; - u32 pasid; - - if (!cpu_feature_enabled(X86_FEATURE_ENQCMD)) - return; - - if (!current->mm) - return; - - pasid = READ_ONCE(current->mm->pasid); - /* Set the valid bit in the PASID MSR/state only for valid pasid. */ - pasid_state = pasid == PASID_DISABLED ? - pasid : pasid | MSR_IA32_PASID_VALID; - - /* - * No need to hold fregs_lock() since the task's fpstate won't - * be changed by others (e.g. ptrace) while the task is being - * switched to or is in IPI. - */ - if (!test_thread_flag(TIF_NEED_FPU_LOAD)) { - /* The MSR is active and can be directly updated. */ - wrmsrl(MSR_IA32_PASID, pasid_state); - } else { - struct fpu *fpu = ¤t->thread.fpu; - struct ia32_pasid_state *ppasid_state; - struct xregs_state *xsave; - - /* - * The CPU's xstate registers are not currently active. Just - * update the PASID state in the memory buffer here. The - * PASID MSR will be loaded when returning to user mode. - */ - xsave = &fpu->state.xsave; - xsave->header.xfeatures |= XFEATURE_MASK_PASID; - ppasid_state = get_xsave_addr(xsave, XFEATURE_PASID); - /* - * Since XFEATURE_MASK_PASID is set in xfeatures, ppasid_state - * won't be NULL and no need to check its value. - * - * Only update the task's PASID state when it's different - * from the mm's pasid. - */ - if (ppasid_state->pasid != pasid_state) { - /* - * Invalid fpregs so that state restoring will pick up - * the PASID state. - */ - __fpu_invalidate_fpregs_state(fpu); - ppasid_state->pasid = pasid_state; - } - } -} -#endif /* CONFIG_IOMMU_SUPPORT */
WARNING: multiple messages have this Message-ID (diff)
From: Thomas Gleixner <tglx@linutronix.de> To: Fenghua Yu <fenghua.yu@intel.com> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>, Tony Luck <tony.luck@intel.com>, Dave Jiang <dave.jiang@intel.com>, Ashok Raj <ashok.raj@intel.com>, Ravi V Shankar <ravi.v.shankar@intel.com>, Peter Zijlstra <peterz@infradead.org>, x86 <x86@kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Jacob Jun Pan <jacob.jun.pan@intel.com>, Christoph Hellwig <hch@infradead.org>, Dave Hansen <dave.hansen@intel.com>, iommu@lists.linux-foundation.org, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, Randy Dunlap <rdunlap@infradead.org>, Andy Lutomirski <luto@kernel.org>, H Peter Anvin <hpa@zytor.com>, David Woodhouse <dwmw2@infradead.org> Subject: [PATCH] x86/cpufeatures: Force disable X86_FEATURE_ENQCMD and remove update_pasid() Date: Sat, 29 May 2021 11:17:30 +0200 [thread overview] Message-ID: <87mtsd6gr9.ffs@nanos.tec.linutronix.de> (raw) In-Reply-To: <1600187413-163670-10-git-send-email-fenghua.yu@intel.com> While digesting the XSAVE related horrors, which got introduced with the supervisor/user split, the recent addition of ENQCMD related functionality got on the radar and turned out to be similarly broken. update_pasid(), which is only required when X86_FEATURE_ENQCMD is available, is invoked from two places: 1) From switch_to() for the incoming task 2) Via a SMP function call from the IOMMU/SMV code #1 is half-ways correct as it hacks around the brokenness of get_xsave_addr() by enforcing the state to be 'present', but all the conditionals in that code are completely pointless for that. Also the invocation is just useless overhead because at that point it's guaranteed that TIF_NEED_FPU_LOAD is set on the incoming task and all of this can be handled at return to user space. #2 is broken beyond repair. The comment in the code claims that it is safe to invoke this in an IPI, but that's just wishful thinking. FPU state of a running task is protected by fregs_lock() which is nothing else than a local_bh_disable(). As BH disabled regions run usually with interrupts enabled the IPI can hit a code section which modifies FPU state and there is absolutely no guarantee that any of the assumptions which are made for the IPI case is true. Also the IPI is sent to all CPUs in mm_cpumask(mm), but the IPI is invoked with a NULL pointer argument, so it can hit a completely unrelated task and unconditionally force an update for nothing. Worse it can hit a kernel thread which operates on a user space address space and set a random PASID for it. The offending commit does not cleanly revert, but it's sufficient to force disable X86_FEATURE_ENQCMD and to remove the broken update_pasid() code to make this dysfunctional all over the place. Anything more complex would require more surgery and none of the related functions outside of the x86 core code are blatantly wrong, so removing those would be overkill. As nothing enables the PASID bit in the IA32_XSS MSR yet, which is required to make this actually work, this cannot result in a regression except for related out of tree train-wrecks, but they are broken already today. Fixes: 20f0afd1fb3d ("x86/mmu: Allocate/free a PASID") Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: stable@vger.kernel.org --- See also: https://lore.kernel.org/lkml/874keo80bh.ffs@nanos.tec.linutronix.de --- arch/x86/include/asm/disabled-features.h | 7 +-- arch/x86/include/asm/fpu/api.h | 6 --- arch/x86/include/asm/fpu/internal.h | 7 --- arch/x86/kernel/fpu/xstate.c | 57 ------------------------------- 4 files changed, 3 insertions(+), 74 deletions(-) --- a/arch/x86/include/asm/disabled-features.h +++ b/arch/x86/include/asm/disabled-features.h @@ -56,11 +56,8 @@ # define DISABLE_PTI (1 << (X86_FEATURE_PTI & 31)) #endif -#ifdef CONFIG_IOMMU_SUPPORT -# define DISABLE_ENQCMD 0 -#else -# define DISABLE_ENQCMD (1 << (X86_FEATURE_ENQCMD & 31)) -#endif +/* Force disable because it's broken beyond repair */ +#define DISABLE_ENQCMD (1 << (X86_FEATURE_ENQCMD & 31)) #ifdef CONFIG_X86_SGX # define DISABLE_SGX 0 --- a/arch/x86/include/asm/fpu/api.h +++ b/arch/x86/include/asm/fpu/api.h @@ -106,10 +106,6 @@ extern int cpu_has_xfeatures(u64 xfeatur */ #define PASID_DISABLED 0 -#ifdef CONFIG_IOMMU_SUPPORT -/* Update current's PASID MSR/state by mm's PASID. */ -void update_pasid(void); -#else static inline void update_pasid(void) { } -#endif + #endif /* _ASM_X86_FPU_API_H */ --- a/arch/x86/include/asm/fpu/internal.h +++ b/arch/x86/include/asm/fpu/internal.h @@ -583,13 +583,6 @@ static inline void switch_fpu_finish(str pkru_val = pk->pkru; } __write_pkru(pkru_val); - - /* - * Expensive PASID MSR write will be avoided in update_pasid() because - * TIF_NEED_FPU_LOAD was set. And the PASID state won't be updated - * unless it's different from mm->pasid to reduce overhead. - */ - update_pasid(); } #endif /* _ASM_X86_FPU_INTERNAL_H */ --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -1402,60 +1402,3 @@ int proc_pid_arch_status(struct seq_file return 0; } #endif /* CONFIG_PROC_PID_ARCH_STATUS */ - -#ifdef CONFIG_IOMMU_SUPPORT -void update_pasid(void) -{ - u64 pasid_state; - u32 pasid; - - if (!cpu_feature_enabled(X86_FEATURE_ENQCMD)) - return; - - if (!current->mm) - return; - - pasid = READ_ONCE(current->mm->pasid); - /* Set the valid bit in the PASID MSR/state only for valid pasid. */ - pasid_state = pasid == PASID_DISABLED ? - pasid : pasid | MSR_IA32_PASID_VALID; - - /* - * No need to hold fregs_lock() since the task's fpstate won't - * be changed by others (e.g. ptrace) while the task is being - * switched to or is in IPI. - */ - if (!test_thread_flag(TIF_NEED_FPU_LOAD)) { - /* The MSR is active and can be directly updated. */ - wrmsrl(MSR_IA32_PASID, pasid_state); - } else { - struct fpu *fpu = ¤t->thread.fpu; - struct ia32_pasid_state *ppasid_state; - struct xregs_state *xsave; - - /* - * The CPU's xstate registers are not currently active. Just - * update the PASID state in the memory buffer here. The - * PASID MSR will be loaded when returning to user mode. - */ - xsave = &fpu->state.xsave; - xsave->header.xfeatures |= XFEATURE_MASK_PASID; - ppasid_state = get_xsave_addr(xsave, XFEATURE_PASID); - /* - * Since XFEATURE_MASK_PASID is set in xfeatures, ppasid_state - * won't be NULL and no need to check its value. - * - * Only update the task's PASID state when it's different - * from the mm's pasid. - */ - if (ppasid_state->pasid != pasid_state) { - /* - * Invalid fpregs so that state restoring will pick up - * the PASID state. - */ - __fpu_invalidate_fpregs_state(fpu); - ppasid_state->pasid = pasid_state; - } - } -} -#endif /* CONFIG_IOMMU_SUPPORT */ _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-05-29 9:17 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-15 16:30 [PATCH v8 0/9] x86: tag application address space for devices Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 1/9] drm, iommu: Change type of pasid to u32 Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] " tip-bot2 for Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 2/9] iommu/vt-d: Change flags type to unsigned int in binding mm Fenghua Yu 2020-09-15 16:30 ` Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] " tip-bot2 for Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 3/9] Documentation/x86: Add documentation for SVA (Shared Virtual Addressing) Fenghua Yu 2020-09-17 7:53 ` Borislav Petkov 2020-09-17 7:53 ` Borislav Petkov 2020-09-17 14:56 ` Raj, Ashok 2020-09-17 14:56 ` Raj, Ashok 2020-09-17 17:18 ` Borislav Petkov 2020-09-17 17:18 ` Borislav Petkov 2020-09-17 17:22 ` Raj, Ashok 2020-09-17 17:22 ` Raj, Ashok 2020-09-17 17:30 ` Borislav Petkov 2020-09-17 17:30 ` Borislav Petkov 2020-09-18 16:22 ` Fenghua Yu 2020-09-18 16:22 ` Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] " tip-bot2 for Ashok Raj 2020-09-15 16:30 ` [PATCH v8 4/9] x86/cpufeatures: Enumerate ENQCMD and ENQCMDS instructions Fenghua Yu 2020-09-15 16:30 ` Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] " tip-bot2 for Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 5/9] x86/fpu/xstate: Add supervisor PASID state for ENQCMD feature Fenghua Yu 2020-09-15 16:30 ` Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] x86/fpu/xstate: Add supervisor PASID state for ENQCMD tip-bot2 for Yu-cheng Yu 2020-09-15 16:30 ` [PATCH v8 6/9] x86/msr-index: Define IA32_PASID MSR Fenghua Yu 2020-09-15 16:30 ` Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] x86/msr-index: Define an " tip-bot2 for Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 7/9] mm: Define pasid in mm Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] mm: Add a pasid member to struct mm_struct tip-bot2 for Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 8/9] x86/cpufeatures: Mark ENQCMD as disabled when configured out Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] " tip-bot2 for Fenghua Yu 2020-09-15 16:30 ` [PATCH v8 9/9] x86/mmu: Allocate/free PASID Fenghua Yu 2020-09-15 16:30 ` Fenghua Yu 2020-09-18 7:42 ` [tip: x86/pasid] x86/mmu: Allocate/free a PASID tip-bot2 for Fenghua Yu 2021-05-29 9:17 ` Thomas Gleixner [this message] 2021-05-29 9:17 ` [PATCH] x86/cpufeatures: Force disable X86_FEATURE_ENQCMD and remove update_pasid() Thomas Gleixner 2021-05-31 8:43 ` Borislav Petkov 2021-05-31 8:43 ` Borislav Petkov 2021-05-31 10:16 ` Thomas Gleixner 2021-05-31 10:16 ` Thomas Gleixner 2021-06-02 20:37 ` Luck, Tony 2021-06-02 20:37 ` Luck, Tony 2021-06-03 17:31 ` Andy Lutomirski 2021-06-03 17:31 ` Andy Lutomirski 2021-06-09 17:32 ` Luck, Tony 2021-06-09 17:32 ` Luck, Tony 2021-06-09 23:34 ` Andy Lutomirski 2021-06-09 23:34 ` Andy Lutomirski 2021-06-25 15:46 ` Luck, Tony 2021-06-25 15:46 ` Luck, Tony 2021-06-02 10:14 ` Borislav Petkov 2021-06-02 10:14 ` Borislav Petkov 2021-06-02 10:20 ` Thomas Gleixner 2021-06-02 10:20 ` Thomas Gleixner 2021-06-03 11:20 ` Vinod Koul 2021-06-03 11:20 ` Vinod Koul 2021-06-03 11:42 ` Borislav Petkov 2021-06-03 11:42 ` Borislav Petkov 2021-06-03 12:47 ` Vinod Koul 2021-06-03 12:47 ` Vinod Koul 2021-06-03 14:33 ` Borislav Petkov 2021-06-03 14:33 ` Borislav Petkov 2021-06-02 19:49 ` [tip: x86/urgent] " tip-bot2 for Thomas Gleixner 2021-06-03 14:38 ` tip-bot2 for Thomas Gleixner 2020-09-16 8:06 ` [PATCH v8 0/9] x86: tag application address space for devices Joerg Roedel 2020-09-16 8:06 ` Joerg Roedel 2020-09-17 23:53 ` Fenghua Yu 2020-09-17 23:53 ` Fenghua Yu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87mtsd6gr9.ffs@nanos.tec.linutronix.de \ --to=tglx@linutronix.de \ --cc=ashok.raj@intel.com \ --cc=baolu.lu@linux.intel.com \ --cc=bp@alien8.de \ --cc=dave.hansen@intel.com \ --cc=dave.jiang@intel.com \ --cc=dwmw2@infradead.org \ --cc=fenghua.yu@intel.com \ --cc=hch@infradead.org \ --cc=hpa@zytor.com \ --cc=iommu@lists.linux-foundation.org \ --cc=jacob.jun.pan@intel.com \ --cc=jean-philippe@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=ravi.v.shankar@intel.com \ --cc=rdunlap@infradead.org \ --cc=sohil.mehta@intel.com \ --cc=tony.luck@intel.com \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.