Messages in this thread | | | Subject | Re: [PATCH v26 24/30] x86/cet/shstk: Introduce shadow stack token setup/verify routines | From | "Yu, Yu-cheng" <> | Date | Mon, 17 May 2021 13:55:01 -0700 |
| |
On 5/17/2021 12:45 AM, Borislav Petkov wrote: > On Tue, Apr 27, 2021 at 01:43:09PM -0700, Yu-cheng Yu wrote: >> +static inline int write_user_shstk_32(u32 __user *addr, u32 val) >> +{ >> + WARN_ONCE(1, "%s used but not supported.\n", __func__); >> + return -EFAULT; >> +} >> +#endif > > What is that supposed to catch? Any concrete (mis-)use cases? >
If 32-bit apps are not supported, there should be no need of 32-bit shadow stack write, otherwise there is a bug.
[...]
>> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c >> index d387df84b7f1..48a0c87414ef 100644 >> --- a/arch/x86/kernel/shstk.c >> +++ b/arch/x86/kernel/shstk.c >> @@ -20,6 +20,7 @@ >> #include <asm/fpu/xstate.h> >> #include <asm/fpu/types.h> >> #include <asm/cet.h> >> +#include <asm/special_insns.h> >> >> static void start_update_msrs(void) >> { >> @@ -176,3 +177,128 @@ void shstk_disable(void) >> >> shstk_free(current); >> } >> + >> +static unsigned long _get_user_shstk_addr(void) > > What's the "_" prefix in the name supposed to denote? > > Ditto for the other functions with "_" prefix you're adding. >
These are static functions. I thought that would make the static scope clear. I can remove "_".
>> +{ >> + struct fpu *fpu = ¤t->thread.fpu; >> + unsigned long ssp = 0; >> + >> + fpregs_lock(); >> + >> + if (fpregs_state_valid(fpu, smp_processor_id())) { >> + rdmsrl(MSR_IA32_PL3_SSP, ssp); >> + } else { >> + struct cet_user_state *p; >> + >> + p = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >> + if (p) >> + ssp = p->user_ssp; >> + } >> + >> + fpregs_unlock(); > > <---- newline here. > >> + return ssp; >> +} >> + >> +#define TOKEN_MODE_MASK 3UL >> +#define TOKEN_MODE_64 1UL >> +#define IS_TOKEN_64(token) (((token) & TOKEN_MODE_MASK) == TOKEN_MODE_64) >> +#define IS_TOKEN_32(token) (((token) & TOKEN_MODE_MASK) == 0) > > Why do you have to look at the second, busy bit, too in order to > determine the mode? >
If the busy bit is set, it is only for SAVEPREVSSP, and invalid as a normal restore token.
> Also, you don't need most of those defines - see below. > >> +/* >> + * Create a restore token on the shadow stack. A token is always 8-byte >> + * and aligned to 8. >> + */ >> +static int _create_rstor_token(bool ia32, unsigned long ssp, >> + unsigned long *token_addr) >> +{ >> + unsigned long addr; >> + >> + *token_addr = 0; > > What for? Callers should check this function's retval and then interpret > the validity of token_addr and it should not unconditionally write into > it. >
Ok.
>> + >> + if ((!ia32 && !IS_ALIGNED(ssp, 8)) || !IS_ALIGNED(ssp, 4)) > > Flip this logic: > > if ((ia32 && !IS_ALIGNED(ssp, 4)) || !IS_ALIGNED(ssp, 8)) > >> + return -EINVAL; >> + >> + addr = ALIGN_DOWN(ssp, 8) - 8; > > Yah, so this is weird. Why does the restore token need to be at -8 > instead on the shadow stack address itself?
With the lower two bits masked out, the restore token must point directly above itself.
> > Looking at > > Figure 18-2. RSTORSSP to Switch to New Shadow Stack > Figure 18-3. SAVEPREVSSP to Save a Restore Point > > in the SDM, it looks like unnecessarily more complex than it should be. > But maybe there's some magic I'm missing. > >> + >> + /* Is the token for 64-bit? */ >> + if (!ia32) >> + ssp |= TOKEN_MODE_64; > > |= BIT(0); >
Ok, then, we don't use #define's. I will put in comments about what it is doing, and fix the rest.
Thanks, Yu-cheng
| |