Re: [RFC v2-fix-v2 2/2] x86/tdx: Ignore WBINVD instruction for TDX guest

[Andi Kleen]

On 5/24/2021 6:45 PM, Dan Williams wrote:
> > > but
> > > it does not make the description of this patch correct.
> > If KVM was broken I'm sure we would hear about it.
> KVM does not try to support the cases where wbinvd being unavailable
> would break the system. That is not the claim being made in this
> patch.

I thought we made that claim.


"We just want to be the same as KVM"

> > The ACPI cases are for S3, which is not supported in guests, or for the
> > old style manual IO port C6, which isn't supported either.
> > The persistent memory cases would require working DMA mappings,
> No, that analysis is wrong.The wbinvd audit would have found that
> persistent memory secure-erase and unlock, which has nothing to do
> with DMA, needs wbinvd to ensure that the CPU has not retained a copy
> of the PMEM contents from before the unlock happened and it needs to
> make sure that any data that was meant to be destroyed by an erasure
> is not retained in cache.

But that's all not supported in TDX.

And the only way it could work in KVM is when there is some DMA, likely 
at least an IOMMU, e.g. to set up the persistent memory. That's what I 
meant with working DMA mappings.

Otherwise KVM would be really broken, but I don't really believe that 
without some real evidence.


> It's fine to not support the above cases, I am asking for the
> explanation to demonstrate the known risks and the known mitigations.

The analysis is that all this stuff that you are worried about cannot be 
enabled in a TDX guest

(it would be a nightmare if it could, we would need to actually make it 
secure against a malicious host)

> IgnorePAT is not the mitigation, the mitigation is an audit to
> describe why the known users are unlikely to be triggered. Even better
> would be an addition patch that does something like:
>
> iff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
> index 4b80150e4afa..a6b13a1ae319 100644
> --- a/drivers/nvdimm/security.c
> +++ b/drivers/nvdimm/security.c
> @@ -170,6 +170,9 @@ static int __nvdimm_security_unlock(struct nvdimm *nvdimm)
>          const void *data;
>          int rc;
>
> +       if (is_protected_guest())
> +               return -ENXIO;
> +
>          /* The bus lock should be held at the top level of the call stack */
>          lockdep_assert_held(&nvdimm_bus->reconfig_mutex);
>
> ...to explicitly error out a wbinvd use case before data is altered
> and wbinvd is needed.

I don't see any point of all of this. We really just want to be the same 
as KVM. Not get into the business of patching a bazillion sub systems 
that cannot be used in TDX anyways.

-Andi