On Thu, Oct 20, 2022, Michal Luczaj wrote:
> 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 highly doubt they are 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]
...
> I can't reproduce it under non-KASAN build, I'm not sure what's going on.
KASAN is rightly complaining because, as you already pointed out, the high ports[i]
value will touch memory well beyond the shinfo->evtchn_pending array. Non-KASAN
builds don't have visible failures because the rogue access is only a read, and
the result of the test_bit() only affects whether or not KVM temporarily stalls
the vCPU. In other words, KVM is leaking host state to the guest, but there is
no memory corruption and no functional impact on the guest.
I think this would be the way to fix this particular mess?
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 93c628d3e3a9..5d09a47db732 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -961,7 +961,9 @@ static bool wait_pending_event(struct kvm_vcpu *vcpu, int nr_ports,
struct kvm *kvm = vcpu->kvm;
struct gfn_to_pfn_cache *gpc = &kvm->arch.xen.shinfo_cache;
unsigned long *pending_bits;
+ unsigned long nr_bits;
unsigned long flags;
+ evtchn_port_t port;
bool ret = true;
int idx, i;
@@ -974,13 +976,19 @@ static bool wait_pending_event(struct kvm_vcpu *vcpu, int nr_ports,
if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) {
struct shared_info *shinfo = gpc->khva;
pending_bits = (unsigned long *)&shinfo->evtchn_pending;
+ nr_bits = sizeof(shinfo->evtchn_pending) * BITS_PER_BYTE;
} else {
struct compat_shared_info *shinfo = gpc->khva;
pending_bits = (unsigned long *)&shinfo->evtchn_pending;
+ nr_bits = sizeof(shinfo->evtchn_pending) * BITS_PER_BYTE;
}
for (i = 0; i < nr_ports; i++) {
- if (test_bit(ports[i], pending_bits)) {
+ port = ports[i];
+ if (port >= nr_bits)
+ continue;
+
+ if (test_bit(array_index_nospec(port, nr_bits), pending_bits)) {
ret = true;
break;
}
