On Fri, 2023-02-03 at 20:09 +0100, Borislav Petkov wrote:

On Thu, Jan 19, 2023 at 01:22:45PM -0800, Rick Edgecombe wrote:

quoted

Subject: Re: [PATCH v5 07/39] x86: Add user control-protection
fault handler

Subject: x86/shstk: Add...

Sure.

quoted

From: Yu-cheng Yu <redacted>

A control-protection fault is triggered when a control-flow
transfer
attempt violates Shadow Stack or Indirect Branch Tracking
constraints.
For example, the return address for a RET instruction differs from
the copy
on the shadow stack.

...

quoted

diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
new file mode 100644
index 000000000000..33d7d119be26
--- /dev/null
+++ b/arch/x86/kernel/cet.c

@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ptrace.h>
+#include <asm/bugs.h>
+#include <asm/traps.h>
+
+enum cp_error_code {
+	CP_EC        = (1 << 15) - 1,

That looks like a mask, so

	CP_EC_MASK

I guess.

The name seems better, but this is actually from the existing kernel
IBT control protection exception code. So it seems like an separate
change. Would you like to see it snuck into the user shadow stack
handler, or could we leave this for future cleanups?

Kees pointed out that adding to the handler and moving it in the same
patch makes it difficult to see where the changes are. I'm splitting
this one into two patches for the next version.

quoted

+
+	CP_RET       = 1,
+	CP_IRET      = 2,
+	CP_ENDBR     = 3,
+	CP_RSTRORSSP = 4,
+	CP_SETSSBSY  = 5,
+
+	CP_ENCL	     = 1 << 15,
+};

...

quoted

+static void do_user_cp_fault(struct pt_regs *regs, unsigned long
error_code)
+{
+	struct task_struct *tsk;
+	unsigned long ssp;
+
+	/*
+	 * An exception was just taken from userspace. Since interrupts
are disabled
+	 * here, no scheduling should have messed with the registers
yet and they
+	 * will be whatever is live in userspace. So read the SSP
before enabling
+	 * interrupts so locking the fpregs to do it later is not
required.
+	 */
+	rdmsrl(MSR_IA32_PL3_SSP, ssp);
+
+	cond_local_irq_enable(regs);
+
+	tsk = current;

Hmm, should you read current before you enable interrupts? Not that
it
changes from under us...

I think we have to read it before we enable interrupts or use
fpregs_lock(). So reading it before saves disabling preemption later.

quoted

+	tsk->thread.error_code = error_code;
+	tsk->thread.trap_nr = X86_TRAP_CP;
+
+	/* Ratelimit to prevent log spamming. */
+	if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
+	    __ratelimit(&cpf_rate)) {
+		pr_emerg("%s[%d] control protection ip:%lx sp:%lx
ssp:%lx error:%lx(%s)%s",
+			 tsk->comm, task_pid_nr(tsk),
+			 regs->ip, regs->sp, ssp, error_code,
+			 cp_err_string(error_code),
+			 error_code & CP_ENCL ? " in enclave" : "");
+		print_vma_addr(KERN_CONT " in ", regs->ip);
+		pr_cont("\n");
+	}
+
+	force_sig_fault(SIGSEGV, SEGV_CPERR, (void __user *)0);
+	cond_local_irq_disable(regs);
+}