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

[Borislav Petkov]

On Mon, May 17, 2021 at 01:55:01PM -0700, Yu, Yu-cheng wrote:
> If 32-bit apps are not supported, there should be no need of 32-bit shadow
> stack write, otherwise there is a bug.

Aha, just a precaution. Then you can reduce the ifdeffery a bit (ontop
of yours):

---
diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index f962da1fe9b5..5b48c91fa8d4 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -235,9 +235,14 @@ static inline void clwb(volatile void *__p)
 }
 
 #ifdef CONFIG_X86_SHADOW_STACK
-#if defined(CONFIG_IA32_EMULATION) || defined(CONFIG_X86_X32)
 static inline int write_user_shstk_32(u32 __user *addr, u32 val)
 {
+	if (WARN_ONCE(!IS_ENABLED(CONFIG_IA32_EMULATION) &&
+		      !IS_ENABLED(CONFIG_X86_X32),
+		      "%s used but not supported.\n", __func__)) {
+		return -EFAULT;
+	}
+
 	asm_volatile_goto("1: wrussd %[val], (%[addr])\n"
 			  _ASM_EXTABLE(1b, %l[fail])
 			  :: [addr] "r" (addr), [val] "r" (val)
@@ -246,13 +251,6 @@ static inline int write_user_shstk_32(u32 __user *addr, u32 val)
 fail:
 	return -EFAULT;
 }
-#else
-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
 
 static inline int write_user_shstk_64(u64 __user *addr, u64 val)
 {

> These are static functions.  I thought that would make the static scope
> clear.  I can remove "_".

No, "_" or "__" prefixed functions are generally supposed to denote
internal interfaces which should not be used by other kernel facilities.
In that case you have the external api <function_name> and the lower
level helpers _<function_name>, __<function_name>, etc. They can be
static but not necessarily.

This is not the case here so you can simply drop the "_" prefixes.

> If the busy bit is set, it is only for SAVEPREVSSP, and invalid as a
> normal restore token.

Sure but the busy bit is independent from the mode.

> With the lower two bits masked out, the restore token must point
> directly above itself.

That I know - I'm just questioning the design. It should be

	addr = ALIGN_DOWN(ssp, 8);

Plain and simple.

Not this silly pushing and popping of stuff. But it is too late now
anyway and it's not like hw people talk to software people who get to
implement their shit.

> Ok, then, we don't use #define's. I will put in comments about what it
> is doing, and fix the rest.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette