Re: [PATCH v5 9/9] mshv: Disable movable regions upfront if device passthru

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Bypass of memory pinning enforcement on device creation via `copy_to_user` failure.

Pre-existing issues:
- [High] Error paths in `mshv_map_user_memory` and `mshv_prepare_pinned_region` cause a NULL pointer dereference in `unpin_user_pages()`.
--

--- Patch [9]: [PATCH v5 9/9] mshv: Disable movable regions upfront if device passthru ---
commit 43489ca84b127042602836c2214d379f535126bf
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 to allow the use of SLAT for IOMMU. This commit introduces a
module parameter to pin regions upfront and updates the device creation ioctl
to mark regions as pinned when a device is passed through.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
[ ... ]
> @@ -1332,7 +1342,7 @@ static int mshv_partition_create_region(struct mshv_partition *partition,
>  
>  	if (is_mmio)
>  		rg->mreg_type = MSHV_REGION_TYPE_MMIO;
> -	else if (mshv_partition_encrypted(partition) ||
> +	else if (mshv_do_pt_regions_pinned(partition) ||
>  		 !mshv_region_movable_init(rg))
>  		rg->mreg_type = MSHV_REGION_TYPE_MEM_PINNED;

[Severity: High]
This is a pre-existing issue, but I noticed an error handling problem with
pinned regions that might be triggered more frequently now.

If pinning fails in mshv_map_user_memory(), we drop the reference:

mshv_map_user_memory() {
    ...
    errout:
        mshv_region_put(region);
        return ret;
}

This triggers mshv_region_destroy() which unconditionally invalidates the
region:

mshv_region_destroy() {
    ...
    mshv_region_invalidate(region);
    vfree(region);
}

If the region pages were already partially unpinned and NULLed out during
the pinning failure, mshv_region_invalidate_pages() will pass those NULL
pointers directly to unpin_user_pages():

mshv_region_invalidate_pages() {
    ...
    if (region->mreg_type == MSHV_REGION_TYPE_MEM_PINNED)
        unpin_user_pages(region->mreg_pages + page_offset, page_count);
}

Since unpin_user_pages() does not check for NULL pointers, does this
sequence result in a NULL pointer dereference when cleaning up after a
pinning failure?

[ ... ]
> @@ -1818,6 +1828,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, just mark regions pinned if there is device passthru. */
> +	partition->pt_regions_pinned = true;
> +
>  	return 0;

[Severity: High]
If copy_to_user() fails here, we return -EFAULT, but the file descriptor
has already been installed into the process's file descriptor table via
anon_inode_getfd() and remains active. 

Could this allow userspace to bypass setting pt_regions_pinned to true
while successfully obtaining and using the device file descriptor? If a
process intentionally passes an invalid pointer, it seems it might be able
to map movable memory while still attaching a device.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9