Skip to content

Commit 9bfecd0

Browse files
KAGA-KOKOsuryasaimadhu
authored andcommitted
x86/cpufeatures: Force disable X86_FEATURE_ENQCMD and remove update_pasid()
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 Rust-for-Linux#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. Rust-for-Linux#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: 20f0afd ("x86/mmu: Allocate/free a PASID") Signed-off-by: Thomas Gleixner <[email protected]> Signed-off-by: Borislav Petkov <[email protected]> Acked-by: Andy Lutomirski <[email protected]> Cc: [email protected] Link: https://lkml.kernel.org/r/[email protected]
1 parent 74b2fc8 commit 9bfecd0

File tree

4 files changed

+3
-74
lines changed

4 files changed

+3
-74
lines changed

arch/x86/include/asm/disabled-features.h

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,8 @@
5656
# define DISABLE_PTI (1 << (X86_FEATURE_PTI & 31))
5757
#endif
5858

59-
#ifdef CONFIG_IOMMU_SUPPORT
60-
# define DISABLE_ENQCMD 0
61-
#else
62-
# define DISABLE_ENQCMD (1 << (X86_FEATURE_ENQCMD & 31))
63-
#endif
59+
/* Force disable because it's broken beyond repair */
60+
#define DISABLE_ENQCMD (1 << (X86_FEATURE_ENQCMD & 31))
6461

6562
#ifdef CONFIG_X86_SGX
6663
# define DISABLE_SGX 0

arch/x86/include/asm/fpu/api.h

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -106,10 +106,6 @@ extern int cpu_has_xfeatures(u64 xfeatures_mask, const char **feature_name);
106106
*/
107107
#define PASID_DISABLED 0
108108

109-
#ifdef CONFIG_IOMMU_SUPPORT
110-
/* Update current's PASID MSR/state by mm's PASID. */
111-
void update_pasid(void);
112-
#else
113109
static inline void update_pasid(void) { }
114-
#endif
110+
115111
#endif /* _ASM_X86_FPU_API_H */

arch/x86/include/asm/fpu/internal.h

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -584,13 +584,6 @@ static inline void switch_fpu_finish(struct fpu *new_fpu)
584584
pkru_val = pk->pkru;
585585
}
586586
__write_pkru(pkru_val);
587-
588-
/*
589-
* Expensive PASID MSR write will be avoided in update_pasid() because
590-
* TIF_NEED_FPU_LOAD was set. And the PASID state won't be updated
591-
* unless it's different from mm->pasid to reduce overhead.
592-
*/
593-
update_pasid();
594587
}
595588

596589
#endif /* _ASM_X86_FPU_INTERNAL_H */

arch/x86/kernel/fpu/xstate.c

Lines changed: 0 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1402,60 +1402,3 @@ int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns,
14021402
return 0;
14031403
}
14041404
#endif /* CONFIG_PROC_PID_ARCH_STATUS */
1405-
1406-
#ifdef CONFIG_IOMMU_SUPPORT
1407-
void update_pasid(void)
1408-
{
1409-
u64 pasid_state;
1410-
u32 pasid;
1411-
1412-
if (!cpu_feature_enabled(X86_FEATURE_ENQCMD))
1413-
return;
1414-
1415-
if (!current->mm)
1416-
return;
1417-
1418-
pasid = READ_ONCE(current->mm->pasid);
1419-
/* Set the valid bit in the PASID MSR/state only for valid pasid. */
1420-
pasid_state = pasid == PASID_DISABLED ?
1421-
pasid : pasid | MSR_IA32_PASID_VALID;
1422-
1423-
/*
1424-
* No need to hold fregs_lock() since the task's fpstate won't
1425-
* be changed by others (e.g. ptrace) while the task is being
1426-
* switched to or is in IPI.
1427-
*/
1428-
if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
1429-
/* The MSR is active and can be directly updated. */
1430-
wrmsrl(MSR_IA32_PASID, pasid_state);
1431-
} else {
1432-
struct fpu *fpu = &current->thread.fpu;
1433-
struct ia32_pasid_state *ppasid_state;
1434-
struct xregs_state *xsave;
1435-
1436-
/*
1437-
* The CPU's xstate registers are not currently active. Just
1438-
* update the PASID state in the memory buffer here. The
1439-
* PASID MSR will be loaded when returning to user mode.
1440-
*/
1441-
xsave = &fpu->state.xsave;
1442-
xsave->header.xfeatures |= XFEATURE_MASK_PASID;
1443-
ppasid_state = get_xsave_addr(xsave, XFEATURE_PASID);
1444-
/*
1445-
* Since XFEATURE_MASK_PASID is set in xfeatures, ppasid_state
1446-
* won't be NULL and no need to check its value.
1447-
*
1448-
* Only update the task's PASID state when it's different
1449-
* from the mm's pasid.
1450-
*/
1451-
if (ppasid_state->pasid != pasid_state) {
1452-
/*
1453-
* Invalid fpregs so that state restoring will pick up
1454-
* the PASID state.
1455-
*/
1456-
__fpu_invalidate_fpregs_state(fpu);
1457-
ppasid_state->pasid = pasid_state;
1458-
}
1459-
}
1460-
}
1461-
#endif /* CONFIG_IOMMU_SUPPORT */

0 commit comments

Comments
 (0)