Received-SPF: pass (google.com: domain of linux-kernel+bounces-153929-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 22 Apr 2024 12:43:30 -0700
In-Reply-To: <20240422130558.86965-1-wei.w.wang@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240422130558.86965-1-wei.w.wang@intel.com>
Message-ID: <Zia94vbLD-DF1GEw@google.com>
Subject: Re: [PATCH v1] KVM: x86: Validate values set to guest's MSR_IA32_ARCH_CAPABILITIES
From: Sean Christopherson <seanjc@google.com>
To: Wei Wang <wei.w.wang@intel.com>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

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: