Re: [PATCH v6 27/33] x86/fred: fixup fault on ERETU by jumping to fred_entrypoint_user

[Lai Jiangshan]

On Mon, Mar 27, 2023 at 4:24 PM Xin Li <xin3.li@xxxxxxxxx> wrote:
>
> If the stack frame contains an invalid user context (e.g. due to invalid SS,
> a non-canonical RIP, etc.) the ERETU instruction will trap (#SS or #GP).
>
> From a Linux point of view, this really should be considered a user space
> failure, so use the standard fault fixup mechanism to intercept the fault,
> fix up the exception frame, and redirect execution to fred_entrypoint_user.
> The end result is that it appears just as if the hardware had taken the
> exception immediately after completing the transition to user space.
>
> Suggested-by: H. Peter Anvin (Intel) <hpa@xxxxxxxxx>
> Tested-by: Shan Kang <shan.kang@xxxxxxxxx>
> Signed-off-by: Xin Li <xin3.li@xxxxxxxxx>
> ---
>
> Changes since v5:
> * Move the NMI bit from an invalid stack frame, which caused ERETU to fault,
>   to the fault handler's stack frame, thus to unblock NMI ASAP if NMI is blocked
>   (Lai Jiangshan).
> ---
>  arch/x86/entry/entry_64_fred.S             |  8 +++--
>  arch/x86/include/asm/extable_fixup_types.h |  4 ++-
>  arch/x86/mm/extable.c                      | 36 ++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64_fred.S b/arch/x86/entry/entry_64_fred.S
> index d975cacd060f..efe2bcd11273 100644
> --- a/arch/x86/entry/entry_64_fred.S
> +++ b/arch/x86/entry/entry_64_fred.S
> @@ -5,8 +5,10 @@
>   * The actual FRED entry points.
>   */
>  #include <linux/linkage.h>
> -#include <asm/errno.h>
> +#include <asm/asm.h>
>  #include <asm/asm-offsets.h>
> +#include <asm/errno.h>
> +#include <asm/export.h>
>  #include <asm/fred.h>
>
>  #include "calling.h"
> @@ -38,7 +40,9 @@ SYM_CODE_START_NOALIGN(fred_entrypoint_user)
>         call    fred_entry_from_user
>  SYM_INNER_LABEL(fred_exit_user, SYM_L_GLOBAL)
>         FRED_EXIT
> -       ERETU
> +1:     ERETU
> +
> +       _ASM_EXTABLE_TYPE(1b, fred_entrypoint_user, EX_TYPE_ERETU)
>  SYM_CODE_END(fred_entrypoint_user)
>
>  .fill fred_entrypoint_kernel - ., 1, 0xcc
> diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
> index 991e31cfde94..1585c798a02f 100644
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -64,6 +64,8 @@
>  #define        EX_TYPE_UCOPY_LEN4              (EX_TYPE_UCOPY_LEN | EX_DATA_IMM(4))
>  #define        EX_TYPE_UCOPY_LEN8              (EX_TYPE_UCOPY_LEN | EX_DATA_IMM(8))
>
> -#define EX_TYPE_ZEROPAD                        20 /* longword load with zeropad on fault */
> +#define        EX_TYPE_ZEROPAD                 20 /* longword load with zeropad on fault */
> +
> +#define        EX_TYPE_ERETU                   21
>
>  #endif
> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> index 60814e110a54..a5d75b27a993 100644
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -6,6 +6,7 @@
>  #include <xen/xen.h>
>
>  #include <asm/fpu/api.h>
> +#include <asm/fred.h>
>  #include <asm/sev.h>
>  #include <asm/traps.h>
>  #include <asm/kdebug.h>
> @@ -195,6 +196,37 @@ static bool ex_handler_ucopy_len(const struct exception_table_entry *fixup,
>         return ex_handler_uaccess(fixup, regs, trapnr);
>  }
>
> +#ifdef CONFIG_X86_FRED
> +static bool ex_handler_eretu(const struct exception_table_entry *fixup,
> +                            struct pt_regs *regs, unsigned long error_code)
> +{
> +       struct pt_regs *uregs = (struct pt_regs *)(regs->sp - offsetof(struct pt_regs, ip));
> +       unsigned short ss = uregs->ss;
> +       unsigned short cs = uregs->cs;
> +
> +       /*
> +        * Move the NMI bit from the invalid stack frame, which caused ERETU
> +        * to fault, to the fault handler's stack frame, thus to unblock NMI
> +        * with the fault handler's ERETS instruction ASAP if NMI is blocked.
> +        */
> +       regs->nmi = uregs->nmi;
> +
> +       fred_info(uregs)->edata = fred_event_data(regs);
> +       uregs->ssx = regs->ssx;
> +       uregs->ss = ss;
> +       uregs->csx = regs->csx;
> +       uregs->nmi = 0; /* The NMI bit was moved away above */
> +       uregs->current_stack_level = 0;
> +       uregs->cs = cs;
> +
> +       /* Copy error code to uregs and adjust stack pointer accordingly */
> +       uregs->orig_ax = error_code;

The address of uregs->orig_ax is below regs->sp, so I think
some comments are needed here to state why it is safe to
write to uregs->orig_ax (a.k.a it is not verlapped with regs).



Thanks
Lai

> +       regs->sp -= 8;
> +
> +       return ex_handler_default(fixup, regs);
> +}
> +#endif
> +
>  int ex_get_fixup_type(unsigned long ip)
>  {
>         const struct exception_table_entry *e = search_exception_tables(ip);
> @@ -272,6 +304,10 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
>                 return ex_handler_ucopy_len(e, regs, trapnr, reg, imm);
>         case EX_TYPE_ZEROPAD:
>                 return ex_handler_zeropad(e, regs, fault_addr);
> +#ifdef CONFIG_X86_FRED
> +       case EX_TYPE_ERETU:
> +               return ex_handler_eretu(e, regs, error_code);
> +#endif
>         }
>         BUG();
>  }
> --
> 2.34.1
>