linux-kernel - Re: [PATCH] KVM: x86: Add GPA limit check to kvm_arch_vcpu_pre_fault

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZpmnZZrh21e9sjLU@ls.amr.corp.intel.com>
Date: Thu, 18 Jul 2024 16:38:13 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org, isaku.yamahata@...il.com,
	Paolo Bonzini <pbonzini@...hat.com>, rick.p.edgecombe@...el.com,
	linux-kernel@...r.kernel.org, isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH] KVM: x86: Add GPA limit check to
 kvm_arch_vcpu_pre_fault_memory()

On Tue, Jul 16, 2024 at 04:49:00PM -0700,
Isaku Yamahata <isaku.yamahata@...el.com> wrote:

> > > - For non-TDX case (DEFAULT_VM, SW_PROTECTED_VM, or SEV):
> > >   When the host supports 5-level TDP, KVM decides to use 4-level TDP if
> > >   cpuid_maxphyaddr() <= 48.  cpuid_maxhyaddr() check prevents
> > >   KVM_PRE_FAULT_MEMORY from passing GFN beyond mappable GFN.
> > 
> > Hardening against cpuid_maxphyaddr() should be out of scope.  We don't enforce
> > it for guest faults, e.g. KVM doesn't kill the guest if allow_smaller_maxphyaddr
> > is false and the GPA is supposed to be illegal.  And trying to enforce it here is
> > a fool's errand since userspace can simply do KVM_SET_CPUID2 to circumvent the
> > restriction.
> 
> Ok, I'll drop maxphys addr check.

Now Rick added a patch to check aliased GFN.  This patch and per-VM mmu_max_gfn
become unnecessarily.  Now I come up with update to pre_fault to test no
memslot case.
https://lore.kernel.org/kvm/20240718211230.1492011-19-rick.p.edgecombe@intel.com/

For non-x86 case, I'm not sure if we can expect what error.


>From d62fc5170b17788041d364e6a17f97f01be4130e Mon Sep 17 00:00:00 2001
Message-ID: <d62fc5170b17788041d364e6a17f97f01be4130e.1721345479.git.isaku.yamahata@...el.com>
From: Isaku Yamahata <isaku.yamahata@...el.com>
Date: Wed, 29 May 2024 12:13:20 -0700
Subject: [PATCH] KVM: selftests: Update pre_fault_memory_test.c to test no
 memslot case

Add test cases to pass GPA to get ENOENT where no memslot is assigned.

Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
---
This tests passes for kvm queue branch, also with KVM TDX branch.
---
 .../selftests/kvm/pre_fault_memory_test.c     | 37 ++++++++++++++-----
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/kvm/pre_fault_memory_test.c b/tools/testing/selftests/kvm/pre_fault_memory_test.c
index 0350a8896a2f..8d057a0bc6fd 100644
--- a/tools/testing/selftests/kvm/pre_fault_memory_test.c
+++ b/tools/testing/selftests/kvm/pre_fault_memory_test.c
@@ -30,8 +30,8 @@ static void guest_code(uint64_t base_gpa)
 	GUEST_DONE();
 }
 
-static void pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size,
-			     u64 left)
+static void __pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size,
+			       u64 left, int *ret, int *save_errno)
 {
 	struct kvm_pre_fault_memory range = {
 		.gpa = gpa,
@@ -39,21 +39,28 @@ static void pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size,
 		.flags = 0,
 	};
 	u64 prev;
-	int ret, save_errno;
 
 	do {
 		prev = range.size;
-		ret = __vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range);
-		save_errno = errno;
-		TEST_ASSERT((range.size < prev) ^ (ret < 0),
+		*ret = __vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range);
+		*save_errno = errno;
+		TEST_ASSERT((range.size < prev) ^ (*ret < 0),
 			    "%sexpecting range.size to change on %s",
-			    ret < 0 ? "not " : "",
-			    ret < 0 ? "failure" : "success");
-	} while (ret >= 0 ? range.size : save_errno == EINTR);
+			    *ret < 0 ? "not " : "",
+			    *ret < 0 ? "failure" : "success");
+	} while (*ret >= 0 ? range.size : *save_errno == EINTR);
 
 	TEST_ASSERT(range.size == left,
 		    "Completed with %lld bytes left, expected %" PRId64,
 		    range.size, left);
+}
+
+static void pre_fault_memory(struct kvm_vcpu *vcpu, u64 gpa, u64 size,
+			     u64 left)
+{
+	int ret, save_errno;
+
+	__pre_fault_memory(vcpu, gpa, size, left, &ret, &save_errno);
 
 	if (left == 0)
 		__TEST_ASSERT_VM_VCPU_IOCTL(!ret, "KVM_PRE_FAULT_MEMORY", ret, vcpu->vm);
@@ -77,6 +84,7 @@ static void __test_pre_fault_memory(unsigned long vm_type, bool private)
 	uint64_t guest_test_phys_mem;
 	uint64_t guest_test_virt_mem;
 	uint64_t alignment, guest_page_size;
+	int ret, save_errno;
 
 	vm = vm_create_shape_with_one_vcpu(shape, &vcpu, guest_code);
 
@@ -101,6 +109,17 @@ static void __test_pre_fault_memory(unsigned long vm_type, bool private)
 	pre_fault_memory(vcpu, guest_test_phys_mem + SZ_2M, PAGE_SIZE * 2, PAGE_SIZE);
 	pre_fault_memory(vcpu, guest_test_phys_mem + TEST_SIZE, PAGE_SIZE, PAGE_SIZE);
 
+#ifdef __x86_64__
+	__pre_fault_memory(vcpu, guest_test_phy_mem - guest_page_size,
+			   guest_page_size, guest_page_size, &ret, &save_errno);
+	__TEST_ASSERT_VM_VCPU_IOCTL(ret && save_errno == ENOENT,
+				    "KVM_PRE_FAULT_MEMORY", ret, vcpu->vm);
+	__pre_fault_memory(vcpu, (vm->max_gfn + 1) << vm->page_shift,
+			   guest_page_size, guest_page_size, &ret, &save_errno);
+	__TEST_ASSERT_VM_VCPU_IOCTL(ret && save_errno == ENOENT,
+				    "KVM_PRE_FAULT_MEMORY", ret, vcpu->vm);
+#endif
+
 	vcpu_args_set(vcpu, 1, guest_test_virt_mem);
 	vcpu_run(vcpu);
 

base-commit: c8b8b8190a80b591aa73c27c70a668799f8db547
-- 
2.45.2



-- 
Isaku Yamahata <isaku.yamahata@...el.com>