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_reg=
ion` cause a NULL pointer dereference in `unpin_user_pages()`.
--

--- Patch [9]: [PATCH v5 9/9] mshv: Disable movable regions upfront if devi=
ce 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 ioc=
tl
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,
> =20
>  	if (is_mmio)
>  		rg->mreg_type =3D 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 =3D 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 =3D=3D 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(stru=
ct mshv_partition *partition,
>  	if (copy_to_user(uarg, &devargk, sizeof(devargk)))
>  		return -EFAULT;    /* cleanup in mshv_device_fop_release() */
> =20
> +	/* For now, just mark regions pinned if there is device passthru. */
> +	partition->pt_regions_pinned =3D 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.=20

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.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260731223427.2554=
[email protected]?part=3D9