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

[Edgecombe, Rick P]

On Wed, 2022-10-05 at 01:20 +0000, Andrew Cooper wrote:
> On 29/09/2022 23:29, Rick Edgecombe wrote:
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index d62b2cb85cea..b7dde8730236 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -229,16 +223,74 @@ enum cp_error_code {
> >  	CP_ENCL	     = 1 << 15,
> >  };
> >  
> > -DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> > +#ifdef CONFIG_X86_SHADOW_STACK
> > +static const char * const control_protection_err[] = {
> > +	"unknown",
> > +	"near-ret",
> > +	"far-ret/iret",
> > +	"endbranch",
> > +	"rstorssp",
> > +	"setssbsy",
> > +};
> 
> These are a mix of SHSTK and IBT errors.  They should be inside
> CONFIG_X86_CET using Kees' suggestion.
> 
> Also, if you express this as
> 
> static const char errors[][10] = {
>     [0] = "unknown",
>     [1] = "near ret",
>     [2] = "far/iret",
>     [3] = "endbranch",
>     [4] = "rstorssp",
>     [5] = "setssbsy",
> };
> 
> then you can encode all the strings in roughly the space it takes to
> lay
> out the pointers above.

It is only used in the user shadow stack side of the handler. I guess
the kernel IBT side of the handler could print these out too.

Can you explain more about why this array is better than the other one?

> 
> > +
> > +static DEFINE_RATELIMIT_STATE(cpf_rate,
> > DEFAULT_RATELIMIT_INTERVAL,
> > +			      DEFAULT_RATELIMIT_BURST);
> > +
> > +static void do_user_control_protection_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;
> > +
> > +	/* Read SSP before enabling interrupts. */
> > +	rdmsrl(MSR_IA32_PL3_SSP, ssp);
> > +
> > +	cond_local_irq_enable(regs);
> > +
> > +	if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > +		WARN_ONCE(1, "User-mode control protection fault with
> > shadow support disabled\n");
> 
> So it's ok to get an unexpected #CP on CET-capable hardware, but not
> on
> CET-incapable hardware?
> 
> The conditions for this WARN() (and others) probably want adjusting
> to
> what the kernel has enabled, not what hardware is capable of.

Sorry, I don't follow. This code is only compiled in if the kernel has
been compiled for userspace shadow stacks. If the HW supports it and
the kernel is configured for it, it should be enabled. If you clear it
with the clearcpuid command line it should be as if the HW doesn't
support it. So I think it should not be too unexpected, in situations
where it gets passed this check.

> 
> > @@ -283,9 +335,29 @@ static int __init ibt_setup(char *str)
> >  }
> >  
> >  __setup("ibt=", ibt_setup);
> > -
> > +#else
> > +static void do_kernel_control_protection_fault(struct pt_regs
> > *regs)
> > +{
> > +	WARN_ONCE(1, "Kernel-mode control protection fault with IBT
> > disabled\n");
> > +}
> >  #endif /* CONFIG_X86_KERNEL_IBT */
> >  
> > +#if defined(CONFIG_X86_KERNEL_IBT) ||
> > defined(CONFIG_X86_SHADOW_STACK)
> > +DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> > +{
> > +	if (!cpu_feature_enabled(X86_FEATURE_IBT) &&
> > +	    !cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> > +		pr_err("Unexpected #CP\n");
> 
> Do some future poor sole a favour and render the numeric error code
> too.  Without it, the error is ambiguous between SHSTK and IBT when
> %rip
> points at a call/ret instruction.
> 

This was from the original kernel IBT handler. Yes, all these messages
should probably be unified too. Thanks.