[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <49b6c23b-dfd8-4874-bd6e-998dd039ed1a@zytor.com>
Date: Thu, 7 Nov 2024 10:54:23 -0800
From: Xin Li <xin@...or.com>
To: linux-kernel@...r.kernel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
peterz@...radead.org, andrew.cooper3@...rix.com
Subject: Re: [PATCH v2 1/1] x86/fred: Clear WFE in missing-ENDBRANCH #CPs
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@...or.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
Powered by blists - more mailing lists