Re: [PATCH v7 34/41] x86/shstk: Support WRSS for userspace
From: Borislav Petkov <bp@alien8.de>
Date: 2023-03-10 16:48:01
Also in:
linux-api, linux-arch, linux-mm, lkml
Subsystem:
the rest, x86 architecture (32-bit and 64-bit) · Maintainers:
Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen
On Mon, Feb 27, 2023 at 02:29:50PM -0800, Rick Edgecombe wrote:
For the current shadow stack implementation, shadow stacks contents can't easily be provisioned with arbitrary data. This property helps apps protect themselves better, but also restricts any potential apps that may want to do exotic things at the expense of a little security. The x86 shadow stack feature introduces a new instruction, WRSS, which can be enabled to write directly to shadow stack permissioned memory from
s/permissioned // By now it is clear that shadow stack memory is a special thing anyway.
userspace. Allow it to get enabled via the prctl interface. Only enable the userspace WRSS instruction, which allows writes to userspace shadow stacks from userspace. Do not allow it to be enabled independently of shadow stack, as HW does not support using WRSS when shadow stack is disabled. From a fault handler perspective, WRSS will behave very similar to WRUSS, which is treated like a user access from a #PF err code perspective.
...
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h index 65ec1965cd28..2d3b35c957ad 100644 --- a/arch/x86/include/asm/msr.h +++ b/arch/x86/include/asm/msr.h@@ -310,6 +310,17 @@ void msrs_free(struct msr *msrs); int msr_set_bit(u32 msr, u8 bit); int msr_clear_bit(u32 msr, u8 bit); +/* Helper that can never get accidentally un-inlined. */ +#define set_clr_bits_msrl(msr, set, clear) do { \
Uff, pls kill this thing. Our MSR interfaces universe is already insane and arch/x86/lib/msr.c already has similar attempts to what you're doing here in addition to all the other gunk in msr.h. I highly doubt this can't be done the usual way, lemme see...
quoted hunk ↗ jump to hunk
+ u64 __val, __new_val, __msr = msr; \ + \ + rdmsrl(__msr, __val); \ + __new_val = (__val & ~(clear)) | (set); \ + \ + if (__new_val != __val) \ + wrmsrl(__msr, __new_val); \ +} while (0) + #ifdef CONFIG_SMP int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h); int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h index 7dfd9dc00509..e31495668056 100644 --- a/arch/x86/include/uapi/asm/prctl.h +++ b/arch/x86/include/uapi/asm/prctl.h@@ -28,5 +28,6 @@ /* ARCH_SHSTK_ features bits */ #define ARCH_SHSTK_SHSTK (1ULL << 0) +#define ARCH_SHSTK_WRSS (1ULL << 1) #endif /* _ASM_X86_PRCTL_H */diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index 0a3decab70ee..009cb3fa0ae5 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c@@ -363,6 +363,36 @@ void shstk_free(struct task_struct *tsk) unmap_shadow_stack(shstk->base, shstk->size); } +static int wrss_control(bool enable) +{ + if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK)) + return -EOPNOTSUPP; + + /* + * Only enable wrss if shadow stack is enabled. If shadow stack is not
"WRSS". Insns in all caps pls.
+ * enabled, wrss will already be disabled, so don't bother clearing it
Ditto.
+ * when disabling.
+ */
+ if (!features_enabled(ARCH_SHSTK_SHSTK))
+ return -EPERM;
+
+ /* Already enabled/disabled? */
+ if (features_enabled(ARCH_SHSTK_WRSS) == enable)
+ return 0;
+
+ fpregs_lock_and_load();
+ if (enable) {
+ set_clr_bits_msrl(MSR_IA32_U_CET, CET_WRSS_EN, 0);
+ features_set(ARCH_SHSTK_WRSS);
+ } else {
+ set_clr_bits_msrl(MSR_IA32_U_CET, 0, CET_WRSS_EN);
+ features_clr(ARCH_SHSTK_WRSS);
+ }
+ fpregs_unlock();Yes, doing it the "usual" way is more readable because it is a common code pattern which one encounters all around arch/x86/. Diff ontop:
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 009cb3fa0ae5..914feff26b23 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c@@ -365,6 +365,8 @@ void shstk_free(struct task_struct *tsk) static int wrss_control(bool enable) { + u64 msrval; + if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK)) return -EOPNOTSUPP;
@@ -381,13 +383,22 @@ static int wrss_control(bool enable) return 0; fpregs_lock_and_load(); + rdmsrl(MSR_IA32_U_CET, msrval); + if (enable) { - set_clr_bits_msrl(MSR_IA32_U_CET, CET_WRSS_EN, 0); features_set(ARCH_SHSTK_WRSS); + msrval |= CET_WRSS_EN; } else { - set_clr_bits_msrl(MSR_IA32_U_CET, 0, CET_WRSS_EN); features_clr(ARCH_SHSTK_WRSS); + if (!(msrval & CET_WRSS_EN)) + goto unlock; + + msrval &= ~CET_WRSS_EN; } + + wrmsrl(MSR_IA32_U_CET, msrval); + +unlock: fpregs_unlock(); return 0;
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette