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.
...
> 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...
> + 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
