[PATCH] mshv: Do not map mmio in map user memory ioctl

[PATCH] mshv: Do not map mmio in map user memory ioctl

[Mukesh R]

Currently, mshv_map_user_memory() snoops into vma to get the mmio
pfn via vm_pgoff. The mmio pfn is no longer stored there and will
cause the system to crash. Moreover, it is not legit to obtain
mmio pfn via that method. So, remove upfront mmio mappings, just
create region and do basic checks. The mmio entries will be populated
on demand during intercepts in future.

Signed-off-by: Mukesh R <mrathor@linux.microsoft.com>
---
 drivers/hv/mshv_root.h         |  1 -
 drivers/hv/mshv_root_hv_call.c | 15 ---------------
 drivers/hv/mshv_root_main.c    | 28 +++++++++++-----------------
 3 files changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/hv/mshv_root.h b/drivers/hv/mshv_root.h
index c61b52db7f00..c0844b28b6a8 100644
--- a/drivers/hv/mshv_root.h
+++ b/drivers/hv/mshv_root.h
@@ -281,7 +281,6 @@ int hv_call_create_partition(u64 flags,
 int hv_call_initialize_partition(u64 partition_id);
 int hv_call_finalize_partition(u64 partition_id);
 int hv_call_delete_partition(u64 partition_id);
-int hv_call_map_mmio_pages(u64 partition_id, u64 gfn, u64 mmio_spa, u64 numpgs);
 int hv_call_map_gpa_pages(u64 partition_id, u64 gpa_target, u64 page_count,
 			  u32 flags, struct page **pages);
 int hv_call_unmap_gpa_pages(u64 partition_id, u64 gpa_target, u64 page_count,
diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
index e8485d949951..1287c0a8d118 100644
--- a/drivers/hv/mshv_root_hv_call.c
+++ b/drivers/hv/mshv_root_hv_call.c
@@ -327,21 +327,6 @@ int hv_call_map_gpa_pages(u64 partition_id, u64 gpa_target, u64 page_count,
 				   flags, pages, 0);
 }
 
-/* Ask the hypervisor to map guest mmio space */
-int hv_call_map_mmio_pages(u64 partition_id, u64 gfn, u64 mmio_spa, u64 numpgs)
-{
-	int i;
-	u32 flags = HV_MAP_GPA_READABLE | HV_MAP_GPA_WRITABLE |
-		    HV_MAP_GPA_NOT_CACHED;
-
-	for (i = 0; i < numpgs; i++)
-		if (page_is_ram(mmio_spa + i))
-			return -EINVAL;
-
-	return hv_do_map_gpa_hcall(partition_id, gfn, numpgs, flags, NULL,
-				   mmio_spa);
-}
-
 int hv_call_unmap_gpa_pages(u64 partition_id, u64 gfn, u64 page_count_4k,
 			    u32 flags)
 {
diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
index f0bfbbbd7bdd..a11756e5c48d 100644
--- a/drivers/hv/mshv_root_main.c
+++ b/drivers/hv/mshv_root_main.c
@@ -1345,16 +1345,8 @@ static int mshv_prepare_pinned_region(struct mshv_mem_region *region)
 }
 
 /*
- * This maps two things: guest RAM and for pci passthru mmio space.
- *
- * mmio:
- *  - vfio overloads vm_pgoff to store the mmio start pfn/spa.
- *  - Two things need to happen for mapping mmio range:
- *	1. mapped in the uaddr so VMM can access it.
- *	2. mapped in the hwpt (gfn <-> mmio phys addr) so guest can access it.
- *
- *   This function takes care of the second. The first one is managed by vfio,
- *   and hence is taken care of via vfio_pci_mmap_fault().
+ * This is called for both user ram and mmio space. The mmio space is not
+ * mapped here, that should happen later during intercept on demand.
  */
 static long
 mshv_map_user_memory(struct mshv_partition *partition,
@@ -1363,7 +1355,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
 	struct mshv_mem_region *region;
 	struct vm_area_struct *vma;
 	bool is_mmio;
-	ulong mmio_pfn;
+	size_t vma_sz;
 	long ret;
 
 	if (mem->flags & BIT(MSHV_SET_MEM_BIT_UNMAP) ||
@@ -1373,12 +1365,18 @@ mshv_map_user_memory(struct mshv_partition *partition,
 	mmap_read_lock(current->mm);
 	vma = vma_lookup(current->mm, mem->userspace_addr);
 	is_mmio = vma ? !!(vma->vm_flags & (VM_IO | VM_PFNMAP)) : 0;
-	mmio_pfn = is_mmio ? vma->vm_pgoff : 0;
+	if (vma && is_mmio)
+		vma_sz = vma->vm_end - mem->userspace_addr;
 	mmap_read_unlock(current->mm);
 
 	if (!vma)
 		return -EINVAL;
 
+	if (is_mmio)
+		/* Upon mmio intercept, entire region is mapped */
+		if (mem->size > vma_sz)
+			return -EPERM;
+
 	ret = mshv_partition_create_region(partition, mem, &region,
 					   is_mmio);
 	if (ret)
@@ -1399,11 +1397,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
 					    region->nr_pages,
 					    HV_MAP_GPA_NO_ACCESS, NULL);
 		break;
-	case MSHV_REGION_TYPE_MMIO:
-		ret = hv_call_map_mmio_pages(partition->pt_id,
-					     region->start_gfn,
-					     mmio_pfn,
-					     region->nr_pages);
+	default:
 		break;
 	}
 
-- 
2.51.2.vfs.0.1

Re: [PATCH] mshv: Do not map mmio in map user memory ioctl

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] mshv: missing Fixes trailer on commit fixing crash in
  mshv_map_user_memory()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] mshv: ignored mshv_region_map() error in
  mshv_prepare_pinned_region() [1]
