diff mbox series

[v1] KVM: x86: Validate values set to guest's MSR_IA32_ARCH_CAPABILITIES

Message ID 20240422130558.86965-1-wei.w.wang@intel.com (mailing list archive)
State New, archived
Headers show
Series [v1] KVM: x86: Validate values set to guest's MSR_IA32_ARCH_CAPABILITIES | expand

Commit Message

Wang, Wei W April 22, 2024, 1:05 p.m. UTC 
If the bits set by userspace to the guest's MSR_IA32_ARCH_CAPABILITIES
are not supported by KVM, fails the write. This safeguards against the
launch of a guest with a feature set, enumerated via
MSR_IA32_ARCH_CAPABILITIES, that surpasses the capabilities supported by
KVM.

Fixes: 0cf9135b773b ("KVM: x86: Emulate MSR_IA32_ARCH_CAPABILITIES on AMD hosts")
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 49ff3b4aec51e3abfc9369997cc603319b02af9a
prerequisite-patch-id: adcf6a23955e33796219e612d703ae107482d1a5
prerequisite-patch-id: dbb173ac5bdfc012168f13188de6fda47dd109ca
prerequisite-patch-id: b0fab89edfe2456f4e892d008eaac0648c420f5d
prerequisite-patch-id: 8371de5f48c05e346824364fb6155958d21b37df
prerequisite-patch-id: 05382cd95d03b5117dbab4affa4deb1f325af11b
prerequisite-patch-id: 4597cf183484342bf1ae96fccaab209a10fa0a5c
prerequisite-patch-id: a89dfcd6ce3748d297cbe338af9ccf4178bd6538
prerequisite-patch-id: 77189fb281d97a6ec63be83c7c0659dded09c046
prerequisite-patch-id: db39eb599599bdedaf6ce3565817b484f9190d83
prerequisite-patch-id: 840f990b7e127d2610ba2633a77b96b076e5b699
prerequisite-patch-id: b4934fe6c00e8794578e8e1c43784bdeac8fe7bb
prerequisite-patch-id: b2a88fe95fb4d57757798576af88d9b10ecf0b44
prerequisite-patch-id: d2b0f2992dba636908972d75a569f1294cc5dfb1
prerequisite-patch-id: e5a19717c15d8a1ff906dc5ea097b7a8392abf80
prerequisite-patch-id: 7fc7bedbde2814763e9860d65903f1987e61107e
prerequisite-patch-id: 15b1621fda294d8b486f19a514b733dc7de94a70
prerequisite-patch-id: 87b48657d42fd4b80ad3c74d6009c06048ad5c68
prerequisite-patch-id: f5020e37f76403b649908b3a6682db1330f1202c
prerequisite-patch-id: 44b3adbeab1096ab3093cbbb2a72c9fa837d8100
prerequisite-patch-id: 50821a9074c303f3cc8cf4aefb91fe39c7bbd2b4
prerequisite-patch-id: 1168a01580cf2a4dae5ea36e58f0633da5d624e1
prerequisite-patch-id: 912e431eee034bc19cae9bd4ec3cf2aa1b86e66f
prerequisite-patch-id: 8b410b87d9c4cd67e37b59af4800aed8640ae2b4
prerequisite-patch-id: 39d09da8c9dfde6fea0ebc313b41fcf50bad9e8f
prerequisite-patch-id: df2b2c3c5116d994c3d103ea7586e189c0a8b38f
prerequisite-patch-id: d9e8b09ef589e51e925182b66c20d53a6d42d074
prerequisite-patch-id: 50ede137eb3500592b91f7ac6ba741fb680bb8d1
prerequisite-patch-id: 50e770836f91502903de710da1649ca25d06adac

Comments

Sean Christopherson April 22, 2024, 7:43 p.m. UTC | #1 
On Mon, Apr 22, 2024, Wei Wang wrote:
> If the bits set by userspace to the guest's MSR_IA32_ARCH_CAPABILITIES
> are not supported by KVM, fails the write. This safeguards against the
> launch of a guest with a feature set, enumerated via
> MSR_IA32_ARCH_CAPABILITIES, that surpasses the capabilities supported by
> KVM.

