[PATCH 0/2] KVM: Fix a lurking bug in kvm_clear_guest()

[PATCH 0/2] KVM: Fix a lurking bug in kvm_clear_guest()

[Sean Christopherson]

Fix a bug in kvm_clear_guest() where it would write beyond the target
page _if_ handed a gpa+len that would span multiple pages.  Luckily, the
bug is unhittable in the current code base as all users ensure the
gpa+len is bound to a single page.

Patch 2 hardens the underlying single page APIs to guard against a bad
offset+len, e.g. so that bugs like the one in kvm_clear_guest() are noisy
and don't escalate to an out-of-bounds access.

Verified and tested by hacking KVM to use kvm_clear_guest() when zeroing
all three pages used for KVM's hidden TSS.

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index f18c2d8c7476..ce64e490e9c7 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3872,14 +3872,17 @@ bool __vmx_guest_state_valid(struct kvm_vcpu *vcpu)
 
 static int init_rmode_tss(struct kvm *kvm, void __user *ua)
 {
-       const void *zero_page = (const void *) __va(page_to_phys(ZERO_PAGE(0)));
+       // const void *zero_page = (const void *) __va(page_to_phys(ZERO_PAGE(0)));
        u16 data;
-       int i;
+       // int i;
 
-       for (i = 0; i < 3; i++) {
-               if (__copy_to_user(ua + PAGE_SIZE * i, zero_page, PAGE_SIZE))
-                       return -EFAULT;
-       }
+       if (kvm_clear_guest(kvm, to_kvm_vmx(kvm)->tss_addr, PAGE_SIZE * 3))
+               return -EFAULT;
+
+       // for (i = 0; i < 3; i++) {
+       //      if (__copy_to_user(ua + PAGE_SIZE * i, zero_page, PAGE_SIZE))
+       //              return -EFAULT;
+       // }
 
        data = TSS_BASE_SIZE + TSS_REDIRECTION_SIZE;
        if (__copy_to_user(ua + TSS_IOPB_BASE_OFFSET, &data, sizeof(u16)))

Sean Christopherson (2):
  KVM: Write the per-page "segment" when clearing (part of) a guest page
  KVM: Harden guest memory APIs against out-of-bounds accesses

 virt/kvm/kvm_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)


base-commit: 15e1c3d65975524c5c792fcd59f7d89f00402261
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH 1/2] KVM: Write the per-page "segment" when clearing (part of) a guest page

[Sean Christopherson]

Pass "seg" instead of "len" when writing guest memory in kvm_clear_guest(),
as "seg" holds the number of bytes to write for the current page, while
"len" holds the total bytes remaining.

Luckily, all users of kvm_clear_guest() are guaranteed to not cross a page
boundary, and so the bug is unhittable in the current code base.

Fixes: 2f5414423ef5 ("KVM: remove kvm_clear_guest_page")
Reported-by: zyr_ms@outlook.com
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219104
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 92901656a0d4..e036c17c4342 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3582,7 +3582,7 @@ int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len)
 	int ret;
 
 	while ((seg = next_segment(len, offset)) != 0) {
-		ret = kvm_write_guest_page(kvm, gfn, zero_page, offset, len);
+		ret = kvm_write_guest_page(kvm, gfn, zero_page, offset, seg);
 		if (ret < 0)
 			return ret;
 		offset = 0;
-- 
2.46.0.469.g59c65b2a67-goog

[PATCH 2/2] KVM: Harden guest memory APIs against out-of-bounds accesses

[Sean Christopherson]

When reading or writing a guest page, WARN and bail if offset+len would
result in a read to a different page so that KVM bugs are more likely to
be detected, and so that any such bugs are less likely to escalate to an
out-of-bounds access.  E.g. if userspace isn't using guard pages and the
target page is at the end of a memslot.

Note, KVM already hardens itself in similar APIs, e.g. in the "cached"
variants, it's just the vanilla APIs that are playing with fire.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e036c17c4342..909d9dd7b448 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3276,6 +3276,9 @@ static int __kvm_read_guest_page(struct kvm_memory_slot *slot, gfn_t gfn,
 	int r;
 	unsigned long addr;
 
+	if (WARN_ON_ONCE(offset + len > PAGE_SIZE))
+		return -EFAULT;
+
 	addr = gfn_to_hva_memslot_prot(slot, gfn, NULL);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
@@ -3349,6 +3352,9 @@ static int __kvm_read_guest_atomic(struct kvm_memory_slot *slot, gfn_t gfn,
 	int r;
 	unsigned long addr;
 
+	if (WARN_ON_ONCE(offset + len > PAGE_SIZE))
+		return -EFAULT;
+
 	addr = gfn_to_hva_memslot_prot(slot, gfn, NULL);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
@@ -3379,6 +3385,9 @@ static int __kvm_write_guest_page(struct kvm *kvm,
 	int r;
 	unsigned long addr;
 
+	if (WARN_ON_ONCE(offset + len > PAGE_SIZE))
+		return -EFAULT;
+
 	addr = gfn_to_hva_memslot(memslot, gfn);
 	if (kvm_is_error_hva(addr))
 		return -EFAULT;
-- 
2.46.0.469.g59c65b2a67-goog

Re: [PATCH 0/2] KVM: Fix a lurking bug in kvm_clear_guest()

[Sean Christopherson]

On Thu, 29 Aug 2024 12:14:11 -0700, Sean Christopherson wrote:
> Fix a bug in kvm_clear_guest() where it would write beyond the target
> page _if_ handed a gpa+len that would span multiple pages.  Luckily, the
> bug is unhittable in the current code base as all users ensure the
> gpa+len is bound to a single page.
> 
> Patch 2 hardens the underlying single page APIs to guard against a bad
> offset+len, e.g. so that bugs like the one in kvm_clear_guest() are noisy
> and don't escalate to an out-of-bounds access.
> 
> [...]

Applied to kvm-x86 generic, thanks!

[1/2] KVM: Write the per-page "segment" when clearing (part of) a guest page
      https://github.com/kvm-x86/linux/commit/ec495f2ab122
[2/2] KVM: Harden guest memory APIs against out-of-bounds accesses
      https://github.com/kvm-x86/linux/commit/025dde582bbf

--
https://github.com/kvm-x86/linux/tree/next