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.
