| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZAtehgwB5/jGL9dC@zn.tnic>
Date: Fri, 10 Mar 2023 17:44:54 +0100
From: Borislav Petkov <bp@...en8.de>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc: x86@...nel.org, "H . Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-mm@...ck.org,
linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Andy Lutomirski <luto@...nel.org>,
Balbir Singh <bsingharora@...il.com>,
Cyrill Gorcunov <gorcunov@...il.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Eugene Syromiatnikov <esyr@...hat.com>,
Florian Weimer <fweimer@...hat.com>,
"H . J . Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
Jonathan Corbet <corbet@....net>,
Kees Cook <keescook@...omium.org>,
Mike Kravetz <mike.kravetz@...cle.com>,
Nadav Amit <nadav.amit@...il.com>,
Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
Peter Zijlstra <peterz@...radead.org>,
Randy Dunlap <rdunlap@...radead.org>,
Weijiang Yang <weijiang.yang@...el.com>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
John Allen <john.allen@....com>, kcc@...gle.com,
eranian@...gle.com, rppt@...nel.org, jamorris@...ux.microsoft.com,
dethoma@...rosoft.com, akpm@...ux-foundation.org,
Andrew.Cooper3@...rix.com, christina.schimpe@...el.com,
david@...hat.com, debug@...osinc.com
Subject: Re: [PATCH v7 34/41] x86/shstk: Support WRSS for userspace
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
Powered by blists - more mailing lists