I'm not entirely certain KVM cares.  Similar to guest CPUID, advertising features
to the guest that are unbeknownst may actually make sense in some scenarios, e.g.
if userspace learns of yet another "NO" bit that says a CPU isn't vulnerable to
some flaw.

ARCH_CAPABILITIES is read-only, i.e. KVM _can't_ shove it into hardware.  So as
long as KVM treats the value as "untrusted", like KVM does for guest CPUID, I
think the current behavior is actually ok.

> Fixes: 0cf9135b773b ("KVM: x86: Emulate MSR_IA32_ARCH_CAPABILITIES on AMD hosts")

This goes all the way back to:

  28c1c9fabf48 ("KVM/VMX: Emulate MSR_IA32_ARCH_CAPABILITIES")

the above just moved the logic from vmx.c to x86.c.

> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  arch/x86/kvm/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ebcc12d1e1de..21d476e8e4b0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3808,6 +3808,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  	case MSR_IA32_ARCH_CAPABILITIES:
>  		if (!msr_info->host_initiated)
>  			return 1;
> +		if (data & ~kvm_get_arch_capabilities())
> +			return 1;
> +
>  		vcpu->arch.arch_capabilities = data;
>  		break;
>  	case MSR_IA32_PERF_CAPABILITIES:
Wang, Wei W April 23, 2024, 3:20 a.m. UTC | #2 
On Tuesday, April 23, 2024 3:44 AM, Sean Christopherson wrote:
> On Mon, Apr 22, 2024, Wei Wang wrote:
> > If the bits set by userspace to the guest's MSR_IA32_ARCH_CAPABILITIES
> > are not supported by KVM, fails the write. This safeguards against the
> > launch of a guest with a feature set, enumerated via
> > MSR_IA32_ARCH_CAPABILITIES, that surpasses the capabilities supported
> > by KVM.
> 
> I'm not entirely certain KVM cares.  Similar to guest CPUID, advertising features
> to the guest that are unbeknownst may actually make sense in some scenarios,
> e.g.
> if userspace learns of yet another "NO" bit that says a CPU isn't vulnerable to
> some flaw.

I think it might be more appropriate for the guest to see the "NO" bit only when
the host, such as the hardware (i.e., host_arch_capabilities), already supports it.
Otherwise, the guest could be misled by a false "NO" bit. For instance, the guest
might assume it's not vulnerable to a certain flaw as it sees the "NO" bit from the
MSR, even though the enhancement feature isn't actually supported by the host,
and thus bypass a workaround (to the vulnerability) it should have used. This could
arise with a faulty or compromised userspace.
Another scenario pertains to guest live migration: the source platform physically
supports the "NO" bit, but the destination platform does not. If KVM fails the MSR
write here, it could prevent such a live migration from proceeding.

So I think it might be prudent for KVM to perform this check. This is similar to the
MSR_IA32_PERF_CAPABILITIES case that we have implemented.

> 
> ARCH_CAPABILITIES is read-only, i.e. KVM _can't_ shove it into hardware.  So
> as long as KVM treats the value as "untrusted", like KVM does for guest CPUID,
> I think the current behavior is actually ok.

Yes, the value coming from userspace could be considered "untrusted", but should
KVM ensure to expose a trusted/reliable value to the guest?


