Re: [PATCH QEMU HCI-8.1 v6 22/34] kvm: tolerate ioctls after kvm_close()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] kvm: ioctl wrappers return 0 after kvm_close(), faking fd 0 for fd-returning ioctls
- [Medium] kvm: closed-state ioctl guard returns 0 for fd-returning ioctls, handing out fd 0
- [Low] kvm: stale fds left in kvm_parked_vcpus after kvm_close() now stay reachable
--

commit d8d6e98a861f3d1c2b3dafd0181c053060a2ea99
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

kvm: tolerate ioctls after kvm_close()

This patch keeps kvm_state allocated after kvm_close() instead of setting it
to NULL. It makes kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return 0
once the descriptors are closed. The goal is to stop a retired cpr-transfer
source from crashing on late interrupts.

> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c

[ ... ]

> @@ -644,7 +644,6 @@ void kvm_close(void)
>          close(kvm_state->fd);
>          kvm_state->fd = -1;
>      }
> -    kvm_state = NULL;
>  }

[Severity: Low]
kvm_close() runs cpu_remove_sync() for each CPU, which reaches
do_kvm_destroy_vcpu() and then kvm_park_vcpu(). That stores the vCPU fd
number in kvm_state->kvm_parked_vcpus:

    vcpu->kvm_fd = cpu->kvm_fd;
    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);

kvm_close() then runs close(cpu->kvm_fd). Only cpu->kvm_fd is set to -1, and
the parked entry keeps the closed fd number.

Before this patch kvm_state was NULL after kvm_close(), so this list could not
be reached. With kvm_state kept alive, can kvm_create_vcpu() get a stale fd
from kvm_unpark_vcpu() on a vCPU hotplug after kvm_close()?

kvm_create_vcpu()
  kvm_fd = kvm_unpark_vcpu(s, vcpu_id);

kvm_init_vcpu() would then run mmap() and kvm_vcpu_ioctl() on that number.
The kvm_vcpu_ioctl() guard does not catch this, because kvm_create_vcpu()
has just assigned the stale value to cpu->kvm_fd. If the number has been
reused, an unrelated descriptor would be used.

If no parked entry matches, the new guard in kvm_vm_ioctl() returns 0 for
KVM_CREATE_VCPU:

    kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
    if (kvm_fd < 0) {

Here 0 is accepted as the vCPU fd, so cpu->kvm_fd becomes 0.

The later patch in the series, "kvm: fix coalesced MMIO flush racing with
kvm_close()", only clears coalesced_mmio_ring and calls drain_call_rcu(). It
does not purge the parked list or guard kvm_unpark_vcpu().

[ ... ]

> @@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
>      va_end(ap);
>  
> +    /* kvm_close() has been called: there is no VM to talk to any more */
> +    if (s->vmfd == -1) {
> +        return 0;
> +    }
> +

[Severity: Medium]
Can this blanket return 0 hand out fd 0 to callers of ioctls that return a
new fd? Several of those callers only test for an error return, and the
argument structure is not filled in.

query_stats_cb() and query_stats_schemas_cb() only test for -1:

    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
    if (stats_fd == -1) {
        ...
    }
    query_stats(result, target, names, stats_fd, NULL, errp);
    close(stats_fd);

After kvm_close(), a QMP query-stats or query-stats-schemas command gets
stats_fd == 0. The code then reads from fd 0 and calls close(0), which closes
stdin or whatever owns fd 0.

kvm_create_guest_memfd() tests only fd < 0:

    fd = kvm_vm_ioctl(kvm_state, KVM_CREATE_GUEST_MEMFD, &guest_memfd);
    if (fd < 0) {

A return of 0 is accepted there as a valid guest_memfd.

kvm_create_device() returns the unfilled field as a success:

    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &create_dev);
    if (ret) {
        return ret;
    }
    return test ? 0 : create_dev.fd;

Here create_dev.fd is still -1.

In hw/vfio/helpers.c, vfio_kvm_device_add_fd() has a similar problem:

    if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
        ...
    }
    vfio_kvm_device_fd = cd.fd;

vfio_kvm_device_close() resets vfio_kvm_device_fd to -1, so this path is
taken again on a VFIO hot-add after kvm_close(). cd is initialised with only
.type, so cd.fd is 0 and vfio_kvm_device_fd becomes 0. Later ioctl() and
close() calls then act on stdin. It can be reached through
iommufd_cdev_kvm_device_add() -> vfio_kvm_device_add_fd().

The commit message gives the reason for returning 0 only for interrupt
injection, route commit and ioeventfd:

    Returning an error instead would still kill it: kvm_set_irq() aborts and
    kvm_irqchip_commit_routes() asserts on any failure, and the ioeventfd
    callers abort on a stale errno.

That reasoning does not cover the ioctls that return a descriptor. Should
those return a negative errno in the closed state, or should their callers
check for the closed state?

[ ... ]

> @@ -3409,6 +3418,11 @@ int kvm_vcpu_ioctl(CPUState *cpu, unsigned long type, ...)
>      va_end(ap);
>  
> +    /* kvm_close() has been called: the vCPU fds are gone */
> +    if (cpu->kvm_fd == -1) {
> +        return 0;
> +    }
> +

[Severity: Medium]
Does this have the same problem for vCPU ioctls that return an fd?

kvm_init_vcpu() sets:

    cpu->kvm_vcpu_stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);

If the vCPU fd is closed, this stores 0 as the stats fd.

The follow-up patches in the series change only kvm_close(). They add no
guard for any of these fd-returning callers.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=22