Re: [PATCH v5 1/9] mshv: retain memory regions until unmap succeeds

[sashiko-bot@kernel.org]

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

New issues:
- [High] Partial mapping failure for pinned memory causes permanent VM quarantine and memory leak without attempting rollback.
- [Medium] Data race on `region->mapping_may_exist` between the initialization mapping hypercall and the MMU interval notifier.

Pre-existing issues:
- [Critical] Missing verification of contiguous physical pages when mapping 2M chunks to the hypervisor, allowing guests to bypass COW and modify shared huge folios.
- [High] The mmu_interval_notifier invalidate callback returns false on a blockable failure, violating the API contract and causing host use-after-free.
- [High] GFP_KERNEL allocation inside MMU interval notifier invalidate callback causes reclaim deadlocks and atomic context sleep violations.
- [High] mshv_region_destroy calls unpin_user_pages on a NULL array after error paths have already invalidated the region, causing a kernel panic.
- [High] mshv_map_user_memory computes mmio_pfn incorrectly when userspace_addr is not at the VMA start, mapping the wrong physical memory into the guest.
--

commit ae5e74dcd02edb43f5a42fb59ce2354d9c3d2b17
Author: Wei Hu <weh@microsoft.com>

mshv: retain memory regions until unmap succeeds

Do not remove a region from the partition list or unpin its pages until
a checked UnmapGpaPages hypercall has completed. Validate repetition
progress so malformed zero or oversized completion counts cannot spin or
conceal partial state.

