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

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Tue, 2022-12-20 at 17:19 +0100, Borislav Petkov wrote:
> On Fri, Dec 02, 2022 at 04:35:34PM -0800, Rick Edgecombe wrote:
> > From: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > 
> > 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.
> > 
> > There already exists a control-protection fault handler for
> > handling kernel
> > IBT. Refactor this fault handler into separate user and kernel
> > handlers,
> 
> Unknown word [separate] in commit message.
> Suggestions: ['separate', 
> 
> You could use a spellchecker with your commit messages so that it
> catches all those typos.

Argh, sorry. I'll upgrade my tools.

> 
> ...
> 
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 8b83d8fbce71..e35c70dc1afb 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -213,12 +213,7 @@ DEFINE_IDTENTRY(exc_overflow)
> >         do_error_trap(regs, 0, "overflow", X86_TRAP_OF, SIGSEGV, 0,
> > NULL);
> >  }
> >  
> > -#ifdef CONFIG_X86_KERNEL_IBT
> > -
> > -static __ro_after_init bool ibt_fatal = true;
> > -
> > -extern void ibt_selftest_ip(void); /* code label defined in asm
> > below */
> > -
> > +#ifdef CONFIG_X86_CET
> >  enum cp_error_code {
> >         CP_EC        = (1 << 15) - 1,
> >  
> > @@ -231,15 +226,87 @@ enum cp_error_code {
> >         CP_ENCL      = 1 << 15,
> >  };
> >  
> > -DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> > +static const char control_protection_err[][10] = {
> 
> You already use the "cp_" prefix for the other things, might as well
> use
> it here too.

Sure.

> 
> > +       [0] = "unknown",
> > +       [1] = "near ret",
> > +       [2] = "far/iret",
> > +       [3] = "endbranch",
> > +       [4] = "rstorssp",
> > +       [5] = "setssbsy",
> > +};
> > +
> > +static const char *cp_err_string(unsigned long error_code)
> > +{
> > +       unsigned int cpec = error_code & CP_EC;
> > +
> > +       if (cpec >= ARRAY_SIZE(control_protection_err))
> > +               cpec = 0;
> > +       return control_protection_err[cpec];
> > +}
> > +
> > +static void do_unexpected_cp(struct pt_regs *regs, unsigned long
> > error_code)
> > +{
> > +       WARN_ONCE(1, "Unexpected %s #CP, error_code: %s\n",
> > +                    user_mode(regs) ? "user mode" : "kernel mode",
> > +                    cp_err_string(error_code));
> > +}
> > +#endif /* CONFIG_X86_CET */
> > +
> > +void do_user_cp_fault(struct pt_regs *regs, unsigned long
> > error_code);
> 
> What's that forward declaration for?

The reason is cpu_feature_enabled(X86_FEATURE_USER_SHSTK) will compile-
time evaluate to false when user shadow stack is not configured, so a
do_user_cp_fault() implementation is not needed in that case. The
reference in exec_control_protection will get optimized out, but it
still needs to be defined.

Otherwise it could have a stub implementation in an #else block, but
this seemed cleaner.

> 
> In any case, push it into traps.h pls.

It's not really supposed to be called from outside traps.c. The only
reason it is not static it because it doesn't work with these
IS_ENABLED()-style solutions for compiling out code. Now I'm wondering
if these are not preferred...

> 
> I gotta say, I'm not a big fan of that ifdeffery here. Do we really
> really need it?

You mean having separate paths for kernel IBT and user shadow stack
that compile out? I guess it could just all be in place if
CONFIG_X86_CET is in place.

I don't know, I thought it was relatively clean, but I can remove it.

> 
> > +#ifdef CONFIG_X86_USER_SHADOW_STACK
> > +static DEFINE_RATELIMIT_STATE(cpf_rate,
> > DEFAULT_RATELIMIT_INTERVAL,
> > +                             DEFAULT_RATELIMIT_BURST);
> > +
> > +void do_user_cp_fault(struct pt_regs *regs, unsigned long
> > error_code)
> >  {
> > -       if (!cpu_feature_enabled(X86_FEATURE_IBT)) {
> > -               pr_err("Unexpected #CP\n");
> > -               BUG();
> > +       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;
> > +       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");
> >         }
> >  
> > -       if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) !=
> > CP_ENDBR))
> > +       force_sig_fault(SIGSEGV, SEGV_CPERR, (void __user *)0);
> > +       cond_local_irq_disable(regs);
> > +}
> > +#endif
> > +
> > +void do_kernel_cp_fault(struct pt_regs *regs, unsigned long
> > error_code);
> > +
> > +#ifdef CONFIG_X86_KERNEL_IBT
> > +static __ro_after_init bool ibt_fatal = true;
> > +
> > +extern void ibt_selftest_ip(void); /* code label defined in asm
> > below */
> 
> Yeah, pls put that comment above the function. Side comments are
> nasty.
> 
> Thx.
> 
Sure. Thanks.