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] [day] [month] [year] [list]

Message-ID: <SJ1PR11MB6083B1533EA1AB79F091B3A4FC122@SJ1PR11MB6083.namprd11.prod.outlook.com>
Date: Mon, 22 Apr 2024 16:57:23 +0000
From: "Luck, Tony" <tony.luck@...el.com>
To: Yazen Ghannam <yazen.ghannam@....com>, Borislav Petkov <bp@...en8.de>
CC: "x86@...nel.org" <x86@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-edac@...r.kernel.org"
	<linux-edac@...r.kernel.org>, "patches@...ts.linux.dev"
	<patches@...ts.linux.dev>
Subject: RE: [PATCH] x86/mce: Implement recovery for errors in TDX/SEAM
 non-root mode

> > --- 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.

For absolute architectural purity you are right. But the MCG_SEAM_NR bit has never been
used in IA32_MCG_STATUS, so I felt it would just be extra noise in a busy piece of code
to add it.

> > +           /*
> > +            * 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?

That looks like a very nice start to tackle this. Much appreciated as I'm
in the beginning steps to figure out some other SRAR recovery actions.
Having a way to keep track of which action to take will make everything
much cleaner.

It would also solve the " architectural purity" issue above as the check for the
MCG_CAP bit could be imbedded in the severity -> action code. So do_machine_check()
would just have a switch(action) { case MCE_DO_THIS: ... }

-Tony

-Tony