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

[sashiko-bot@kernel.org]

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