Re: [PATCH v26 24/30] x86/cet/shstk: Introduce shadow stack token setup/verify routines

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 = &current->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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux