| 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: <e9271515f40ef1350cb8b365cc3665da9ea14f5e.camel@intel.com>
Date: Tue, 4 Oct 2022 22:13:51 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "keescook@...omium.org" <keescook@...omium.org>
CC: "bsingharora@...il.com" <bsingharora@...il.com>,
"hpa@...or.com" <hpa@...or.com>,
"Syromiatnikov, Eugene" <esyr@...hat.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"rdunlap@...radead.org" <rdunlap@...radead.org>,
"Yu, Yu-cheng" <yu-cheng.yu@...el.com>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"Eranian, Stephane" <eranian@...gle.com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"fweimer@...hat.com" <fweimer@...hat.com>,
"nadav.amit@...il.com" <nadav.amit@...il.com>,
"jannh@...gle.com" <jannh@...gle.com>,
"dethoma@...rosoft.com" <dethoma@...rosoft.com>,
"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
"kcc@...gle.com" <kcc@...gle.com>, "bp@...en8.de" <bp@...en8.de>,
"oleg@...hat.com" <oleg@...hat.com>,
"hjl.tools@...il.com" <hjl.tools@...il.com>,
"Yang, Weijiang" <weijiang.yang@...el.com>,
"Lutomirski, Andy" <luto@...nel.org>,
"pavel@....cz" <pavel@....cz>, "arnd@...db.de" <arnd@...db.de>,
"Moreira, Joao" <joao.moreira@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
"x86@...nel.org" <x86@...nel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>,
"john.allen@....com" <john.allen@....com>,
"rppt@...nel.org" <rppt@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>,
"Shankar, Ravi V" <ravi.v.shankar@...el.com>,
"corbet@....net" <corbet@....net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
"gorcunov@...il.com" <gorcunov@...il.com>
Subject: Re: [PATCH v2 26/39] x86/cet/shstk: Introduce routines modifying
shstk
On Mon, 2022-10-03 at 13:44 -0700, Kees Cook wrote:
> On Thu, Sep 29, 2022 at 03:29:23PM -0700, Rick Edgecombe wrote:
> > From: Yu-cheng Yu <yu-cheng.yu@...el.com>
> >
> > Shadow stack's are normally written to via CALL/RET or specific CET
> > instuctions like RSTORSSP/SAVEPREVSSP. However during some Linux
> > operations the kernel will need to write to directly using the
> > ring-0 only
> > WRUSS instruction.
> >
> > A shadow stack restore token marks a restore point of the shadow
> > stack, and
> > the address in a token must point directly above the token, which
> > is within
> > the same shadow stack. This is distinctively different from other
> > pointers
> > on the shadow stack, since those pointers point to executable code
> > area.
> >
> > Introduce token setup and verify routines. Also introduce WRUSS,
> > which is
> > a kernel-mode instruction but writes directly to user shadow stack.
> >
> > In future patches that enable shadow stack to work with signals,
> > the kernel
> > will need something to denote the point in the stack where
> > sigreturn may be
> > called. This will prevent attackers calling sigreturn at arbitrary
> > places
> > in the stack, in order to help prevent SROP attacks.
> >
> > To do this, something that can only be written by the kernel needs
> > to be
> > placed on the shadow stack. This can be accomplished by setting bit
> > 63 in
> > the frame written to the shadow stack. Userspace return addresses
> > can't
> > have this bit set as it is in the kernel range. It is also can't be
> > a
> > valid restore token.
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@...el.com>
> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> > Cc: Kees Cook <keescook@...omium.org>
> >
> > ---
> >
> > v2:
> > - Add data helpers for writing to shadow stack.
> >
> > v1:
> > - Use xsave helpers.
> >
> > Yu-cheng v30:
> > - Update commit log, remove description about signals.
> > - Update various comments.
> > - Remove variable 'ssp' init and adjust return value accordingly.
> > - Check get_user_shstk_addr() return value.
> > - Replace 'ia32' with 'proc32'.
> >
> > Yu-cheng v29:
> > - Update comments for the use of get_xsave_addr().
> >
> > arch/x86/include/asm/special_insns.h | 13 ++++
> > arch/x86/kernel/shstk.c | 108
> > +++++++++++++++++++++++++++
> > 2 files changed, 121 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/special_insns.h
> > b/arch/x86/include/asm/special_insns.h
> > index 35f709f619fb..f096f52bd059 100644
> > --- a/arch/x86/include/asm/special_insns.h
> > +++ b/arch/x86/include/asm/special_insns.h
> > @@ -223,6 +223,19 @@ static inline void clwb(volatile void *__p)
> > : [pax] "a" (p));
> > }
> >
> > +#ifdef CONFIG_X86_SHADOW_STACK
> > +static inline int write_user_shstk_64(u64 __user *addr, u64 val)
> > +{
> > + asm_volatile_goto("1: wrussq %[val], (%[addr])\n"
> > + _ASM_EXTABLE(1b, %l[fail])
> > + :: [addr] "r" (addr), [val] "r" (val)
> > + :: fail);
> > + return 0;
> > +fail:
> > + return -EFAULT;
> > +}
> > +#endif /* CONFIG_X86_SHADOW_STACK */
> > +
> > #define nop() asm volatile ("nop")
> >
> > static inline void serialize(void)
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index db4e53f9fdaf..8904aef487bf 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -25,6 +25,8 @@
> > #include <asm/fpu/api.h>
> > #include <asm/prctl.h>
> >
> > +#define SS_FRAME_SIZE 8
> > +
> > static bool feature_enabled(unsigned long features)
> > {
> > return current->thread.features & features;
> > @@ -40,6 +42,31 @@ static void feature_clr(unsigned long features)
> > current->thread.features &= ~features;
> > }
> >
> > +/*
> > + * Create a restore token on the shadow stack. A token is always
> > 8-byte
> > + * and aligned to 8.
> > + */
> > +static int create_rstor_token(unsigned long ssp, unsigned long
> > *token_addr)
> > +{
> > + unsigned long addr;
> > +
> > + /* Token must be aligned */
> > + if (!IS_ALIGNED(ssp, 8))
> > + return -EINVAL;
> > +
> > + addr = ssp - SS_FRAME_SIZE;
> > +
> > + /* Mark the token 64-bit */
> > + ssp |= BIT(0);
>
> Wow, that confused me for a moment. :) SDE says:
>
> - Bit 63:2 – Value of shadow stack pointer when this restore point
> was created.
> - Bit 1 – Reserved. Must be zero.
> - Bit 0 – Mode bit. If 0, the token is a compatibility/legacy mode
> “shadow stack restore” token. If 1, then this shadow stack
> restore
> token can be used with a RSTORSSP instruction in 64-bit
> mode.
>
> So shouldn't this actually be:
>
> ssp &= ~BIT(1); /* Reserved */
> ssp |= BIT(0); /* RSTORSSP instruction in 64-bit mode */
Since SSP is aligned, it should not have bits 0 to 2 set. I'll add a
comment.
>
> > +
> > + if (write_user_shstk_64((u64 __user *)addr, (u64)ssp))
> > + return -EFAULT;
> > +
> > + *token_addr = addr;
> > +
> > + return 0;
> > +}
> > +
> > static unsigned long alloc_shstk(unsigned long size)
> > {
> > int flags = MAP_ANONYMOUS | MAP_PRIVATE;
> > @@ -158,6 +185,87 @@ int shstk_alloc_thread_stack(struct
> > task_struct *tsk, unsigned long clone_flags,
> > return 0;
> > }
> >
> > +static unsigned long get_user_shstk_addr(void)
> > +{
> > + unsigned long long ssp;
> > +
> > + fpu_lock_and_load();
> > +
> > + rdmsrl(MSR_IA32_PL3_SSP, ssp);
> > +
> > + fpregs_unlock();
> > +
> > + return ssp;
> > +}
> > +
> > +static int put_shstk_data(u64 __user *addr, u64 data)
> > +{
> > + WARN_ON(data & BIT(63));
>
> Let's make this a bit more defensive:
>
> if (WARN_ON_ONCE(data & BIT(63)))
> return -EFAULT;
Hmm, sure. I'm thinking EINVAL since the failure has nothing to do with
faulting.
>
> > +
> > + /*
> > + * Mark the high bit so that the sigframe can't be processed as
> > a
> > + * return address.
> > + */
> > + if (write_user_shstk_64(addr, data | BIT(63)))
> > + return -EFAULT;
> > + return 0;
> > +}
> > +
> > +static int get_shstk_data(unsigned long *data, unsigned long
> > __user *addr)
> > +{
> > + unsigned long ldata;
> > +
> > + if (unlikely(get_user(ldata, addr)))
> > + return -EFAULT;
> > +
> > + if (!(ldata & BIT(63)))
> > + return -EINVAL;
> > +
> > + *data = ldata & ~BIT(63);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Verify the user shadow stack has a valid token on it, and then
> > set
> > + * *new_ssp according to the token.
> > + */
> > +static int shstk_check_rstor_token(unsigned long *new_ssp)
> > +{
> > + unsigned long token_addr;
> > + unsigned long token;
> > +
> > + token_addr = get_user_shstk_addr();
> > + if (!token_addr)
> > + return -EINVAL;
> > +
> > + if (get_user(token, (unsigned long __user *)token_addr))
> > + return -EFAULT;
> > +
> > + /* Is mode flag correct? */
> > + if (!(token & BIT(0)))
> > + return -EINVAL;
> > +
> > + /* Is busy flag set? */
>
> "Busy"? Not "Reserved"?
Yes reserved is more accurate, I'll change it. In a previous-ssp token
it is 1, so kind of busy-like. That is probably where the comment came
from.
>
> > + if (token & BIT(1))
> > + return -EINVAL;
> > +
> > + /* Mask out flags */
> > + token &= ~3UL;
> > +
> > + /* Restore address aligned? */
> > + if (!IS_ALIGNED(token, 8))
> > + return -EINVAL;
> > +
> > + /* Token placed properly? */
> > + if (((ALIGN_DOWN(token, 8) - 8) != token_addr) || token >=
> > TASK_SIZE_MAX)
> > + return -EINVAL;
> > +
> > + *new_ssp = token;
> > +
> > + return 0;
> > +}
> > +
> > void shstk_free(struct task_struct *tsk)
> > {
> > struct thread_shstk *shstk = &tsk->thread.shstk;
> > --
> > 2.17.1
> >
>
>
Powered by blists - more mailing lists