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 <[email protected]> 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/[email protected]?part=4
