Re: [PATCH 8/8] KVM: x86: Fix NULL pointer dereference in kvm_xen_set_evtchn_fast()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/13/22 22:28, Sean Christopherson wrote:
> On Thu, Oct 13, 2022, Sean Christopherson wrote:
>> On Mon, Oct 10, 2022, Sean Christopherson wrote:
>>> On Wed, Sep 21, 2022, Michal Luczaj wrote:
>>> If this fixes things on your end (I'll properly test tomorrow too), I'll post a
>>> v2 of the entire series.  There are some cleanups that can be done on top, e.g.
>>> I think we should drop kvm_gpc_unmap() entirely until there's actually a user,
>>> because it's not at all obvious that it's (a) necessary and (b) has desirable
>>> behavior.
>>
>> Sorry for the delay, I initially missed that you included a selftest for the race
>> in the original RFC.  The kernel is no longer exploding, but the test is intermittently
>> soft hanging waiting for the "IRQ".  I'll debug and hopefully post tomorrow.
> 
> Ended up being a test bug (technically).  KVM drops the timer IRQ if the shared
> info page is invalid.  As annoying as that is, there's isn't really a better
> option, and invalidating a shared page while vCPUs are running really is a VMM
> bug.
> 
> To fix, I added an intermediate stage in the test that re-arms the timer if the
> IRQ doesn't arrive in a reasonable amount of time.
> 
> Patches incoming...

Sorry for the late reply, I was away.
Thank you for the whole v2 series. And I'm glad you've found my testcase
useful, even if a bit buggy ;)

Speaking about SCHEDOP_poll, are XEN vmcalls considered trusted?
I've noticed that kvm_xen_schedop_poll() fetches guest-provided
sched_poll.ports without checking if the values are sane. Then, in
wait_pending_event(), there's test_bit(ports[i], pending_bits) which
(for some high ports[i] values) results in KASAN complaining about
"use-after-free":

[   36.463417] ==================================================================
[   36.463564] BUG: KASAN: use-after-free in kvm_xen_hypercall+0xf39/0x1110 [kvm]
[   36.463962] Read of size 8 at addr ffff88815b87b800 by task xen_shinfo_test/956
[   36.464149] CPU: 1 PID: 956 Comm: xen_shinfo_test Not tainted 6.1.0-rc1-kasan+ #49
[   36.464252] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.0-3-3 04/01/2014
[   36.464259] Call Trace:
[   36.464259]  <TASK>
[   36.464259]  dump_stack_lvl+0x5b/0x73
[   36.464259]  print_report+0x17f/0x477
[   36.464259]  ? __virt_addr_valid+0xd5/0x150
[   36.464259]  ? kvm_xen_hypercall+0xf39/0x1110 [kvm]
[   36.464259]  ? kvm_xen_hypercall+0xf39/0x1110 [kvm]
[   36.464259]  kasan_report+0xbb/0xf0
[   36.464259]  ? kvm_xen_hypercall+0xf39/0x1110 [kvm]
[   36.464259]  kasan_check_range+0x136/0x1b0
[   36.464259]  kvm_xen_hypercall+0xf39/0x1110 [kvm]
[   36.464259]  ? kvm_xen_set_evtchn.part.0+0x190/0x190 [kvm]
[   36.464259]  ? get_kvmclock+0x86/0x360 [kvm]
[   36.464259]  ? pvclock_clocksource_read+0x13a/0x190
[   36.464259]  kvm_emulate_hypercall+0x1d7/0x860 [kvm]
[   36.464259]  ? get_kvmclock+0x151/0x360 [kvm]
[   36.464259]  ? kvm_fast_pio+0x260/0x260 [kvm]
[   36.464259]  ? kvm_post_set_cr4+0xf0/0xf0 [kvm]
[   36.464259]  ? lock_release+0x9c/0x430
[   36.464259]  ? rcu_qs+0x2b/0xb0
[   36.464259]  ? rcu_note_context_switch+0x18e/0x9b0
[   36.464259]  ? rcu_read_lock_sched_held+0x10/0x70
[   36.464259]  ? lock_acquire+0xb1/0x3d0
[   36.464259]  ? vmx_vmexit+0x6c/0x19d [kvm_intel]
[   36.464259]  ? vmx_vmexit+0x8d/0x19d [kvm_intel]
[   36.464259]  ? rcu_read_lock_sched_held+0x10/0x70
[   36.464259]  ? lock_acquire+0xb1/0x3d0
[   36.464259]  ? lock_downgrade+0x380/0x380
[   36.464259]  ? vmx_vcpu_run+0x5bf/0x1260 [kvm_intel]
[   36.464259]  vmx_handle_exit+0x295/0xa50 [kvm_intel]
[   36.464259]  vcpu_enter_guest.constprop.0+0x1436/0x1ed0 [kvm]
[   36.464259]  ? kvm_check_and_inject_events+0x800/0x800 [kvm]
[   36.464259]  ? lock_downgrade+0x380/0x380
[   36.464259]  ? __blkcg_punt_bio_submit+0xd0/0xd0
[   36.464259]  ? kvm_arch_vcpu_ioctl_run+0xa46/0xf70 [kvm]
[   36.464259]  ? unlock_page_memcg+0x1e0/0x1e0
[   36.464259]  ? __local_bh_enable_ip+0x8f/0x100
[   36.464259]  ? trace_hardirqs_on+0x2d/0xf0
[   36.464259]  ? fpu_swap_kvm_fpstate+0xbd/0x1c0
[   36.464259]  ? kvm_arch_vcpu_ioctl_run+0x418/0xf70 [kvm]
[   36.464259]  kvm_arch_vcpu_ioctl_run+0x418/0xf70 [kvm]
[   36.464259]  kvm_vcpu_ioctl+0x332/0x8f0 [kvm]
[   36.464259]  ? kvm_clear_dirty_log_protect+0x430/0x430 [kvm]
[   36.464259]  ? do_vfs_ioctl+0x951/0xbf0
[   36.464259]  ? vfs_fileattr_set+0x480/0x480
[   36.464259]  ? kernel_write+0x360/0x360
[   36.464259]  ? selinux_inode_getsecctx+0x50/0x50
[   36.464259]  ? ioctl_has_perm.constprop.0.isra.0+0x133/0x200
[   36.464259]  ? selinux_inode_getsecctx+0x50/0x50
[   36.464259]  ? ksys_write+0xc4/0x140
[   36.464259]  ? __ia32_sys_read+0x40/0x40
[   36.464259]  ? lockdep_hardirqs_on_prepare+0xe/0x220
[   36.464259]  __x64_sys_ioctl+0xb8/0xf0
[   36.464259]  do_syscall_64+0x55/0x80
[   36.464259]  ? lockdep_hardirqs_on_prepare+0xe/0x220
[   36.464259]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
[   36.464259] RIP: 0033:0x7f81e303ec6b
[   36.464259] Code: 73 01 c3 48 8b 0d b5 b1 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 85 b1 1b 00 f7 d8 64 89 01 48
[   36.464259] RSP: 002b:00007fff72009028 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   36.464259] RAX: ffffffffffffffda RBX: 00007f81e33936c0 RCX: 00007f81e303ec6b
[   36.464259] RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000007
[   36.464259] RBP: 00000000008457b0 R08: 0000000000000000 R09: 00000000004029f6
[   36.464259] R10: 00007f81e31b838b R11: 0000000000000246 R12: 00007f81e33a4000
[   36.464259] R13: 00007f81e33a2000 R14: 0000000000000000 R15: 00007f81e33a3020
[   36.464259]  </TASK>
[   36.464259] The buggy address belongs to the physical page:
[   36.464259] page:00000000d9b176a3 refcount:0 mapcount:-128 mapping:0000000000000000 index:0x1 pfn:0x15b87b
[   36.464259] flags: 0x17ffffc0000000(node=0|zone=2|lastcpupid=0x1fffff)
[   36.464259] raw: 0017ffffc0000000 ffffea0005504e48 ffffea00056cf688 0000000000000000
[   36.464259] raw: 0000000000000001 0000000000000000 00000000ffffff7f 0000000000000000
[   36.464259] page dumped because: kasan: bad access detected
[   36.464259] Memory state around the buggy address:
[   36.464259]  ffff88815b87b700: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   36.464259]  ffff88815b87b780: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   36.464259] >ffff88815b87b800: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   36.464259]                    ^
[   36.464259]  ffff88815b87b880: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   36.464259]  ffff88815b87b900: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
[   36.464259] ==================================================================

