Re: [PATCH v5 8/9] mshv: Populate mmio mappings for PCI passthru

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

New issues:
- [Critical] Insecure MMIO region bounds check and PFN extrapolation allows=
 mapping arbitrary host RAM into the guest.
- [Critical] Conflation of Host PFNs with Hyper-V 4KB PFNs causes arbitrary=
 memory mapping on architectures with >4KB pages (ARM64).
- [Critical] Missing MMU interval notifier allows stale physical mappings (=
TOCTOU and Use-After-Unmap).
- [High] Use-After-Free of VMA pointer after dropping mmap_read_lock.
- [High] Alignment check logic error causes hypercall protocol violation fo=
r large pages.
- [High] Overloading `mmio_mfn` with `0` bypasses `page_is_ram()` checks fo=
r PFN 0.

Pre-existing issues:
- [High] Incorrect unit passed to unmap function on error path causes silen=
t resource leak.
- [High] Unbounded O(N) `page_is_ram()` checks in process context cause CPU=
 soft lockups.
--

--- Patch [8]: [PATCH v5 8/9] mshv: Populate mmio mappings for PCI passthru=
 ---
commit ef68862e98379b2ba894e7946e9748f29e46d61f
Author: Mukesh R <[email protected]>
Subject: mshv: Populate mmio mappings for PCI passthru

This commit handles hypervisor unmapped GPA intercepts by resolving the PCI
resource PFN and mapping it via hypercall. It attempts to map 2M pages where
possible to improve performance and removes broken VMA offset code.

> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_cal=
l.c
> index cb55d4d4be2e5..4f6eb3a2aed03 100644
> --- a/drivers/hv/mshv_root_hv_call.c
> +++ b/drivers/hv/mshv_root_hv_call.c
[ ... ]
> -static int hv_do_map_gpa_hcall(u64 partition_id, u64 gfn, u64 page_struc=
t_count,
> -			       u32 flags, struct page **pages, u64 mmio_spa)
> +static int hv_do_map_gpa_hcall(u64 partition_id, u64 gfn, u64 page_count,
> +			       u32 flags, struct page **pages, u64 mmio_mfn)
>  {
[ ... ]
> -	if (page_count =3D=3D 0 || (pages && mmio_spa))
> -		return -EINVAL;
> +	if (mmio_mfn) {
> +		if (pages)
>  			return -EINVAL;
> +		for (i =3D 0; i < page_count; i++)
> +			if (page_is_ram(mmio_mfn + i))
> +				return -EINVAL;
> +	}

[Severity: High]
Does this check evaluate to false when a legitimate MMIO region points to
physical PFN 0?

If mmio_mfn is 0, it appears to bypass the page_is_ram() loop entirely. Cou=
ld
this allow a VMM process to map the host's physical RAM page 0 into the gue=
st
as MMIO?

[Severity: High]
This is a pre-existing issue, but does this loop iterate over a potentially
massive number of pages without yielding?

Since page_count is derived from the user-provided region size, a malicious=
ly
large region might force the host CPU into a soft lockup. Should we add a
cond_resched() here?

[ ... ]
>  		} else if (!hv_result_success(status)) {
> +			pr_err("%s: failed to map pages at gfn %#llx: completed %u/%llu, flag=
s=3D%#x, status=3D%#llx (%s)\n",
> +			       __func__, gfn, done, page_count, flags, status,
> +			       hv_result_to_string(hv_result(status)));
>  			ret =3D hv_result_to_errno(status);
>  			break;
>  		}

[Severity: High]
This is a pre-existing issue, but does the error rollback path near the end
of this function pass the wrong units to hv_call_unmap_gpa_pages()?

The variable done tracks successfully mapped large pages, but
hv_call_unmap_gpa_pages() expects its page_count argument in 4KB units. If =
we
pass the large page count instead, it seems like it will fail the alignment
check and silently leak dangling large page mappings in the hypervisor's
stage 2 tables.

