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:
> > Subject: Re: [PATCH v5 07/39] x86: Add user control-protection
> > fault handler
>
> Subject: x86/shstk: Add...
Sure.
>
> > From: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> >
> > 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.
>
> ...
>
> > 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.
>
> > +
> > + CP_RET = 1,
> > + CP_IRET = 2,
> > + CP_ENDBR = 3,
> > + CP_RSTRORSSP = 4,
> > + CP_SETSSBSY = 5,
> > +
> > + CP_ENCL = 1 << 15,
> > +};
>
> ...
>
> > +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.
>
> > + 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);
> > +}
>
>
