linux-kernel - Re: [PATCH] KVM: nSVM: Remove a user-triggerable WARN on nested_svm_load

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 for Android: free password hash cracker in your pocket

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

Message-ID: <qgxsouujnruyg3toab5r4thuktx4j45ifl2ivwlq24qccackeb@rw7ve5xzhi47>
Date: Tue, 16 Dec 2025 17:01:34 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: nSVM: Remove a user-triggerable WARN on
 nested_svm_load_cr3() succeeding

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@...r.kernel.org
> Cc: Yosry Ahmed <yosry.ahmed@...ux.dev>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>

Reviewed-by: Yosry Ahmed <yosry.ahmed@...ux.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
>