Re: [PATCH QEMU HCI-8.1 v7 23/35] 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: kvm_vm_ioctl() returns 0 after kvm_close() for fd-returning ioctls, yielding fd 0
- [Medium] kvm: ioctl wrappers return 0 after kvm_close() without filling outputs
- [Medium] kvm: ioctl wrappers return 0 after kvm_close(), callers treat it as a valid fd
- [Low] kvm: closed-state checks in ioctl wrappers race with kvm_close()
--

commit e7e22762666ee0142be58442e3f90a44a9a53d12
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 file descriptors have been closed. The aim 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]
Can the closed-state checks added below race with kvm_close()?

kvm_close() stores -1 into kvm_state->vmfd and kvm_state->fd with plain
stores after close(). It does not call accel_ioctl_inhibit_begin(), so it
does not wait for in-flight ioctls from threads that don't hold the BQL.

A thread without the BQL could pass the new s->vmfd == -1 test in
kvm_vm_ioctl(). kvm_close() could then close the descriptor. The first
thread would re-read s->vmfd for ioctl() and either get -EBADF or issue
the ioctl on a recycled descriptor number.

kvm_set_irq() and the ioeventfd callers treat that error as fatal. Is
there any synchronization that excludes this?

Also, kvm_enabled() and kvm_dirty_ring_enabled() keep reporting a live KVM
now that kvm_state is no longer set to NULL. Only the three ioctl wrappers
check for the closed state.

> @@ -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: Medium]
Is returning 0 here safe for callers that read output from the ioctl?

This stub never writes the caller's output buffers or return value.
KVM_CHECK_EXTENSION, KVM_GET_VCPU_MMAP_SIZE and KVM_GET_SUPPORTED_* all
report success with no data. kvm_check_extension() would read the 0 as
"extension 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 return 0 be mistaken for a valid file descriptor?

Several VM ioctls return a new fd as their result: KVM_GET_STATS_FD,
KVM_CREATE_GUEST_MEMFD, KVM_CREATE_VCPU and KVM_CREATE_DEVICE (through
cd.fd). After kvm_close() these now return 0, and the callers take that as
fd 0, which is stdin.

The most reachable case is query_stats_cb():

    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
    if (stats_fd == -1) {
        error_setg_errno(errp, errno, "KVM stats: ioctl failed");
        return;
    }
    query_stats(result, target, names, stats_fd, NULL, errp);
    close(stats_fd);

The check for -1 passes, so a QMP query-stats reads from fd 0 and then
calls close(0). query_stats_schemas_cb() has the same pattern.

Other callers:

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

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

kvm_create_guest_memfd() accepts 0 as a guest_memfd. kvm_create_vcpu()
sets cpu->kvm_fd = 0.

In hw/vfio/helpers.c, vfio_kvm_device_add_fd() has this:

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

cd.fd is only initialised for .type, so it stays 0. vfio_kvm_device_fd
becomes 0, and a later vfio_kvm_device_close() calls close(0). It can also
be reached after vfio_kvm_device_close() has reset vfio_kvm_device_fd to
-1, because kvm_enabled() is still true after kvm_close().

openpic_kvm.c and s390_flic_kvm.c use cd.fd after a zero-initialised
KVM_CREATE_DEVICE call in the same way. kvm_create_device() sets
create_dev.fd = -1, but it would still return success with -1 as the fd.

Before this patch these calls dereferenced a NULL kvm_state. Should the
fd-returning requests return an error instead?

Also, for requests that fill an output struct, KVM_GET_CLOCK,
KVM_GET_IRQCHIP, KVM_GET_PIT2 and KVM_GET_DIRTY_LOG return success with
the buffer untouched. Can kvm_update_clock() in hw/i386/kvm/clock.c then
read data.clock from an uninitialised stack struct? Can kvm_pic_get() and
kvm_ioapic_get() copy uninitialised stack data into device state?
kvm_get_current_ns() in target/i386/kvm/xen-emu.c has the same pattern as
kvm_update_clock().

The commit message justifies the 0 only for the interrupt, irqfd and
ioeventfd callers. Does it cover callers that use the return value as a
handle or read the output buffer?

>      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: Medium]
Does the same issue apply to kvm_vcpu_ioctl()?

KVM_GET_STATS_FD on a closed vCPU returns 0, and kvm_init_vcpu() stores
that result in cpu->kvm_vcpu_stats_fd. The vCPU GET_* ioctls also return
success without writing any data. KVM_RUN returns without running.

If kvm_get_msrs() were reached through a stale CPUState, it would hit
assert(ret == nmsrs). cpu_remove_sync() takes the CPUs off CPU_FOREACH in
kvm_close(), so this is probably only reachable through a stale CPUState
pointer.

>      trace_kvm_vcpu_ioctl(cpu->cpu_index, type, arg);
>      accel_cpu_ioctl_begin(cpu);
>      ret = ioctl(cpu->kvm_fd, type, arg);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=23