diff mbox series

kvm: Disallow wraparound in kvm_gfn_to_hva_cache_init

Message ID 20181217215333.161892-1-jmattson@google.com (mailing list archive)
State New, archived
Headers show
Series kvm: Disallow wraparound in kvm_gfn_to_hva_cache_init | expand

Commit Message

Jim Mattson Dec. 17, 2018, 9:53 p.m. UTC 
Previously, in the case where (gpa + len) wrapped around, the entire
region was not validated, as the comment claimed. It doesn't actually
seem that wraparound should be allowed here at all.

Furthermore, since some callers don't check the return code from this
function, it seems prudent to clear ghc->memslot in the event of an
error.

Fixes: 8f964525a121f ("KVM: Allow cross page reads and writes from cached translations.")
Reported-by: Cfir Cohen <cfir@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Cfir Cohen <cfir@google.com>
Reviewed-by: Marc Orr <marcorr@google.com>
Cc: Andrew Honig <ahonig@google.com>
---
 virt/kvm/kvm_main.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

Comments

Radim Krčmář Dec. 20, 2018, 8:13 p.m. UTC | #1 
2018-12-17 13:53-0800, Jim Mattson:
> Previously, in the case where (gpa + len) wrapped around, the entire
> region was not validated, as the comment claimed. It doesn't actually
> seem that wraparound should be allowed here at all.
> 
> Furthermore, since some callers don't check the return code from this
> function, it seems prudent to clear ghc->memslot in the event of an
> error.
> 
> Fixes: 8f964525a121f ("KVM: Allow cross page reads and writes from cached translations.")
> Reported-by: Cfir Cohen <cfir@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Cfir Cohen <cfir@google.com>
> Reviewed-by: Marc Orr <marcorr@google.com>
> Cc: Andrew Honig <ahonig@google.com>
> ---

Queued, thanks.
diff mbox series

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 0041947b7390d..3be46841db06b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2005,32 +2005,33 @@  static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
 	gfn_t end_gfn = (gpa + len - 1) >> PAGE_SHIFT;
 	gfn_t nr_pages_needed = end_gfn - start_gfn + 1;
 	gfn_t nr_pages_avail;
+	int r = start_gfn <= end_gfn ? 0 : -EINVAL;
 
 	ghc->gpa = gpa;
 	ghc->generation = slots->generation;
 	ghc->len = len;
-	ghc->memslot = __gfn_to_memslot(slots, start_gfn);
-	ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn, NULL);
-	if (!kvm_is_error_hva(ghc->hva) && nr_pages_needed <= 1) {
+	ghc->hva = KVM_HVA_ERR_BAD;
+
+	/*
+	 * If the requested region crosses two memslots, we still
+	 * verify that the entire region is valid here.
+	 */
+	while (!r && start_gfn <= end_gfn) {
+		ghc->memslot = __gfn_to_memslot(slots, start_gfn);
+		ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn,
+					   &nr_pages_avail);
+		if (kvm_is_error_hva(ghc->hva))
+			r = -EFAULT;
+		start_gfn += nr_pages_avail;
+	}
+
+	/* Use the slow path for cross page reads and writes. */
+	if (!r && nr_pages_needed == 1)
 		ghc->hva += offset;
-	} else {
-		/*
-		 * If the requested region crosses two memslots, we still
-		 * verify that the entire region is valid here.
-		 */
-		while (start_gfn <= end_gfn) {
-			nr_pages_avail = 0;
-			ghc->memslot = __gfn_to_memslot(slots, start_gfn);
-			ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn,
-						   &nr_pages_avail);
-			if (kvm_is_error_hva(ghc->hva))
-				return -EFAULT;
-			start_gfn += nr_pages_avail;
-		}
-		/* Use the slow path for cross page reads and writes. */
+	else
 		ghc->memslot = NULL;
-	}
-	return 0;
+
+	return r;
 }
 
 int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc,