linux-kernel - Re: [PATCH] x86/mce: Implement recovery for errors in TDX/SEAM non-root mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 Apr 2024 14:16:06 -0400
From: Yazen Ghannam <yazen.ghannam@....com>
To: Tony Luck <tony.luck@...el.com>, Borislav Petkov <bp@...en8.de>
Cc: yazen.ghannam@....com, x86@...nel.org, linux-kernel@...r.kernel.org,
 linux-edac@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH] x86/mce: Implement recovery for errors in TDX/SEAM
 non-root mode

On 4/8/2024 2:09 PM, Tony Luck wrote:
> Machine check SMIs (MSMI) signaled during SEAM operation (typically
> inside TDX guests), on a system with Intel eMCA enabled, might eventually
> be reported to the kernel #MC handler with the saved RIP on the stack
> pointing to the instruction in kernel code after the SEAMCALL instruction
> that entered the SEAM operation. Linux currently says that is a fatal
> error and shuts down.
> 
> There is a new bit in IA32_MCG_STATUS that, when set to 1, indicates
> that the machine check didn't originally occur at that saved RIP, but
> during SEAM non-root operation.
> 
> Add new entries to the severity table to detect this for both data load
> and instruction fetch that set the severity to "AR" (action required).
> 
> Increase the width of the mcgmask/mcgres fields in "struct severity"
> from unsigned char to unsigned short since the new bit is in position 12.
> 
> Action required for these errors is just mark the page as poisoned and
> return from the machine check handler.
> 
> Backport note. Little value in backporting this patch to stable or LTS
> kernels as this is only relevant with support for TDX, which I assume
> won't be backported. But for anyone taking this to v6.1 or older, you
> also need commit a51cbd0d86d3 ("x86/mce: Use severity table to handle
> uncorrected errors in kernel")
> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> 
> ---
> The SEAM_NR bit in IA32_MCG_STATUS hasn't yet made it into the Intel
> Software Developers' Manual. But it is described in section 16.5.2
> of "Intel(R) Trust Domain Extensions (Intel(R) TDX) Module Base
> Architecture Specification" downloadable from:
> https://cdrdv2.intel.com/v1/dl/getContent/733575
> ---
>  arch/x86/include/asm/mce.h         |  2 ++
>  arch/x86/kernel/cpu/mce/core.c     | 18 ++++++++++++++++++
>  arch/x86/kernel/cpu/mce/severity.c | 16 ++++++++++++++--
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index de3118305838..dfd2e9699bd7 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -13,6 +13,7 @@
>  #define MCG_CTL_P		BIT_ULL(8)   /* MCG_CTL register available */
>  #define MCG_EXT_P		BIT_ULL(9)   /* Extended registers available */
>  #define MCG_CMCI_P		BIT_ULL(10)  /* CMCI supported */
> +#define MCG_SEAM_NR		BIT_ULL(12)  /* MCG_STATUS_SEAM_NR supported */
>  #define MCG_EXT_CNT_MASK	0xff0000     /* Number of Extended registers */
>  #define MCG_EXT_CNT_SHIFT	16
>  #define MCG_EXT_CNT(c)		(((c) & MCG_EXT_CNT_MASK) >> MCG_EXT_CNT_SHIFT)
> @@ -25,6 +26,7 @@
>  #define MCG_STATUS_EIPV		BIT_ULL(1)   /* ip points to correct instruction */
>  #define MCG_STATUS_MCIP		BIT_ULL(2)   /* machine check in progress */
>  #define MCG_STATUS_LMCES	BIT_ULL(3)   /* LMCE signaled */
> +#define MCG_STATUS_SEAM_NR	BIT_ULL(12)  /* Machine check inside SEAM non-root mode */
>  
>  /* MCG_EXT_CTL register defines */
>  #define MCG_EXT_CTL_LMCE_EN	BIT_ULL(0) /* Enable LMCE */
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 84d41be6d06b..771a9f183260 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1593,6 +1593,24 @@ noinstr void do_machine_check(struct pt_regs *regs)
>  		else
>  			queue_task_work(&m, msg, kill_me_maybe);
>  
> +	} else if (m.mcgstatus & MCG_STATUS_SEAM_NR) {

MCG_CAP[12] (MCG_SEAM_NR) should be checked first, correct? This could be a
new mce_vendor_flags field set during MCA init.

> +		/*
> +		 * Saved RIP on stack makes it look like the machine check
> +		 * was taken in the kernel on the instruction following
> +		 * the entry to SEAM mode. But MCG_STATUS_SEAM_NR indicates
> +		 * that the machine check was taken inside SEAM non-root
> +		 * mode.  CPU core has already marked that guest as dead.
> +		 * It is OK for the kernel to resume execution at the
> +		 * apparent point of the machine check as the fault did
> +		 * not occur there. Mark the page as poisoned so it won't
> +		 * be added to free list when the guest is terminated.
> +		 */
> +		if (mce_usable_address(&m)) {
> +			struct page *p = pfn_to_online_page(m.addr >> PAGE_SHIFT);
> +
> +			if (p)
> +				SetPageHWPoison(p);
> +		}

I think this is okay, and it could even be more generalized as a "page
offline" action.

Here's some WIP for a generic MCE "action table":
https://github.com/AMDESE/linux/commit/cf0b8a97240ab

This is based on the short discussion here:
https://lore.kernel.org/linux-edac/ZD7gPkfWQeEeEfBe@agluck-desk3.sc.intel.com/

Basically, all the status bits would be checked in mce_severity() and the
appropriate action is set to be done later.

This would be future work, of course. What do you think?

Thanks,
Yazen