[ ... ]
> +int hv_map_mmio_pages(u64 partition_id, struct mshv_mem_region *reg,
> +		      u64 mmio_mfn)
>  {
[ ... ]
> +	while (!HV_PAGE_COUNT_2M_ALIGNED(gfn) &&
> +	       !HV_PAGE_COUNT_2M_ALIGNED(mmio_mfn) &&
> +	       numpgs < reg->nr_pages) {
> +		numpgs++;
> +		gfn++;
> +		mmio_mfn++;
> +	}

[Severity: High]
Does the logical AND operator here cause the alignment loop to exit
prematurely?

If the offsets of gfn and mmio_mfn relative to a 2MB boundary differ, the l=
oop
will break as soon as only one of them becomes aligned, leaving the other
unaligned.

Will this cause us to pass unaligned addresses to a large-page hypercall la=
ter
in this function, violating the Hyper-V protocol?

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index fc08e994ecdf5..b45c6b275f999 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
[ ... ]
> +static bool mshv_handle_unmapped_gpa(struct mshv_vp *vp)
> +{
[ ... ]
> +	mmio_mfn =3D mmio_mfn - (gfn - rg->start_gfn);   /* start of the range =
*/

[Severity: Critical]
Could this backwards extrapolation yield arbitrary physical addresses if a
region spans outside its intended VMA? By extrapolating the base PFN for the
entire region backwards, it looks like it could map adjacent, unauthorized =
host
physical memory into the guest.

Also, on architectures with page sizes larger than 4KB (like ARM64), does t=
his
mix incompatible units?

The function mshv_chk_get_mmio_start_pfn() returns a host PFN (e.g. 64KB
scaled), but this subtracts a 4KB-based Hyper-V GFN offset. Passing this
corrupted value to the hypervisor could cause incorrect physical addresses =
to
be mapped.

> +
> +	/* Map the entire mmio region now. mmap lock not needed because we
> +	 * are just mapping GPA to static device addresses, and the above
> +	 * mshv_partition_region_by_gfn_get() gets a hold on the region and
> +	 * hence its GPA. Unmap will occur when region ref count goes to 0.
> +	 */
> +	rc =3D hv_map_mmio_pages(pt->pt_id, rg, mmio_mfn);

[Severity: Critical]
Are these physical memory mappings protected against stale access?

Because MMIO regions do not register an mmu_interval_notifier, there doesn't
appear to be a mechanism to invalidate the hypervisor's translation tables =
if
the device is unbound or the VMA is unmapped.

Furthermore, dropping the mmap lock earlier seems to introduce a TOCTOU race
where a concurrent munmap could invalidate the PFN before the hypercall is
made. Will this allow stale mappings to persist?

> @@ -1340,7 +1424,6 @@ mshv_map_user_memory(struct mshv_partition *partiti=
on,
[ ... ]
>  	mmap_read_lock(current->mm);
>  	vma =3D vma_lookup(current->mm, mem->userspace_addr);
>  	is_mmio =3D vma ? !!(vma->vm_flags & (VM_IO | VM_PFNMAP)) : 0;
> -	mmio_pfn =3D is_mmio ? vma->vm_pgoff : 0;
>  	mmap_read_unlock(current->mm);
> =20
>  	if (!vma)
>  		return -EINVAL;
> =20
> +	if (is_mmio) {
> +		size_t vma_sz =3D vma->vm_end - vma->vm_start;

[Severity: High]
Can this result in a use-after-free?

The vma pointer is unconditionally dereferenced here, but the lock is dropp=
ed
just above it with mmap_read_unlock(current->mm).

Could a concurrent thread in the VMM process calling munmap() unlink and
free the VMA before this calculation occurs, leading to memory corruption?

> +
> +		/* Upon mmio intercept, entire region is mapped */
> +		if (mem->size > vma_sz)
> +			return -EPERM;
> +	}

[Severity: Critical]
Does this bounds check properly validate the starting offset?

It only compares the total size against vma_sz, neglecting to check if the
region's start offset fits within the VMA. For example, it doesn't verify
that userspace_addr + size <=3D vm_end.

Would this allow a malicious userspace VMM to create a region that spans
outside its intended VMA?

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