--

commit f8208ba7fc5113c6d67967e4650125ea72fda573
Author: Mukesh R <mrathor@linux.microsoft.com>

> mshv: Do not map mmio in map user memory ioctl
>
> Currently, mshv_map_user_memory() snoops into vma to get the mmio
> pfn via vm_pgoff. The mmio pfn is no longer stored there and will
> cause the system to crash. Moreover, it is not legit to obtain
> mmio pfn via that method. So, remove upfront mmio mappings, just
> create region and do basic checks. The mmio entries will be populated
> on demand during intercepts in future.
>
> Signed-off-by: Mukesh R <mrathor@linux.microsoft.com>

[Severity: Low]
This fixes a system crash when mapping MMIO memory regions.  Should this
include:

  Fixes: 621191d709b1 ("Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs")

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index cc2cfce2aefdb..f1f143e32c078 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1345,16 +1345,8 @@ static int mshv_prepare_pinned_region(struct mshv_mem_region *region)
>  }
>  
>  /*
> - * This maps two things: guest RAM and for pci passthru mmio space.
> - *
> - * mmio:
> - *  - vfio overloads vm_pgoff to store the mmio start pfn/spa.
> - *  - Two things need to happen for mapping mmio range:
> - *	1. mapped in the uaddr so VMM can access it.
> - *	2. mapped in the hwpt (gfn <-> mmio phys addr) so guest can access it.
> - *
> - *   This function takes care of the second. The first one is managed by vfio,
> - *   and hence is taken care of via vfio_pci_mmap_fault().
> + * This is called for both user ram and mmio space. The mmio space is not
> + * mapped here, that should happen later during intercept on demand.
>   */
>  static long
>  mshv_map_user_memory(struct mshv_partition *partition,
> @@ -1363,7 +1355,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  	struct mshv_mem_region *region;
>  	struct vm_area_struct *vma;
>  	bool is_mmio;
> -	ulong mmio_pfn;
> +	size_t vma_sz;
>  	long ret;
>  
>  	if (mem->flags & BIT(MSHV_SET_MEM_BIT_UNMAP) ||
> @@ -1373,12 +1365,18 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  	mmap_read_lock(current->mm);
>  	vma = vma_lookup(current->mm, mem->userspace_addr);
>  	is_mmio = vma ? !!(vma->vm_flags & (VM_IO | VM_PFNMAP)) : 0;
> -	mmio_pfn = is_mmio ? vma->vm_pgoff : 0;
> +	if (vma && is_mmio)
> +		vma_sz = vma->vm_end - mem->userspace_addr;
>  	mmap_read_unlock(current->mm);
>  
>  	if (!vma)
>  		return -EINVAL;
>  
> +	if (is_mmio)
> +		/* Upon mmio intercept, entire region is mapped */
> +		if (mem->size > vma_sz)
> +			return -EPERM;
> +
>  	ret = mshv_partition_create_region(partition, mem, &region,
>  					   is_mmio);
>  	if (ret)
> @@ -1399,11 +1397,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  					    region->nr_pages,
>  					    HV_MAP_GPA_NO_ACCESS, NULL);
>  		break;
> -	case MSHV_REGION_TYPE_MMIO:
> -		ret = hv_call_map_mmio_pages(partition->pt_id,
> -					     region->start_gfn,
> -					     mmio_pfn,
> -					     region->nr_pages);
> +	default:
>  		break;
>  	}

[Severity: Medium]
This problem wasn't introduced by this patch, but does
mshv_prepare_pinned_region() ignore mshv_region_map() errors on
non-encrypted partitions [1]?

drivers/hv/mshv_root_main.c:mshv_prepare_pinned_region() {
    ...
	ret = mshv_region_map(region);
	if (ret && mshv_partition_encrypted(partition)) {
		int shrc;

		shrc = mshv_region_share(region);
		if (!shrc)
			goto invalidate_region;
    ...
		goto err_out;
	}

	return 0;

invalidate_region:
	mshv_region_invalidate(region);
err_out:
	return ret;
}

When mshv_region_movable_init() fails on a non-encrypted partition,
mshv_partition_create_region() falls back to MSHV_REGION_TYPE_MEM_PINNED.
If mshv_region_map() then fails, mshv_prepare_pinned_region() skips
mshv_region_invalidate() and returns 0, so mshv_map_user_memory() links the
unmapped region into partition->pt_mem_regions with its pages left pinned
and returns success to userspace.

[1] https://sashiko.dev/#/bug/linux-a3d211d1-2dbc-4947-880d-f5809ec51869

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113031343.896489-1-mrathor@linux.microsoft.com?part=1