linux-kernel - Re: [RFC PATCH 1/5] x86/kexec: Do unconditional WBINVD for bare-metal in stop_this

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]

Message-ID: <37b8fef5a0a77bbd2452be486d4cc4f463cebacb.camel@intel.com>
Date: Fri, 14 Mar 2025 10:03:13 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "tglx@...utronix.de" <tglx@...utronix.de>, "peterz@...radead.org"
	<peterz@...radead.org>, "mingo@...hat.com" <mingo@...hat.com>, "Hansen, Dave"
	<dave.hansen@...el.com>, "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
	"bp@...en8.de" <bp@...en8.de>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>
CC: "dwmw@...zon.co.uk" <dwmw@...zon.co.uk>, "dyoung@...hat.com"
	<dyoung@...hat.com>, "seanjc@...gle.com" <seanjc@...gle.com>,
	"x86@...nel.org" <x86@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "sagis@...gle.com" <sagis@...gle.com>,
	"hpa@...or.com" <hpa@...or.com>, "Chatre, Reinette"
	<reinette.chatre@...el.com>, "bhe@...hat.com" <bhe@...hat.com>, "Williams,
 Dan J" <dan.j.williams@...el.com>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "thomas.lendacky@....com" <thomas.lendacky@....com>,
	"nik.borisov@...e.com" <nik.borisov@...e.com>, "ashish.kalra@....com"
	<ashish.kalra@....com>, "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [RFC PATCH 1/5] x86/kexec: Do unconditional WBINVD for bare-metal
 in stop_this_cpu()

On Thu, 2025-03-13 at 18:40 +0000, Edgecombe, Rick P wrote:
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index 9c75d701011f..8475d9d2d8c4 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -819,18 +819,19 @@ void __noreturn stop_this_cpu(void *dummy)
> >   	mcheck_cpu_clear(c);
> >   
> >   	/*
> > -	 * Use wbinvd on processors that support SME. This provides support
> > -	 * for performing a successful kexec when going from SME inactive
> > -	 * to SME active (or vice-versa). The cache must be cleared so that
> > -	 * if there are entries with the same physical address, both with and
> > -	 * without the encryption bit, they don't race each other when flushed
> > -	 * and potentially end up with the wrong entry being committed to
> > -	 * memory.
> > +	 * Use wbinvd to support kexec for both SME (from inactive to active
> > +	 * or vice-versa) and TDX.  The cache must be cleared so that if there
> > +	 * are entries with the same physical address, both with and without
> > +	 * the encryption bit(s), they don't race each other when flushed and
> > +	 * potentially end up with the wrong entry being committed to memory.
> >   	 *
> > -	 * Test the CPUID bit directly because the machine might've cleared
> > -	 * X86_FEATURE_SME due to cmdline options.
> > +	 * stop_this_cpu() isn't a fast path, just do unconditional WBINVD for
> > +	 * bare-metal to cover both SME and TDX.  Do not do WBINVD in a guest
> > +	 * since performing one will result in an exception (#VE or #VC) for
> > +	 * TDX or SEV-ES/SEV-SNP guests which the guest may not be able to
> > +	 * handle (e.g., TDX guest panics if it sees #VE).
> >   	 */
> > -	if (c->extended_cpuid_level >= 0x8000001f && (cpuid_eax(0x8000001f) & BIT(0)))
> > +	if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))
> >   		wbinvd();
> 
> I see that this already has Tom's RB, but I'm not sure how this works for AMD.
> The original SME patch tried to avoid writing to memory by putting the wbinvd
> immediately before the halt, but today it is further away. Below this hunk there
> are more instructions that could dirty memory before the halt.  Ohh... it's new.
> 9 months ago 26ba7353caaa ("x86/smp: Add smp_ops.stop_this_cpu() callback") adds
> a function call that would touch the stack. I think it's wrong? And probably
> introduced after this patch was originally written.

(I'll reply others separately since today I am a little bit sick.)

That callback is for TDX guset (which doesn't invoke WBINVD during kexec).  It
doesn't impact SME host.

I kinda agree maybe the code here can be improved, e.g., the code to call
smp_ops.stop_this_cpu() perhaps should be called before the WBINVD.

> 
> Then the cpuid_eax() could be non-inlined, but probably not. But the
> boot_cpu_has() added in this patch could call out to kasan and dirty the stack.

Could you elaborate this since I don't see how kasan is involved?

> 
> So I think the existing SME case might be theoretically incorrect, and if so
> this makes things very slightly worse.

As explained above the function call is empty for SME host.