Re: [PATCH v5 4/9] mshv: Add ioctl support for MSHV-VFIO bridge device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failing copy_to_user after anon_inode_getfd causes a file descriptor leak and leaves partition state inconsistent.
- [Medium] The MSHV_CREATE_DEVICE_TEST flag ignores all other flags, rendering capability checks broken.
--

--- Patch [4]: [PATCH v5 4/9] mshv: Add ioctl support for MSHV-VFIO bridge device ---
commit a96aa99c275e6a51436ce19dce86ac7d1a20b10d
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Add ioctl support for MSHV-VFIO bridge device

This commit adds ioctl support for creating MSHV VFIO bridge devices in a
partition. It introduces a partition ioctl to verify the VFIO type and sets
up the bridge code.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 39e9ef3674f29..fc08e994ecdf5 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
[ ... ]
> @@ -1625,6 +1625,122 @@ mshv_partition_ioctl_initialize(struct mshv_partition *partition)
[ ... ]
> +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;
> +
> +	if (devargk.flags)	/* currently, only MSHV_CREATE_DEVICE_TEST */
> +		return -EINVAL;

[Severity: Medium]
If userspace calls this with MSHV_CREATE_DEVICE_TEST combined with an
unknown flag, won't this unconditionally return 0 and bypass the check for
unsupported flags?

Could this mislead userspace into thinking a new flag is supported when
probing capabilities on older kernels?

[ ... ]
> +	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() */

[Severity: High]
Does this leak the file descriptor and the associated mshv_dev kernel
structures if copy_to_user() fails?

Since anon_inode_getfd() immediately installs the new file descriptor into
the task's fdtable, returning -EFAULT here leaves the fd permanently open
until the process exits.

Also, because the fd is installed before copy_to_user() is called, could
another thread guess the fd and start using it while the calling thread
receives -EFAULT and assumes device creation failed?

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