linux-kernel - RE: [PATCH v2 04/21] x86/virt/tdx: Add skeleton for detecting and initializing TDX on demand

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: <BN9PR11MB52760B743E208684A098B61C8C1D9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Mon, 28 Mar 2022 08:26:31 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     "Huang, Kai" <kai.huang@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Gleixner, Thomas" <thomas.gleixner@...el.com>
CC:     "Hansen, Dave" <dave.hansen@...el.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "Luck, Tony" <tony.luck@...el.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: RE: [PATCH v2 04/21] x86/virt/tdx: Add skeleton for detecting and
 initializing TDX on demand

> From: Huang, Kai <kai.huang@...el.com>
> Sent: Monday, March 28, 2022 9:58 AM
> 
> On Wed, 2022-03-23 at 19:49 +1300, Tian, Kevin wrote:
> > > From: Kai Huang <kai.huang@...el.com>
> > > Sent: Sunday, March 13, 2022 6:50 PM
> > > +static bool seamrr_enabled(void)
> > > +{
> > > +     /*
> > > +      * To detect any BIOS misconfiguration among cores, all logical
> > > +      * cpus must have been brought up at least once.  This is true
> > > +      * unless 'maxcpus' kernel command line is used to limit the
> > > +      * number of cpus to be brought up during boot time.  However
> > > +      * 'maxcpus' is basically an invalid operation mode due to the
> > > +      * MCE broadcast problem, and it should not be used on a TDX
> > > +      * capable machine.  Just do paranoid check here and WARN()
> > > +      * if not the case.
> > > +      */
> > > +     if (WARN_ON_ONCE(!cpumask_equal(&cpus_booted_once_mask,
> > > +                                     cpu_present_mask)))
> > > +             return false;
> > > +
> >
> > cpu_present_mask doesn't always represent BIOS-enabled CPUs as it
> > can be further restricted by 'nr_cpus' and 'possible_cpus'. From this
> > angle above check doesn't capture all misconfigured boot options
> > which is incompatible with TDX. Then is such partial check still useful
> > or better to just document those restrictions and let TDX module
> > capture any violation later as what you explained in __init_tdx()?
> >
> > Thanks
> > Kevin
> 
> The purpose of checking cpus_booted_once_mask aganist
> cpu_present_mask is NOT to
> check whether all BIOS-enabled CPUs are brought up at least once.  Instead,
> the
> purpose is to check whether all cpus that kernel can use are brought up at
> least
> once (TDX-capable machine doesn't support ACPI CPU hotplug and all cpus
> are
> marked as enabled in MADT table, therefore cpu_present_mask is used
> instead of
> cpu_possible_mask).  This is used to make sure SEAMRR has been detected
> on all
> cpus that kernel can use.
> 
> Checking whether "all BIOS-enabled cpus are up" is not done here (neither in
> this series as we discussed it seems there's no appropriate variable to
> represent it).  And we just let TDH.SYS.CONFIG to fail if TDH.SYS.LP.INIT is not
> done on all BIOS-enabled CPUs.
> 

TDX requires all BIOS-enabled CPUs to execute certain SEAMCALLs for TDX
initialization. 

cpu_present_mask does not always represent BIOS-enabled CPUs due
to those boot options. Then why do we care whether CPUs in this mask
(if only representing a subset of BIOS-enabled CPUs) are at least brought
up once? It will fail at TDH.SYS.CONFIG anyway.

btw your comment said that 'maxcpus' is basically an invalid mode
due to MCE broadcase problem. I didn't find any code to block it when
MCE is enabled, thus wonder the rationale behind and whether that
rationale can be brought to this series (i.e. no check against those
conflicting boot options and just let SEAMCALL itself to detect and fail).

@Thomas, any guidance here?

Thanks
Kevin