> diff --git a/drivers/hv/mshv_regions.c b/drivers/hv/mshv_regions.c
> index 6d65e5b421529..7b1860d3233d3 100644
> --- a/drivers/hv/mshv_regions.c
> +++ b/drivers/hv/mshv_regions.c
[ ... ]
> @@ -245,13 +247,21 @@ static int mshv_region_chunk_remap(struct mshv_mem_region *region,
>  				   u64 page_offset, u64 page_count,
>  				   bool huge_page)
>  {

[Severity: Critical]
This is a pre-existing issue, but I have a question about the huge page path
here. When mapping 2M chunks to the hypervisor, is there any verification
that the intermediate physical pages are contiguous and belong to the same
folio?

In mshv_chunk_stride() and mshv_region_process_chunk(), the code seems to
check only the first page and increments by a stride of 512. If a huge folio
mapped into the host has pages broken by COW (replaced by private anonymous
pages), the hypervisor might still be instructed to map the entire 2M
contiguous physical page. Could this allow a guest to bypass COW and overwrite
shared huge folio memory belonging to other processes?

> +	bool chunk_mapping_may_exist;
> +	int ret;
> +
>  	if (huge_page)
>  		flags |= HV_MAP_GPA_LARGE_PAGE;
>  
> -	return hv_call_map_gpa_pages(region->partition->pt_id,
> -				     region->start_gfn + page_offset,
> -				     page_count, flags,
> -				     region->mreg_pages + page_offset);
> +	ret = hv_call_map_gpa_pages(region->partition->pt_id,
> +				    region->start_gfn + page_offset,
> +				    page_count, flags,
> +				    region->mreg_pages + page_offset,
> +				    &chunk_mapping_may_exist);
> +	if (chunk_mapping_may_exist)
> +		region->mapping_may_exist = true;
> +
> +	return ret;
>  }
[ ... ]
> @@ -337,37 +347,39 @@ static int mshv_region_chunk_unmap(struct mshv_mem_region *region,
[ ... ]
>  static void mshv_region_destroy(struct kref *ref)
>  {
>  	struct mshv_mem_region *region =
>  		container_of(ref, struct mshv_mem_region, mreg_refcount);
> -	struct mshv_partition *partition = region->partition;
> -	int ret;
> +
> +	/* Callers must retain the list reference until cleanup is certain. */
> +	if (WARN_ON_ONCE(region->mapping_may_exist))
> +		return;
>  
>  	if (region->mreg_type == MSHV_REGION_TYPE_MEM_MOVABLE)
>  		mshv_region_movable_fini(region);
>  
> -	if (mshv_partition_encrypted(partition)) {
> -		ret = mshv_region_share(region);
> -		if (ret) {
> -			pt_err(partition,
> -			       "Failed to regain access to memory, unpinning user pages will fail and crash the host error: %d\n",
> -			       ret);
> -			return;
> -		}
> -	}
> -
> -	mshv_region_unmap(region);
> -
>  	mshv_region_invalidate(region);

[Severity: High]
This is a pre-existing issue, but I am concerned about calling
mshv_region_invalidate() here unconditionally.

On error during mshv_prepare_pinned_region(), the code calls
mshv_region_invalidate_pages(), which unpins the pages and zero-initializes
the mreg_pages array. The error then propagates to mshv_map_user_memory()
which drops the refcount and triggers this mshv_region_destroy().

Because mshv_region_destroy() calls mshv_region_invalidate() again, it will
pass the now-NULL mreg_pages array elements to unpin_user_pages().
Will this result in a kernel panic when unpin_user_pages() dereferences the
NULL page pointers?

> -
>  	vfree(region);
>  }
[ ... ]
> @@ -531,6 +532,12 @@ static bool mshv_region_interval_invalidate(struct mmu_interval_notifier *mni,
>  	page_offset = HVPFN_DOWN(mstart - region->start_uaddr);
>  	page_count = HVPFN_DOWN(mend - mstart);
>  
> +	/*
> +	 * mmap_lock is already held by the notifier core. Take only the region
> +	 * mutex here; taking the fair fault rwsem would deadlock behind an unmap
> +	 * writer that is waiting for this mutex. Publish the sequence after the
> +	 * mutex is acquired even when a checked unmap proved no mapping remains.
> +	 */
>  	if (mmu_notifier_range_blockable(range))
>  		mutex_lock(&region->mreg_mutex);
>  	else if (!mutex_trylock(&region->mreg_mutex))
> @@ -538,15 +545,15 @@ static bool mshv_region_interval_invalidate(struct mmu_interval_notifier *mni,
>  
>  	mmu_interval_set_seq(mni, cur_seq);
>  
> -	ret = mshv_region_remap_pages(region, HV_MAP_GPA_NO_ACCESS,
> -				      page_offset, page_count);
> -	if (ret)
> -		goto out_unlock;
> +	if (READ_ONCE(region->mapping_may_exist)) {
> +		ret = mshv_region_remap_pages(region, HV_MAP_GPA_NO_ACCESS,
> +					      page_offset, page_count);
> +		if (ret)
> +			goto out_unlock;
> +	}

[Severity: High]
This is a pre-existing issue, but I am worried about memory allocations here.

mshv_region_interval_invalidate() is invoked by memory reclaim or atomic
contexts. It calls mshv_region_remap_pages() which triggers a hypercall.
If the hypervisor requires memory, it calls hv_call_deposit_pages(), which
allocates memory using alloc_page(GFP_KERNEL).

Does using sleepable allocations (GFP_KERNEL) from reclaim context cause
deadlocks, or from atomic context trigger a sleeping-in-atomic bug?

[Severity: Medium]
Also, is it safe to use READ_ONCE(region->mapping_may_exist) here when
the writer in hv_do_map_gpa_hcall() uses a plain assignment
(*mapping_may_exist = true)?

Since mmu_interval_notifier_insert() could be called before the hypercall,
mshv_region_interval_invalidate() might run concurrently. Under the C memory
model, this data race could lead to missed invalidations if the compiler
reorders or tears the write.

>  
>  	mshv_region_invalidate_pages(region, page_offset, page_count);
> -
>  	mutex_unlock(&region->mreg_mutex);
> -
>  	return true;
>  
>  out_unlock:

[Severity: High]
This isn't a bug introduced by this patch, but returning false on a
blockable failure here violates the mmu_interval_notifier API contract.

The core MM subsystem strictly forbids returning false when
mmu_notifier_range_blockable(range) is true. Doing so causes a WARN_ON and
ignores the failure, which means the host proceeds to free or reuse the page
while the hypervisor retains guest access.
Could this lead to a cross-process use-after-free reachable under memory
pressure or untrusted guest action?

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index cc2cfce2aefdb..bb6f16fac4de1 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
[ ... ]
> @@ -1397,13 +1398,15 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  		ret = hv_call_map_gpa_pages(partition->pt_id,
>  					    region->start_gfn,
>  					    region->nr_pages,
> -					    HV_MAP_GPA_NO_ACCESS, NULL);
> +					    HV_MAP_GPA_NO_ACCESS, NULL,
> +					    &region->mapping_may_exist);
>  		break;
>  	case MSHV_REGION_TYPE_MMIO:
>  		ret = hv_call_map_mmio_pages(partition->pt_id,
>  					     region->start_gfn,
>  					     mmio_pfn,
> -					     region->nr_pages);
> +					     region->nr_pages,
> +					     &region->mapping_may_exist);

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the mmio_pfn
computation earlier in this function:

	mmio_pfn = is_mmio ? vma->vm_pgoff : 0;

Does this completely ignore the page offset of the requested userspace_addr
within the VMA? If a user requests a mapping for a sub-region of the VMA,
would the guest incorrectly be mapped to the very beginning of the VMA's
physical address space instead of the requested offset?

>  		break;
>  	}
>  
> @@ -1421,7 +1424,17 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  	return 0;
>  
>  errout:
> -	mshv_region_put(region);
> +	if (region->mapping_may_exist) {
> +		spin_lock(&partition->pt_mem_regions_lock);
> +		hlist_add_head(&region->hnode, &partition->pt_mem_regions);
> +		spin_unlock(&partition->pt_mem_regions_lock);
> +		pt_err(partition,
> +		       "Retaining region after uncertain map cleanup at GFN %#llx\n",
> +		       region->start_gfn);
> +		mshv_quarantine_partition(partition);
> +	} else {
> +		mshv_region_put(region);
> +	}

[Severity: High]
If a later chunk fails to map during mshv_prepare_pinned_region(), the
earlier chunks remain mapped, leaving region->mapping_may_exist as true.

When the error propagates back here and mapping_may_exist is true, the
driver immediately calls mshv_quarantine_partition(), permanently leaking
the partition and module.

Should this code attempt to call mshv_region_unmap() on the error path to
clean up the partially mapped chunks before resorting to quarantine, to
prevent a permanent DoS via resource exhaustion?

>  	return ret;
>  }

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