Re: [PATCH 29/60] kvm: Implement KVM_CREATE_PLANE ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Failed KVM_CREATE_PLANE ioctl leaves a dangling pointer in kvm->planes[id] causing Use-After-Free and Double-Free.
- [Medium] Mixing `guard()` scoped cleanup with `goto`-based error handling in `kvm_vm_ioctl_create_plane()`.
--

commit b6872a36c29c142bcf253335bda2d600adade6a7
Author: Paolo Bonzini <pbonzini@redhat.com>

kvm: Implement KVM_CREATE_PLANE ioctl

Add a new VM ioctl to create a new plane. It returns a new file descriptor
which supports per-plane ioctls.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4dbb7659fa2b8..e5d941b1d6fd6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -5287,6 +5315,49 @@ static int kvm_vm_ioctl_get_stats_fd(struct kvm *kvm)
>  	return fd;
>  }
>  
> +static int kvm_vm_ioctl_create_plane(struct kvm *kvm, unsigned id)
> +{
> +	struct kvm_plane *plane;
> +	struct file *file;
> +	int r, fd;
> +
> +	if (id >= kvm_arch_max_planes(kvm) ||
> +	    WARN_ON_ONCE(id >= KVM_MAX_PLANES))
> +		return -EINVAL;
> +
> +	guard(mutex)(&kvm->lock);

[Severity: Medium]
Does mixing a scoped guard with explicit goto-based cleanup labels violate
subsystem guidelines?

The subsystem documentation states that the expectation is that usage of goto
and cleanup helpers is never mixed in the same function.

> +	if (kvm->planes[id])
> +		return -EEXIST;
> +
> +	fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (fd < 0)
> +		return fd;
> +
> +	plane = kvm_create_plane(kvm, id);
> +	if (!plane) {
> +		r = -ENOMEM;
> +		goto put_fd;
> +	}
> +
> +	kvm_get_kvm(kvm);
> +	file = anon_inode_getfile("kvm-plane", &kvm_plane_fops, plane, O_RDWR);
> +	if (IS_ERR(file)) {
> +		r = PTR_ERR(file);
> +		goto put_kvm;
> +	}
> +
> +	fd_install(fd, file);
> +	return fd;
> +
> +put_kvm:
> +	kvm_put_kvm(kvm);
> +	kvm_destroy_one_plane(plane);

[Severity: Critical]
If anon_inode_getfile() fails, does this error path leave a dangling pointer
in kvm->planes[id]?

Assuming kvm_create_plane() publishes the plane to the array before returning:

virt/kvm/kvm_main.c:kvm_create_plane() {
	...
	kvm->planes[plane_level] = plane;
	...
}

When the error path destroys the plane but fails to clear kvm->planes[id],
could this lead to a double-free later when kvm_destroy_planes() iterates over
the kvm->planes array?

virt/kvm/kvm_main.c:kvm_destroy_planes() {
	...
	if (kvm->planes[i] == NULL)
		continue;
	kvm_destroy_one_plane(kvm->planes[i]);
	...
}

> +put_fd:
> +	put_unused_fd(fd);
> +	return r;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=29