[PATCH] KVM: nSVM: Check full nested state size before writing to user buffer

[PATCH] KVM: nSVM: Check full nested state size before writing to user buffer

[Priyanka Mani]

svm_get_nested_state() compares the userspace-provided buffer size against
kvm_state.size before adding KVM_STATE_NESTED_SVM_VMCB_SIZE for a vCPU that
is in guest mode.  When L2 is active and userspace passes a size that is
large enough for the header but too small for the vmcb12 payload, KVM thus
skips the early exit, copies out the header, clears and fills the full 4KiB
vmcb12 area past the end of the buffer described by userspace, and only
then does the caller notice that the returned size is larger than the
provided one and fail with -E2BIG.

As a result, KVM writes up to 4KiB beyond the buffer userspace said it
has, and if that memory isn't mapped, the ioctl fails with -EFAULT instead
of the documented -E2BIG without reporting the required size.

Compute the full size before the check, as vmx_get_nested_state() does, so
that a too-small buffer is rejected without writing anything to it.

Fixes: cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")
Cc: stable@vger.kernel.org
Signed-off-by: Priyanka Mani <priyankamani2100@gmail.com>
---
Tested on an AMD EPYC 7763 host by booting the patched kernel under QEMU
(nested SVM) with KASAN and lockdep.  A reproducer enters L2, exits to
userspace from L2 and calls KVM_GET_NESTED_STATE with
size = sizeof(struct kvm_nested_state).  Without this patch, KVM writes
4096 bytes past the 128-byte buffer before returning -E2BIG, or returns
-EFAULT if the next page is unmapped.  With this patch, KVM returns
-E2BIG and reports size = 4224 without writing to the buffer.

KVM selftests nested_set_state_test, state_test, smm_test, hyperv_svm_test,
svm_nested_vmcb12_gpa, svm_nested_pat_test and the other nested SVM tests
give identical results with and without the patch.  kvm-unit-tests' SVM
tests could not be run in this nested setup.  Not tested on Intel; the VMX
path is unchanged.

 arch/x86/kvm/svm/nested.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 1cecb82702e2..9a70136172f3 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1916,6 +1916,9 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 
 	svm = to_svm(vcpu);
 
+	if (is_guest_mode(vcpu))
+		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
+
 	if (user_data_size < kvm_state.size)
 		goto out;
 
@@ -1925,7 +1928,6 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 		kvm_state.hdr.svm.gpat = 0;
 		if (l2_has_separate_pat(vcpu))
 			kvm_state.hdr.svm.gpat = svm->vmcb->save.g_pat;
-		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
 		kvm_state.flags |= KVM_STATE_NESTED_GUEST_MODE;
 
 		if (vcpu->arch.nested_run_pending)

base-commit: cbe54fc3462b3949ab27b9ae81fb9ddb509040c7
-- 
2.43.0

Re: [PATCH] KVM: nSVM: Check full nested state size before writing to user buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] KVM: SVM: guest mode and MMU leak in svm_set_nested_state() [1]
--

commit 69ab785aa5333f3d6749b1f4b937158e7dfed5c6
Author: Priyanka Mani <priyankamani2100@gmail.com>

KVM: nSVM: Check full nested state size before writing to user buffer

This commit adds KVM_STATE_NESTED_SVM_VMCB_SIZE to kvm_state.size before
checking user_data_size in svm_get_nested_state() when the vCPU is in
guest mode, preventing writes past the end of a too-small userspace
buffer.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 1cecb82702e2c..9a70136172f38 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1916,6 +1916,9 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
>  
>  	svm = to_svm(vcpu);
>  
> +	if (is_guest_mode(vcpu))
> +		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
> +
>  	if (user_data_size < kvm_state.size)
>  		goto out;
>  
> @@ -1925,7 +1928,6 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
>  		kvm_state.hdr.svm.gpat = 0;
>  		if (l2_has_separate_pat(vcpu))
>  			kvm_state.hdr.svm.gpat = svm->vmcb->save.g_pat;
> -		kvm_state.size += KVM_STATE_NESTED_SVM_VMCB_SIZE;
>  		kvm_state.flags |= KVM_STATE_NESTED_GUEST_MODE;
>  
>  		if (vcpu->arch.nested_run_pending)

[Severity: High]
This problem wasn't introduced by this patch, but in the paired setter
svm_set_nested_state() [1], does a failed nested_svm_load_cr3()
leave the vCPU in guest mode with an active nested MMU context?

arch/x86/kvm/svm/nested.c:svm_set_nested_state() {
    ...
	nested_vmcb02_prepare_control(svm);
    ...
	ret = nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3,
				  nested_npt_enabled(svm), false);
	if (ret)
		goto out_free;
    ...
}

nested_vmcb02_prepare_control() enters guest mode and initializes the
nested MMU context, but the out_free error path only frees save and ctl
without calling leave_guest_mode() or nested_svm_uninit_mmu_context().
If userspace passes an invalid guest CR3 and then continues running the
vCPU after KVM_SET_NESTED_STATE fails, the vCPU runs with a corrupted
nested state and leaked MMU state.

[1] https://sashiko.dev/#/bug/linux-2c0d7c2c-169f-45d3-9953-b6aaf919e66b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/57324d544da1e92cf3b30ff7c40595dfbca1933d.1794465097.git.priyankamani2100@gmail.com?part=1