Message-ID: <bcff605a-3b8d-4dcc-a5cb-63dab1a74ed4@intel.com>
Date:   Mon, 4 Dec 2023 09:07:17 -0800
From:   Dave Hansen <dave.hansen@...el.com>
To:     "Huang, Kai" <kai.huang@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "david@...hat.com" <david@...hat.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "sagis@...gle.com" <sagis@...gle.com>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "rafael@...nel.org" <rafael@...nel.org>,
        "Huang, Ying" <ying.huang@...el.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v15 22/23] x86/mce: Improve error log of kernel space TDX
 #MC due to erratum

On 12/3/23 03:44, Huang, Kai wrote:
...
>> It doesn't need perfect accuracy.  But how do we know it's not going to
>> go, for instance, chase a bad pointer?
>>
>>> +   if (tdx_module_status != TDX_MODULE_INITIALIZED)
>>> +           return false;
>>
>> As an example, what prevents this CPU from observing
>> tdx_module_status==TDX_MODULE_INITIALIZED while the PAMT structure is
>> being assembled?
> 
> There are two types of memory order serializing operations between assembling
> the TDMR/PAMT structure and setting the tdx_module_status to
> TDX_MODULE_INITIALIZED: 1) wbvind_on_all_cpus(); 2) bunch of SEAMCALLs;
> 
> WBINVD is a serializing instruction.  SEAMCALL is a VMEXIT to the TDX module,
> which involves GDT/LDT/control registers/MSRs switch so it is also a serializing
> operation.
> 
> But perhaps we can explicitly add a smp_wmb() between assembling TDMR/PAMT
> structure and setting tdx_module_status if that's better.

... and there's zero documentation of this dependency because ... ?

I suspect it's because it was never looked at until Tony made a comment
about it and we started looking at it.  In other words, it worked by
coincidence.

>>> +   for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++) {
>>> +           unsigned long base, size;
>>> +
>>> +           tdmr_get_pamt(tdmr_entry(tdmr_list, i), &base, &size);
>>> +
>>> +           if (phys >= base && phys < (base + size))
>>> +                   return true;
>>> +   }
>>> +
>>> +   return false;
>>> +}
>>> +
>>> +/*
>>> + * Return whether the memory page at the given physical address is TDX
>>> + * private memory or not.  Called from #MC handler do_machine_check().
>>> + *
>>> + * Note this function may not return an accurate result in rare cases.
>>> + * This is fine as the #MC handler doesn't need a 100% accurate result,
>>> + * because it cannot distinguish #MC between software bug and real
>>> + * hardware error anyway.
>>> + */
>>> +bool tdx_is_private_mem(unsigned long phys)
>>> +{
>>> +   struct tdx_module_args args = {
>>> +           .rcx = phys & PAGE_MASK,
>>> +   };
>>> +   u64 sret;
>>> +
>>> +   if (!platform_tdx_enabled())
>>> +           return false;
>>> +
>>> +   /* Get page type from the TDX module */
>>> +   sret = __seamcall_ret(TDH_PHYMEM_PAGE_RDMD, &args);
>>> +   /*
>>> +    * Handle the case that CPU isn't in VMX operation.
>>> +    *
>>> +    * KVM guarantees no VM is running (thus no TDX guest)
>>> +    * when there's any online CPU isn't in VMX operation.
>>> +    * This means there will be no TDX guest private memory
>>> +    * and Secure-EPT pages.  However the TDX module may have
>>> +    * been initialized and the memory page could be PAMT.
>>> +    */
>>> +   if (sret == TDX_SEAMCALL_UD)
>>> +           return is_pamt_page(phys);
>>
>> Either this is comment is wonky or the module initialization is buggy.
>>
>> config_global_keyid() goes and does SEAMCALLs on all CPUs.  There are
>> zero checks or special handling in there for whether the CPU has done
>> VMXON.  So, by the time we've started initializing the TDX module
>> (including the PAMT), all online CPUs must be able to do SEAMCALLs.  Right?
>>
>> So how can we have a working PAMT here when this CPU can't do SEAMCALLs?
> 
> The corner case is KVM can enable VMX on all cpus, initialize the TDX module,
> and then disable VMX on all cpus.  One example is KVM can be unloaded after it
> initializes the TDX module.
> 
> In this case CPU cannot do SEAMCALL but PAMTs are already working :-)
> 
> However if SEAMCALL cannot be made (due to out of VMX), then the module can only
> be initialized or the initialization hasn't been tried, so both
> tdx_module_status and the tdx_tdmr_list are stable to access.

None of this even matters.  Let's remind ourselves how unbelievably
unlikely this is:

1. You're on an affected system that has the erratum
2. The KVM module gets unloaded, runs vmxoff
3. A kernel bug using a very rare partial write corrupts the PAMT
4. A second bug reads the PAMT consuming poison, #MC is generated
5. Enter #MC handler, SEAMCALL fails
6. #MC handler just reports a plain hardware error

The only thing even remotely wrong with this situation is that the
report won't pin the #MC on TDX.  Play stupid games (removing modules),
win stupid prizes (worse error message).

Can we dynamically mark a module as unsafe to remove?  If so, I'd
happily just say that we should make kvm_intel.ko unsafe to remove when
TDX is supported and move on with life.

tl;dr: I think even looking a #MC on the PAMT after the kvm module is
removed is a fool's errand.

Powered by blists - more mailing lists