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
