On 10/20/22 18:58, Sean Christopherson wrote:
> On Thu, Oct 20, 2022, Michal Luczaj wrote:
>> Speaking about SCHEDOP_poll, are XEN vmcalls considered trusted?
>
> I highly doubt they are trusted.
Does it mean a CPL3 guest can vmcall SCHEDOP_poll? If so, is the use of
kvm_mmu_gva_to_gpa_system() justified?
>> 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.
OK, so such vCPU stall-or-not is a side channel leaking host memory bit by
bit, right? I'm trying to understand what is actually being leaked here.
Is it user space memory of process that is using KVM on the host?
> 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;
> }
Great, that looks good and passes the test.
Thanks,
Michal
