Re: [PATCH V3 06/11] mshv: Add ioctl support for MSHV-VFIO bridge device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] FD leak and potential deadlock on copy_to_user() failure after anon_inode_getfd().
- [Medium] Asymmetric memory lifecycle and inconsistent device_release handling.
- [Medium] Missing validation of unknown flags in mshv_partition_ioctl_create_device().
--

commit a0a0db24ca9d194302f458a7afa8a424cc357990
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Add ioctl support for MSHV-VFIO bridge device

This commit adds an ioctl for creating MSHV devices for a partition.
Currently only VFIO device types are supported, doing setup for the bridge
code in mshv_vfio.c.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 02c107458be90..6ceb5f608589f 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c

[ ... ]

> @@ -1625,6 +1625,119 @@ mshv_partition_ioctl_initialize(struct mshv_partition *partition)

[ ... ]

> +static int mshv_device_fop_release(struct inode *inode, struct file *filp)
> +{
> +	struct mshv_device *mshv_dev = filp->private_data;
> +	struct mshv_partition *partition = mshv_dev->device_pt;
> +
> +	if (mshv_dev->device_ops->device_release) {
> +		mutex_lock(&partition->pt_mutex);
> +		hlist_del(&mshv_dev->device_ptnode);
> +		mshv_dev->device_ops->device_release(mshv_dev);
> +		mutex_unlock(&partition->pt_mutex);
> +	}

What happens if a future device type does not implement the device_release
callback? It looks like the device would remain indefinitely linked in 
partition->pt_devices while the file descriptor is closed, leading to
list corruption and a memory leak.

Since the device structure is allocated by the core module via kzalloc(),
would it be safer to handle the list removal and generic kfree()
unconditionally in this function?

> +
> +	mshv_partition_put(partition);
> +	return 0;
> +}
> +
> +static const struct file_operations mshv_device_fops = {
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = mshv_device_fop_ioctl,
> +	.release = mshv_device_fop_release,
> +};
> +
> +static long mshv_partition_ioctl_create_device(struct mshv_partition *partition,
> +					       void __user *uarg)
> +{
> +	long rc;
> +	struct mshv_create_device devargk;
> +	struct mshv_device *mshv_dev;
> +	const struct mshv_device_ops *vfio_ops;
> +
> +	if (copy_from_user(&devargk, uarg, sizeof(devargk)))
> +		return -EFAULT;
> +
> +	/* At present, only VFIO is supported */
> +	if (devargk.type != MSHV_DEV_TYPE_VFIO)
> +		return -ENODEV;
> +
> +	if (devargk.flags & MSHV_CREATE_DEVICE_TEST)
> +		return 0;

Should we explicitly validate that no undefined flags are set here? 
If a future kernel version introduces new flags for device creation, older 
kernels will silently ignore them and return success instead of -EINVAL, 
breaking forward compatibility.

> +
> +	/* This is freed later by mshv_vfio_release_device() */
> +	mshv_dev = kzalloc(sizeof(*mshv_dev), GFP_KERNEL_ACCOUNT);
> +	if (mshv_dev == NULL)
> +		return -ENOMEM;
> +
> +	vfio_ops = &mshv_vfio_device_ops;
> +	mshv_dev->device_ops = vfio_ops;
> +	mshv_dev->device_pt = partition;
> +
> +	rc = vfio_ops->device_create(mshv_dev);
> +	if (rc < 0) {
> +		kfree(mshv_dev);
> +		return rc;
> +	}
> +
> +	hlist_add_head(&mshv_dev->device_ptnode, &partition->pt_devices);
> +
> +	mshv_partition_get(partition);
> +	rc = anon_inode_getfd(vfio_ops->device_name, &mshv_device_fops,
> +			      mshv_dev, O_RDWR | O_CLOEXEC);
> +	if (rc < 0)
> +		goto undo_out;
> +
> +	devargk.fd = rc;
> +	if (copy_to_user(uarg, &devargk, sizeof(devargk)))
> +		return -EFAULT;    /* cleanup in mshv_device_fop_release() */

Does this sequence leak the file descriptor if copy_to_user() fails?

The anon_inode_getfd() call installs the file descriptor into the process's 
fd table. If the subsequent copy_to_user() fails, the function returns 
-EFAULT, but the user space application never receives the fd number to 
explicitly close it, resulting in a leaked file descriptor.

Additionally, can this trigger a deadlock?

The ioctl is called with partition->pt_mutex held. If a malicious user 
provides a buffer backed by userfaultfd, the copy_to_user() call will 
block, holding the mutex. Concurrently, another thread could find the 
new fd in /proc/self/fd and call close(), which invokes 
mshv_device_fop_release(). That release function unconditionally attempts 
to acquire partition->pt_mutex, resulting in a deadlock.

Would it be better to separate file allocation and installation by using 
get_unused_fd_flags(), anon_inode_getfile(), and only calling fd_install() 
after copy_to_user() succeeds?

> +
> +	return 0;
> +
> +undo_out:
> +	hlist_del(&mshv_dev->device_ptnode);
> +	vfio_ops->device_release(mshv_dev);    /* will kfree(mshv_dev) */

If a future device type does not implement the device_release callback,
could this result in a NULL pointer dereference?

> +	mshv_partition_put(partition);
> +	return rc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512020259.1678627-1-mrathor@linux.microsoft.com?part=6