> 
> > Fixes: 0cf9135b773b ("KVM: x86: Emulate MSR_IA32_ARCH_CAPABILITIES on
> > AMD hosts")
> 
> This goes all the way back to:
> 
>   28c1c9fabf48 ("KVM/VMX: Emulate MSR_IA32_ARCH_CAPABILITIES")
> 
> the above just moved the logic from vmx.c to x86.c.

OK.
Sean Christopherson April 23, 2024, 2:37 p.m. UTC | #3 
On Tue, Apr 23, 2024, Wei W Wang wrote:
> On Tuesday, April 23, 2024 3:44 AM, Sean Christopherson wrote:
> > On Mon, Apr 22, 2024, Wei Wang wrote:
> > > If the bits set by userspace to the guest's MSR_IA32_ARCH_CAPABILITIES
> > > are not supported by KVM, fails the write. This safeguards against the
> > > launch of a guest with a feature set, enumerated via
> > > MSR_IA32_ARCH_CAPABILITIES, that surpasses the capabilities supported
> > > by KVM.
> > 
> > I'm not entirely certain KVM cares.  Similar to guest CPUID, advertising
> > features to the guest that are unbeknownst may actually make sense in some
> > scenarios, e.g.  if userspace learns of yet another "NO" bit that says a
> > CPU isn't vulnerable to some flaw.
> 
> I think it might be more appropriate for the guest to see the "NO" bit only when
> the host, such as the hardware (i.e., host_arch_capabilities), already supports it.
> Otherwise, the guest could be misled by a false "NO" bit. For instance, the guest
> might assume it's not vulnerable to a certain flaw as it sees the "NO" bit from the
> MSR, even though the enhancement feature isn't actually supported by the host,
> and thus bypass a workaround (to the vulnerability) it should have used. This could
> arise with a faulty or compromised userspace.
> Another scenario pertains to guest live migration: the source platform physically
> supports the "NO" bit, but the destination platform does not. If KVM fails the MSR
> write here, it could prevent such a live migration from proceeding.
> 
> So I think it might be prudent for KVM to perform this check. This is similar to the
> MSR_IA32_PERF_CAPABILITIES case that we have implemented.

PERF_CAPABILITIES is a bad example.  KVM ended up enforcing the incoming value
through a series of fixes, not because of a concious design choice.  Though to be
fair, we might still have decided to enforce the supported capabilities since KVM
heavily consumes PERF_CAPABILITIES.

> > ARCH_CAPABILITIES is read-only, i.e. KVM _can't_ shove it into hardware.  So
> > as long as KVM treats the value as "untrusted", like KVM does for guest CPUID,
> > I think the current behavior is actually ok.
> 
> Yes, the value coming from userspace could be considered "untrusted", but should
> KVM ensure to expose a trusted/reliable value to the guest?

No, the VMM is firmly in the guest's TCB.  We have general consensus that KVM
should enforce an architecturally consistent model[1] (there was a deeper PUCK
discussion on this, I think, but I can't find the notes offhand).  But even in
that case the reasoning isn't that userspace isn't trusted, it's that trying to
allow userspace to do MSR writes that architecturally should fail, while disallowing
the same writes from the guest is unnecessarily complex and not maintainable.
And, there is no use case for inconsistent setups that is remotely plausible.

ARCH_CAPABILITIES is different.  Like CPUID, KVM itself isn't negatively affected
by userspace enumerating unsupported bits.  And like CPUID[2], there are plausible
scenarios where enumerating unsupported bits would actually make sense, e.g. if
userspace is enumerating a FMS that is not the actual hardware FMS, and based on
FMS the guest may incorrectly think it needs to a mitigate a vulnerability that
isn't actually relevant.

All that said, I'm not completely opposed to enforcing ARCH_CAPABILITIES, but I
would prefer to do so if and only if there's an actual benefit/need to do so.

[1] https://lore.kernel.org/all/ZfDdS8rtVtyEr0UR@google.com
[2] https://lore.kernel.org/all/ZC4qF90l77m3X1Ir@google.com
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ebcc12d1e1de..21d476e8e4b0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3808,6 +3808,9 @@  int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_ARCH_CAPABILITIES:
 		if (!msr_info->host_initiated)
 			return 1;
+		if (data & ~kvm_get_arch_capabilities())
+			return 1;
+
 		vcpu->arch.arch_capabilities = data;
 		break;
 	case MSR_IA32_PERF_CAPABILITIES: