Re: [PATCH 1/4] Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: x86: set_efer() revert breaks test_host_initiated_lmsle() and api.rst
--

commit f8fc3a0796b0b21e716b9ed0c1fcf3c9297d569f
Author: Sean Christopherson <seanjc@google.com>

Revert "KVM: x86: Skip EFER vs. guest CPUID checks for host-initiated writes"

This commit reverts commit 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID
checks for host-initiated writes") so that set_efer() unconditionally validates
EFER against guest CPUID capabilities and CR0.PG even for host-initiated
MSR_EFER writes.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index e91650af44c20..2df42276fd826 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -580,8 +580,11 @@ static int do_get_feature_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
>  				 kvm_get_feature_msr);
>  }
>  
> -static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> +bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
> +	if (efer & ~kvm_caps.supported_efer_bits)
> +		return false;
> +
>  	if (efer & EFER_AUTOIBRS && !guest_cpu_cap_has(vcpu, X86_FEATURE_AUTOIBRS))
>  		return false;
>  
> @@ -610,33 +613,18 @@ static bool __kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
>  
>  	return true;
>  }
> -
> -bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer)
> -{
> -	if (efer & ~kvm_caps.supported_efer_bits)
> -		return false;
> -
> -	return __kvm_valid_efer(vcpu, efer);
> -}
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_valid_efer);
>  
> -static int set_efer(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  {
>  	u64 old_efer = vcpu->arch.efer;
> -	u64 efer = msr_info->data;
>  	int r;
>  
> -	if (efer & ~kvm_caps.supported_efer_bits)
> +	if (!kvm_valid_efer(vcpu, efer))
>  		return 1;
>  
> -	if (!msr_info->host_initiated) {
> -		if (!__kvm_valid_efer(vcpu, efer))
> -			return 1;
> -
> -		if (is_paging(vcpu) &&
> -		    (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
> -			return 1;
> -	}
> +	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
> +		return 1;

[Severity: Medium]
Does reverting this check break test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c and leave
Documentation/virt/kvm/api.rst out of date?

Commit 7358d13e1f19 ("KVM: x86: Honor the guest's EFER_LMSLE_MBZ") and commit
34f749161dab ("KVM: selftests: Add coverage for the EFER_LMSLE_MBZ defeature")
were added after commit 11988499e62b ("KVM: x86: Skip EFER vs. guest CPUID
checks for host-initiated writes"), when set_efer() skipped __kvm_valid_efer()
for host-initiated writes.

With this revert, host-initiated MSR_EFER writes via KVM_SET_MSRS take the path
kvm_arch_vcpu_ioctl() -> msr_io() -> kvm_set_msr() -> kvm_set_msr_common() ->
set_efer() -> kvm_valid_efer(), which rejects EFER_LMSLE when
X86_FEATURE_EFER_LMSLE_MBZ is set in guest CPUID:

arch/x86/kvm/msrs.c:kvm_valid_efer() {
	...
	if (efer & EFER_LMSLE &&
	    guest_cpu_cap_has(vcpu, X86_FEATURE_EFER_LMSLE_MBZ))
		return false;
}

On AMD SVM hosts that support EFER.LMSLE, test_host_initiated_lmsle() in
tools/testing/selftests/kvm/x86/svm_nested_efer_test.c sets
X86_FEATURE_EFER_LMSLE_MBZ and then asserts that a host-initiated write of
EFER_LMSLE succeeds:

tools/testing/selftests/kvm/x86/svm_nested_efer_test.c:test_host_initiated_lmsle() {
	...
	vcpu_set_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ);
	...
	vcpu_set_msr(vcpu, MSR_EFER, efer | EFER_LMSLE);
	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_EFER), efer | EFER_LMSLE);
}

Because set_efer() now returns 1 on that write, vcpu_set_msr() fails and aborts
the selftest (and neither this patch nor follow-up patch 4/4 in the series
updates test_host_initiated_lmsle() or the related comment in
test_sregs_lmsle()).

In addition, Documentation/virt/kvm/api.rst under "Long mode segment limits"
still states:

  Note, ``KVM_SET_MSRS`` is exempt from the check, as host-initiated MSR writes
  skip guest CPUID checks so that userspace can set MSRs before it sets guest
  CPUID.  ``KVM_SET_SREGS`` and nested VMRUN are not exempt.

And efer_trap() in arch/x86/kvm/svm/svm.c still references the removed
__kvm_valid_efer() helper in its comment:

  * Clear the EFER_SVME bit from EFER. The SVM code always sets this
  * bit in svm_set_efer(), but __kvm_valid_efer() checks it against
  * whether the guest has X86_FEATURE_SVM - this avoids a failure if
  * the guest doesn't have X86_FEATURE_SVM.

Should test_host_initiated_lmsle(), Documentation/virt/kvm/api.rst, and the
comment in efer_trap() be updated to match the restored set_efer() behavior?

[ ... ]

>
>  	efer &= ~EFER_LMA;
>  	efer |= vcpu->arch.efer & EFER_LMA;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=1