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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/11/2024 6:54 pm, Xin Li wrote:
> 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?

After discussing with Dave on IRC, the ibt_clear_fred_wfe(regs); really
needs to be inside the ibt_selftest_noendbr path.

It's a selftest where we're deliberately trying to trigger #CP, and in
the one case where we're happy should we say "yeah, safe to clobber WFE
in the interrupted context" to let execution continue.

Clobbering WFE in any other circumstance is a security-relevant bug.

~Andrew