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

[Dan Williams]

On Mon, May 24, 2021 at 6:02 PM Andi Kleen <ak@xxxxxxxxxxxxxxx> wrote:
>
>
> > That makes KVM also broken for the cases where wbinvd is needed,
>
>
> Or maybe your analysis is wrong?

I'm well aware of the fact that wbinvd is problematic for hypervisors
and is an attack vector for a guest to DOS the host.

>
>
> > 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.

> 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.

> which we
> currently don't support. If DMA mappings were added we would need to
> para virtualized WBINVD, like the comments say.
>
> AFAIK all the rest is for some caching attribute change, which is not
> possible in KVM (because it uses EPT.IgnorePAT=1) nor in TDX (which does
> the same). Some are for MTRR which is completely disabled if you're
> running under EPT.

It's fine to not support the above cases, I am asking for the
explanation to demonstrate the known risks and the known mitigations.
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.