Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] File descriptor and associated kernel resources leak on `copy_to_user` error path. - [Medium] The MSHV_CREATE_DEVICE_TEST flag check completely ignores other unknown flags, breaking forward compatibility. --
--- Patch [4]: [PATCH V4 4/9] mshv: Add ioctl support for MSHV-VFIO bridge device --- commit 4db495dcb20f06e9a8bed00e54250347faa9f23f Author: Mukesh R <[email protected]> mshv: Add ioctl support for MSHV-VFIO bridge device This commit adds ioctl support for creating MSHV devices for a partition. It currently supports VFIO device types and performs initialization for the bridge code. The implementation includes file operations and partition ioctl handlers. > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > index f1ff45a77b42c..56422f801ee2f 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] Does this logic break forward compatibility by ignoring unknown flags when the test bit is set? If userspace queries support for a future flag by passing: devargk.flags = MSHV_CREATE_DEVICE_TEST | FUTURE_FLAG The kernel will immediately return 0. This could mislead userspace into believing the flag is supported. However, when userspace attempts to use the flag without the test bit, the kernel will reject it. Would it be better to check for unknown flags before checking the test flag? [ ... ] > + 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] Can this leak the file descriptor and associated resources if copy_to_user fails? The file descriptor is installed into the process fd table via anon_inode_getfd(). If the subsequent copy_to_user() fails, the kernel returns -EFAULT, but userspace does not receive the allocated fd number and cannot close it. Should this approach use get_unused_fd_flags() and anon_inode_getfile(), delaying fd_install() until after copy_to_user() succeeds? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
