linux-kernel - Re: [PATCH v8 2/2] x86/mm: Don't disable PCID if the kernel is running on a hypervisor

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: <ZiATnwBHJThDNpxk@google.com>
Date: Wed, 17 Apr 2024 11:23:27 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>, Xi Ruoyao <xry111@...111.site>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Michael Kelley <mhklinux@...look.com>, 
	Andy Lutomirski <luto@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org, linux-kernel@...r.kernel.org, 
	Andrew Cooper <andrew.cooper3@...rix.com>
Subject: Re: [PATCH v8 2/2] x86/mm: Don't disable PCID if the kernel is
 running on a hypervisor

On Wed, Apr 17, 2024, Dave Hansen wrote:
> On 4/17/24 10:22, Pawan Gupta wrote:
> >>>  static const struct x86_cpu_id invlpg_miss_ids[] = {
> >>> +	/* Only bare-metal is affected.  PCIDs in guests are OK.  */
> >>> +	{
> >>> +	  .vendor	= X86_VENDOR_INTEL,
> >>> +	  .family	= 6,
> >>> +	  .model	= INTEL_FAM6_ANY,

Just in case we go this route (I hope we don't), this should probably be:

        /* Only bare-metal is affected.  PCIDs in guests are OK.  */
        {
          .vendor       = X86_VENDOR_ANY,
          .feature      = X86_FEATURE_HYPERVISOR,
          .driver_data  = 0,
        },

to make it clear that the goal is to match only the feature.  Matching Intel P6
suffices because that's what the other entries in the array all check, but it
makes subtle, confusing code even more subtle and confusing.

> >>> +	  .feature	= X86_FEATURE_HYPERVISOR,
> >> Isn't this inverted?  x86_match_cpu() will return NULL if the CPU doesn't have
> >> HYPERVISOR.  We want it to return NULL if the CPU *does* have HYPERVISOR.
> > I think the implementation is correct, x86_match_cpu() will not return
> > NULL if the CPU doesn't have HYPERVISOR feature *and* matches one of the
> > CPUs below. It will only return NULL if none of the entries match.

Oooh, and because it's the first entry it will always be found even if a different
entry would match the FMS.  Oof.

> I think I gave a crappy suggestion here.
> 
> Let's just do the X86_FEATURE_HYPERVISOR explicitly in the code instead
> of trying to cram it into the invlpg_miss_ids[] check.  It's way easier
> to understand with an explicit code check.

+1.  And it doesn't rely on the HYPERVISOR entry being the first entry, which
is doubly evil.