[PATCH] KVM: nSVM: Remove a user-triggerable WARN on nested_svm_load_cr3() succeeding

Sean Christopherson posted 1 patch 1 day, 21 hours ago
arch/x86/kvm/svm/nested.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
[PATCH] KVM: nSVM: Remove a user-triggerable WARN on nested_svm_load_cr3() succeeding
Posted by Sean Christopherson 1 day, 21 hours ago
Drop the WARN in svm_set_nested_state() on nested_svm_load_cr3() failing
as it is trivially easy to trigger from userspace by modifying CPUID after
loading CR3.  E.g. modifying the state restoration selftest like so:

  --- tools/testing/selftests/kvm/x86/state_test.c
  +++ tools/testing/selftests/kvm/x86/state_test.c
  @@ -280,7 +280,16 @@ int main(int argc, char *argv[])

                 /* Restore state in a new VM.  */
                  vcpu = vm_recreate_with_one_vcpu(vm);
  -               vcpu_load_state(vcpu, state);
  +
  +               if (stage == 4) {
  +                       state->sregs.cr3 = BIT(44);
  +                       vcpu_load_state(vcpu, state);
  +
  +                       vcpu_set_cpuid_property(vcpu, X86_PROPERTY_MAX_PHY_ADDR, 36);
  +                       __vcpu_nested_state_set(vcpu, &state->nested);
  +               } else {
  +                       vcpu_load_state(vcpu, state);
  +               }

                  /*
                   * Restore XSAVE state in a dummy vCPU, first without doing

generates:

  WARNING: CPU: 30 PID: 938 at arch/x86/kvm/svm/nested.c:1877 svm_set_nested_state+0x34a/0x360 [kvm_amd]
  Modules linked in: kvm_amd kvm irqbypass [last unloaded: kvm]
  CPU: 30 UID: 1000 PID: 938 Comm: state_test Tainted: G        W           6.18.0-rc7-58e10b63777d-next-vm
  Tainted: [W]=WARN
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0010:svm_set_nested_state+0x34a/0x360 [kvm_amd]
  Call Trace:
   <TASK>
   kvm_arch_vcpu_ioctl+0xf33/0x1700 [kvm]
   kvm_vcpu_ioctl+0x4e6/0x8f0 [kvm]
   __x64_sys_ioctl+0x8f/0xd0
   do_syscall_64+0x61/0xad0
   entry_SYSCALL_64_after_hwframe+0x4b/0x53

Simply delete the WARN instead of trying to prevent userspace from shoving
"illegal" state into CR3.  For better or worse, KVM's ABI allows userspace
to set CPUID after SREGS, and vice versa, and KVM is very permissive when
it comes to guest CPUID.  I.e. attempting to enforce the virtual CPU model
when setting CPUID could break userspace.  Given that the WARN doesn't
provide any meaningful protection for KVM or benefit for userspace, simply
drop it even though the odds of breaking userspace are minuscule.

Opportunistically delete a spurious newline.

Fixes: b222b0b88162 ("KVM: nSVM: refactor the CR3 reload on migration")
Cc: stable@vger.kernel.org
Cc: Yosry Ahmed <yosry.ahmed@linux.dev>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/nested.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index ba0f11c68372..9be67040e94d 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1870,10 +1870,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	 * thus MMU might not be initialized correctly.
 	 * Set it again to fix this.
 	 */
-
 	ret = nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3,
 				  nested_npt_enabled(svm), false);
-	if (WARN_ON_ONCE(ret))
+	if (ret)
 		goto out_free;
 
 	svm->nested.force_msr_bitmap_recalc = true;

base-commit: 2111f7ca0e92dec60f0a3644ff3b164342af33c1
-- 
2.52.0.239.gd5f0c6e74e-goog
Re: [PATCH] KVM: nSVM: Remove a user-triggerable WARN on nested_svm_load_cr3() succeeding
Posted by Yosry Ahmed 1 day, 20 hours ago
On Tue, Dec 16, 2025 at 08:17:54AM -0800, Sean Christopherson wrote:
> Drop the WARN in svm_set_nested_state() on nested_svm_load_cr3() failing
> as it is trivially easy to trigger from userspace by modifying CPUID after
> loading CR3.  E.g. modifying the state restoration selftest like so:
> 
>   --- tools/testing/selftests/kvm/x86/state_test.c
>   +++ tools/testing/selftests/kvm/x86/state_test.c
>   @@ -280,7 +280,16 @@ int main(int argc, char *argv[])
> 
>                  /* Restore state in a new VM.  */
>                   vcpu = vm_recreate_with_one_vcpu(vm);
>   -               vcpu_load_state(vcpu, state);
>   +
>   +               if (stage == 4) {
>   +                       state->sregs.cr3 = BIT(44);
>   +                       vcpu_load_state(vcpu, state);
>   +
>   +                       vcpu_set_cpuid_property(vcpu, X86_PROPERTY_MAX_PHY_ADDR, 36);
>   +                       __vcpu_nested_state_set(vcpu, &state->nested);
>   +               } else {
>   +                       vcpu_load_state(vcpu, state);
>   +               }
> 
>                   /*
>                    * Restore XSAVE state in a dummy vCPU, first without doing
> 
> generates:
> 
>   WARNING: CPU: 30 PID: 938 at arch/x86/kvm/svm/nested.c:1877 svm_set_nested_state+0x34a/0x360 [kvm_amd]
>   Modules linked in: kvm_amd kvm irqbypass [last unloaded: kvm]
>   CPU: 30 UID: 1000 PID: 938 Comm: state_test Tainted: G        W           6.18.0-rc7-58e10b63777d-next-vm
>   Tainted: [W]=WARN
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>   RIP: 0010:svm_set_nested_state+0x34a/0x360 [kvm_amd]
>   Call Trace:
>    <TASK>
>    kvm_arch_vcpu_ioctl+0xf33/0x1700 [kvm]
>    kvm_vcpu_ioctl+0x4e6/0x8f0 [kvm]
>    __x64_sys_ioctl+0x8f/0xd0
>    do_syscall_64+0x61/0xad0
>    entry_SYSCALL_64_after_hwframe+0x4b/0x53
> 
> Simply delete the WARN instead of trying to prevent userspace from shoving
> "illegal" state into CR3.  For better or worse, KVM's ABI allows userspace
> to set CPUID after SREGS, and vice versa, and KVM is very permissive when
> it comes to guest CPUID.  I.e. attempting to enforce the virtual CPU model
> when setting CPUID could break userspace.  Given that the WARN doesn't
> provide any meaningful protection for KVM or benefit for userspace, simply
> drop it even though the odds of breaking userspace are minuscule.
> 
> Opportunistically delete a spurious newline.
> 
> Fixes: b222b0b88162 ("KVM: nSVM: refactor the CR3 reload on migration")
> Cc: stable@vger.kernel.org
> Cc: Yosry Ahmed <yosry.ahmed@linux.dev>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Yosry Ahmed <yosry.ahmed@linux.dev>

> ---
>  arch/x86/kvm/svm/nested.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ba0f11c68372..9be67040e94d 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1870,10 +1870,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 * thus MMU might not be initialized correctly.
>  	 * Set it again to fix this.
>  	 */
> -
>  	ret = nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3,
>  				  nested_npt_enabled(svm), false);
> -	if (WARN_ON_ONCE(ret))
> +	if (ret)
>  		goto out_free;
>  
>  	svm->nested.force_msr_bitmap_recalc = true;
> 
> base-commit: 2111f7ca0e92dec60f0a3644ff3b164342af33c1
> -- 
> 2.52.0.239.gd5f0c6e74e-goog
>