I can't reproduce it under non-KASAN build, I'm not sure what's going on.

Anyway, here's the testcase, applies after your selftests patches in
https://lore.kernel.org/kvm/20221013211234.1318131-1-seanjc@xxxxxxxxxx/

---
 .../selftests/kvm/x86_64/xen_shinfo_test.c    | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index 2a5727188c8d..402e3d7b86b0 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -375,6 +375,29 @@ static void guest_code(void)
 	guest_saw_irq = false;
 
 	GUEST_SYNC(24);
+	/* Terminate racer thread */
+
+	GUEST_SYNC(25);
+	/* Test SCHEDOP_poll out-of-bounds read */
+
+	p = (struct sched_poll) {
+		.ports = ports,
+		.nr_ports = 1,
+		.timeout = 1
+	};
+
+	ports[0] = (PAGE_SIZE*2) << 3;
+	for (i = 0; i < 0x1000; i++) {
+		asm volatile("vmcall"
+			     : "=a" (rax)
+			     : "a" (__HYPERVISOR_sched_op),
+			       "D" (SCHEDOP_poll),
+			       "S" (&p)
+			     : "memory");
+		ports[0] += PAGE_SIZE << 3;
+	}
+
+	GUEST_SYNC(26);
 }
 
 static int cmp_timespec(struct timespec *a, struct timespec *b)
@@ -925,6 +948,21 @@ int main(int argc, char *argv[])
 
 				ret = pthread_join(thread, 0);
 				TEST_ASSERT(ret == 0, "pthread_join() failed: %s", strerror(ret));
+
+				/* shinfo juggling done; reset to a valid GFN. */
+				struct kvm_xen_hvm_attr ha = {
+					.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
+					.u.shared_info.gfn = SHINFO_REGION_GPA / PAGE_SIZE,
+				};
+				vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &ha);
+				break;
+
+			case 25:
+				if (verbose)
+					printf("Testing SCHEDOP_poll out-of-bounds read\n");
+				break;
+
+			case 26:
 				goto done;
 
 			case 0x20:
-- 
2.38.0




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux