Re: [PATCH QEMU HCI-8.1 v4 20/31] kvm: tolerate ioctls after kvm_close()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] kvm: fake 0 return from kvm_vm_ioctl() after kvm_close() is taken as a valid fd
- [Low] kvm: kvm_vm_ioctl() fake success leaves kvm_set_irq() reading uninit event.status
- [Low] kvm: kvm_close() sentinel checks in ioctl wrappers are not synchronized with non-BQL callers
- [Low] kvm: ioctl wrappers return 0 for every ioctl after kvm_close(), faking success
--

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

kvm: tolerate ioctls after kvm_close()

This patch keeps kvm_state allocated after kvm_close() and makes
kvm_ioctl(), kvm_vm_ioctl() and kvm_vcpu_ioctl() return 0 once the
descriptors have been closed. The aim is to stop a retired cpr-transfer
source from crashing on a late interrupt, such as the MSI-X delivery
triggered by net_cleanup().

> 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]
Is kvm_close() synchronized with non-BQL callers of the ioctl wrappers?

kvm_close() is called from vfio_cpr_kvm_close_notifier() on
MIG_EVENT_PRECOPY_DONE, through vfio_kvm_device_close(). It does not call
accel_ioctl_inhibit_begin(). The fds are closed with plain stores and no
barrier:

    close(kvm_state->vmfd);
    kvm_state->vmfd = -1;
    close(kvm_state->fd);
    kvm_state->fd = -1;

The later patch "kvm: fix coalesced MMIO flush racing with kvm_close()"
only clears coalesced_mmio_ring and calls drain_call_rcu(). It does not
add any synchronization to the fd close or to the new checks.

> @@ -3371,6 +3370,11 @@ int kvm_ioctl(KVMState *s, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      va_end(ap);
>  
> +    /* kvm_close() has been called: there is no KVM to talk to any more */
> +    if (s->fd == -1) {
> +        return 0;
> +    }
> +

[Severity: Low]
Can returning 0 for every system-level ioctl cause callers to act on a
success that never happened?

After kvm_close(), kvm_ioctl() fills in no output. For example,
kvm_check_extension() sees 0 for KVM_CHECK_EXTENSION and so reports every
capability as unsupported.

>      trace_kvm_ioctl(type, arg);
>      ret = ioctl(s->fd, type, arg);

[ ... ]

> @@ -3389,6 +3393,11 @@ int kvm_vm_ioctl(KVMState *s, unsigned long type, ...)
>      arg = va_arg(ap, void *);
>      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 fake 0 be mistaken for a valid file descriptor?

Some kvm_vm_ioctl() requests return a new fd on success, and 0 is
indistinguishable from fd 0 (normally stdin). Two paths are affected after
kvm_close().

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);

Both callbacks are still registered after kvm_close(), so a QMP query-stats
or query-stats-schemas would pass fd 0 to query_stats() and then call
close(0).

vfio_kvm_device_add_fd() in hw/vfio/helpers.c has the same problem:

    if (kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd)) {
        ...
    }

    vfio_kvm_device_fd = cd.fd;

cd is initialized with only .type, so cd.fd stays 0. If a VFIO device is
attached after kvm_close(), vfio_kvm_device_fd becomes 0, the next
ioctl(0, KVM_SET_DEVICE_ATTR) fails with ENOTTY, and vfio_kvm_device_close()
would later call close(0).

kvm_create_guest_memfd() and kvm_create_vcpu() check only fd < 0, so they
would also accept 0. I did not establish that either of them can run after
kvm_close() on the source.

The commit message justifies returning 0 only for interrupt and ioeventfd
callers. Were the ioctls that return an fd audited?

[Severity: Low]
Can kvm_set_irq() return an uninitialized value after kvm_close()?

kvm_set_irq() declares struct kvm_irq_level event; without an initializer
and writes only level and irq:

    event.level = level;
    event.irq = irq;
    ret = kvm_vm_ioctl(s, s->irq_set_ioctl, &event);
    if (ret < 0) {
        perror("kvm_set_irq");
        abort();
    }
    return (s->irq_set_ioctl == KVM_IRQ_LINE) ? 1 : event.status;

With the new guard, kvm_vm_ioctl() returns 0 without running the ioctl, so
the abort() check is skipped. On x86, irq_set_ioctl is KVM_IRQ_LINE_STATUS
when KVM_CAP_IRQ_INJECT_STATUS is present, so event.status is returned
uninitialized.

kvm_ioapic_set_irq() reaches this path after kvm_close(), because kvm_state
is no longer NULL:

    delivered = kvm_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
    kvm_report_irq_delivered(delivered);

The legacy PIC path, kvm_pic_set_irq(), does the same. Should event be
zero-initialized, or should kvm_set_irq() return early when the VM is
closed?

The same shape applies to other getters such as KVM_GET_CLOCK,
KVM_GET_IRQCHIP and KVM_GET_DIRTY_LOG, which would report success without
filling the caller's buffer. I could not establish that any of them runs
after kvm_close().

[Severity: Low]
Is there a window between the new check and the ioctl() call?

s->vmfd is read once for the check and again for ioctl(), and the check sits
outside the accel_ioctl_begin()/accel_ioctl_end() region:

    if (s->vmfd == -1) {
        return 0;
    }

    trace_kvm_vm_ioctl(type, arg);
    accel_ioctl_begin();
    ret = ioctl(s->vmfd, type, arg);

If a non-BQL thread passes the check just as kvm_close() closes the fds, the
ioctl() can fail with EBADF. Callers such as kvm_set_irq() and
kvm_irqchip_commit_routes() treat that as fatal. If the fd number has been
reused, the ioctl lands on an unrelated fd.

vCPU threads are stopped by cpu_remove_sync(), and the dirty-ring reaper
takes the BQL. I could not show that every other non-BQL caller is
excluded. This is speculative, and the window is narrow.

>      trace_kvm_vm_ioctl(type, arg);
>      accel_ioctl_begin();
>      ret = ioctl(s->vmfd, type, arg);

[ ... ]

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

[Severity: Low]
Is cpu->kvm_fd == -1 a reliable marker for the closed state?

The guard returns success for any vCPU whose kvm_fd is -1, whether it was
closed or never created. kvm_fd is set to -1 only in kvm_close(), so the
guard is not tied to a dedicated closed state. Vector getters on this path
would also report success without filling the caller's buffer.

I did not establish that this path runs after kvm_close().

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=20