Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs

[Xin Li <xin@zytor.com>]

On 9/16/2024 11:10 AM, Xin Li (Intel) wrote:
> The WFE, i.e., WAIT_FOR_ENDBRANCH, bit in the augmented CS of FRED
> stack frame is set to 1 in missing-ENDBRANCH #CP exceptions.
> 
> The CPU will generate another missing-ENDBRANCH #CP if the WFE bit
> is left set, because the CPU IBT will be set in the WFE state upon
> completion of the following ERETS instruction and then the CPU will
> resume from the instruction that just caused the previous #CP.
> 
> Clear WFE to avoid dead looping in missing-ENDBRANCH #CPs.
> 
> Describe the IBT story in the comment of ibt_clear_fred_wfe() using
> Andrew Cooper's write-up.
> 
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> 
> Changes since v1:
> * Rewrite the comment of ibt_clear_fred_wfe() using Andrew Cooper's
>    write-up (Andrew Cooper).
> * Unconditionally clear WFE in missing-ENDBRANCH #CPs (Peter Zijlstra).
> * Don't check X86_FEATURE_FRED in ibt_clear_fred_wfe() (Dave Hansen &
>    Andrew Cooper).
> ---
>   arch/x86/kernel/cet.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/arch/x86/kernel/cet.c b/arch/x86/kernel/cet.c
> index d2c732a34e5d..d2cf6ee0d9a0 100644
> --- a/arch/x86/kernel/cet.c
> +++ b/arch/x86/kernel/cet.c
> @@ -81,6 +81,36 @@ static void do_user_cp_fault(struct pt_regs *regs, unsigned long error_code)
>   
>   static __ro_after_init bool ibt_fatal = true;
>   
> +/*
> + * By definition, all missing-ENDBRANCH #CPs are a result of WFE && !ENDBR.
> + *
> + * But, in original CET under IDT delivery, any transfer for
> + * interrupt/exception/etc that does not change privilege will clobber the
> + * WFE state because MSR_{U,S}_CET.WFE is intentionally set by microcode so
> + * as to expect to find an ENDBR at the interrupt/exception/syscall entrypoint.
> + *
> + * In practice, this means interrupts and exceptions hitting the kernel, or
> + * user interrupts, lose the WFE state of the interrupted context.  And
> + * yes, this means that a well timed interrupt (to the precise instruction
> + * boundary) will let an attacker sneak a bad function pointer past the
> + * CET-IBT enforcement.
> + *
> + * In FRED, the WFE state of the interrupted context (even if it is the
> + * same privilege) is preserved and restored, in order to close this hole.
> + *
> + * Therefore, the missing-ENDBRANCH #CP handler needs to clear WFE to avoid
> + * dead looping, now that FRED is causing the state not to get lost.  Otherwise
> + * if the WFE bit is left set in an intentional ibt selftest or when !ibt_fatal,
> + * the CPU will generate another missing-ENDBRANCH #CP because the IBT will be
> + * set in the WFE state upon completion of the following ERETS instruction and
> + * then the CPU will resume from the instruction that just caused the previous
> + * missing-ENDBRANCH #CP.
> + */
> +static void ibt_clear_fred_wfe(struct pt_regs *regs)
> +{
> +	regs->fred_cs.wfe = 0;
> +}
> +
>   static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
>   {
>   	if ((error_code & CP_EC) != CP_ENDBR) {
> @@ -88,6 +118,8 @@ static void do_kernel_cp_fault(struct pt_regs *regs, unsigned long error_code)
>   		return;
>   	}
>   
> +	ibt_clear_fred_wfe(regs);
> +
>   	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_noendbr)) {
>   		regs->ax = 0;
>   		return;
> 
> base-commit: fe85ee391966c4cf3bfe1c405314e894c951f521


Andrew,

can you please take another look?

If there is no obvious problem, please give it a review-by?

Thanks!
     Xin