Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Failing `copy_to_user` during device creation bypasses the `pt_regions_pinned` enforcement while leaving the device fully accessible. - [High] Device creation does not validate or pin pre-existing memory regions, allowing an unsafe mix of movable regions and device passthrough. - [High] Missing synchronization between device creation and region creation allows concurrent bypass of the pinning constraint. --
commit add8cd40c15400cf2b19c8993ce14d4b4a7adef6 Author: Mukesh R <[email protected]> mshv: Mark mem regions as non-movable upfront if device passthru This commit ensures that if a VM is started with a device attached, its memory regions are marked as non-movable upfront to allow the use of SLAT for IOMMU. It also adds a boot parameter to disable movable pages globally. > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > index a7864463961bc..ac71534733bd8 100644 > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c [ ... ] > @@ -1303,6 +1307,12 @@ static void mshv_async_hvcall_handler(void *data, u64 > *status) > *status = partition->async_hypercall_status; > } > > +static bool mshv_do_pt_regions_pinned(struct mshv_partition *pt) > +{ > + return pt->pt_regions_pinned || mshv_partition_encrypted(pt) || > + hv_no_movbl_pgs; > +} Is there a potential race condition here between device creation and region creation? pt->pt_regions_pinned is written in mshv_partition_ioctl_create_device() without holding any locks. Concurrently, mshv_partition_create_region() reads this flag via mshv_do_pt_regions_pinned() without synchronization. If a region is created while a concurrent device creation thread is about to set pt_regions_pinned to true, could the region incorrectly be instantiated as movable while a passthrough device is active? [ ... ] > @@ -1808,6 +1818,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() */ > > + /* For now, all regions must be pinned if there is device passthru. */ > + partition->pt_regions_pinned = true; If the copy_to_user() fails, the function returns early and skips setting pt_regions_pinned to true. However, anon_inode_getfd() has already installed the file descriptor in the process's file descriptor table, meaning the device remains fully accessible. Can this allow userspace to bypass the pt_regions_pinned enforcement? Additionally, when a device is attached, this sets pt_regions_pinned to true for future memory region creations. Does this handle the case where a partition already has existing memory regions? If userspace creates movable regions first and then attaches a device, is there a risk that page migration could cause the device to DMA into incorrect physical memory? > return 0; > > undo_out: -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
