linux-kernel - Re: [PATCH v1] KVM: x86: Validate values set to guest's MSR_IA32_ARCH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zia94vbLD-DF1GEw@google.com>
Date: Mon, 22 Apr 2024 12:43:30 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Wei Wang <wei.w.wang@...el.com>
Cc: pbonzini@...hat.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] KVM: x86: Validate values set to guest's MSR_IA32_ARCH_CAPABILITIES

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@...el.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: