Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Malicious userspace can bypass memory pinning enforcement by forcing copy_to_user() to fail. - [Low] Module parameter `hv_memreg` is exposed as runtime-writable despite being designed as a boot parameter. --
--- Patch [9]: [PATCH V4 9/9] mshv: Disable movable regions upfront if device passthru --- commit c768ee85b3efb1c981444ab02e4e4d365693795e Author: Mukesh R <[email protected]> mshv: Disable movable regions upfront if device passthru If a VM is started with a device attached, the memory regions must be marked non-movable since the device attach hypercall allows the use of SLAT for IOMMU. This patch marks them non-movable upfront upon device passthru and adds a boot parameter to allow a user to pin them manually if anticipated. > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > index 58cf61cb227ba..b085fa13574f0 100644 > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c > @@ -46,6 +46,10 @@ MODULE_DESCRIPTION("Microsoft Hyper-V root partition VMM > interface /dev/mshv"); > #define HV_VP_COUNTER_ROOT_DISPATCH_THREAD_BLOCKED 95 > #endif > > +static uint hv_memreg; /* mshv_mem_region allocations type */ > +module_param(hv_memreg, uint, 0644); [Severity: Low] The commit message states this is a "boot parameter". Is it intentional that this module parameter is created with 0644 permissions? This allows root to change the allocation policy at runtime, whereas 0444 would correctly enforce its boot-only nature. > +MODULE_PARM_DESC(hv_memreg, "0: movable (default), 1: pinned"); > + > struct mshv_root mshv_root; [ ... ] > @@ -1801,6 +1811,9 @@ static long mshv_partition_ioctl_create_device(struct > mshv_partition *partition, > if (copy_to_user(uarg, &devargk, sizeof(devargk))) > return -EFAULT; /* cleanup in mshv_device_fop_release() */ [Severity: High] If userspace passes a read-only memory buffer to the MSHV_CREATE_DEVICE ioctl, copy_from_user() can succeed but copy_to_user() will fault and return -EFAULT. Because the file descriptor has already been installed via anon_inode_getfd() prior to the copy_to_user() call, the user can recover the functional device FD via /proc/self/fd/ even after the ioctl returns an error. Does this early return allow malicious userspace to bypass the memory pinning enforcement by forcing copy_to_user() to fail, intentionally skipping the partition->pt_regions_pinned = true assignment? > > + /* For now, just mark regions pinned if there is device passthru. */ > + partition->pt_regions_pinned = true; > + > return 0; > > undo_out: -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
