Re: [PATCH 5/5] KVM: VMX: Always honor guest PAT on CPUs that support self-snoop

[Yan Zhao <yan.y.zhao@intel.com>]

On Tue, Mar 12, 2024 at 09:07:11AM -0700, Sean Christopherson wrote:
> On Tue, Mar 12, 2024, Kevin Tian wrote:
> > > From: Sean Christopherson <seanjc@google.com>
> > > Sent: Tuesday, March 12, 2024 8:26 AM
> > > 
> > > On Mon, Mar 11, 2024, Yan Zhao wrote:
> > > > For the case of !static_cpu_has(X86_FEATURE_SELFSNOOP) &&
> > > > kvm_arch_has_noncoherent_dma(vcpu->kvm), I think we at least should warn
> > > > about unsafe before honoring guest memory type.
> > > 
> > > I don't think it gains us enough to offset the potential pain such a
> > > message would bring.  Assuming the warning isn't outright ignored, the most
> > > likely scenario is that the warning will cause random end users to worry
> > > that the setup they've been running for years is broken, when in reality
> > > it's probably just fine for their
> > > use case.
> > 
> > Isn't the 'worry' necessary to allow end users evaluate whether "it's
> > probably just fine for their use case"?
> 
> Realistically, outside of large scale deployments, no end user is going to be able
> to make that evaluation, because practically speaking it requires someone with
> quite low-level hardware knowledge to be able to make that judgment call.  And
> counting by number of human end users (as opposed to number of VMs being run), I
> am willing to bet that the overwhelming majority of KVM users aren't kernel or
> systems engineers.
> 
> Understandably, users tend to be alarmed by (or suspicious of) new warnings that
> show up.  E.g. see the ancient KVM_SET_TSS_ADDR pr_warn[*].  And recently, we had
> an internal bug report filed against KVM because they observed a performance
> regression when booting a KVM guest, and saw a new message about some CPU
> vulnerability being mitigated on VM-Exit that showed up in their *guest* kernel.
> 
> In short, my concern is that adding a new pr_warn() will generate noise for end
> users *and* for KVM developers/maintainers, because even if we phrase the message
> to talk specifically about "untrusted workloads", the majority of affected users
> will not have the necessary knowledge to make an informed decision.
> 
> [*] https://lore.kernel.org/all/f1afa6c0-cde2-ab8b-ea71-bfa62a45b956@tarent.de
> 
> > I saw the old comment already mentioned that doing so may lead to unexpected
> > behaviors. But I'm not sure whether such code-level caveat has been visible
> > enough to end users.
>
What about add a new module parameter to turn on honoring guest for
non-coherent DMAs on CPUs without self-snoop?
A previous example is VFIO's "allow_unsafe_interrupts" parameter.

> Another point to consider: KVM is _always_ potentially broken on such CPUs, as
> KVM forces WB for guest accesses.  I.e. KVM will create memory aliasing if the
> host has guest memory mapped as non-WB in the PAT, without non-coherent DMA
> exposed to the guest.
In this case, memory aliasing may only lead to guest not function well, since
guest is not using WC/UC (which can bypass host initialization data in cache).
But if guest has any chance to read information not intended to it, I believe
we need to fix it as well.


> > > I would be quite surprised if there are people running untrusted workloads
> > > on 10+ year old silicon *and* have passthrough devices and non-coherent
> > > IOMMUs/DMA.
What if the guest is a totally malicious one?
Previously we trust the guest in the case of noncoherent DMA is because
we believe a malicious guest will only meet data corruption and shoot his own
foot.

But as Jason raised the security problem in another mail thread [1],
this will expose security hole if CPUs have no self-snoop. So, we need
to fix it, right?
+ Jason, in case I didn't understand this problem correctly.

[1] https://lore.kernel.org/all/20240108153818.GK50406@nvidia.com/

> > this is probably true.
> > 
> > > And anyone exposing a device directly to an untrusted workload really
> > > should have done their homework.
> > 
> > or they run trusted workloads which might be tampered by virus to
> > exceed the scope of their homework. 😊
> 
> If a workload is being run in a KVM guest for host isolation/security purposes,
> and a device was exposed to said workload, then I would firmly consider analyzing
> the impact of a compromised guest to be part of their homework.
> 
> > > And it's not like we're going to change KVM's historical behavior at this point.
> > 
> > I agree with your point of not breaking userspace. But still think a warning
> > might be informative to let users evaluate their setup against a newly
> > identified "unexpected behavior"  which has security implication beyond
> > the guest, while the previous interpretation of "unexpected behavior" 
> > might be that the guest can at most shoot its own foot...
> 
> If this issue weren't limited to 10+ year old hardware, I would be more inclined
> to add a message.  But at this point, realistically the only thing KVM would be
> saying is "you're running old hardware, that might be unsafe in today's world".
> 
> For users that care about security, we'd be telling them something they already
> know (and if they don't know, they've got bigger problems).  And for everyone
> else, it'd be scary noise without any meaningful benefit.