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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Early return in MMU interval notifier allows guest-to-host use-after-free because it falsely signals that guest access has been revoked before the hypervisor unmap completes.
- [High] ABBA deadlock between `mreg_remap_lock` rwsem fair-queueing and HMM sequence counter invalidation due to holding locks across a retry loop.

Pre-existing issues:
- [Critical] mshv_region_interval_invalidate() returns false for blockable ranges if remapping fails, leading to silent UAF.
- [Critical] Incorrect MMIO physical address mapped for user memory region, allowing arbitrary physical memory access.
--

commit bdfaa07680341c254374f6fa979c3fcd550c4840
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..9feb49d9d9d49 100644
> --- a/drivers/hv/mshv_regions.c
> +++ b/drivers/hv/mshv_regions.c

[ ... ]

> @@ -452,12 +458,35 @@ static int mshv_region_range_fault(struct mshv_mem_region *region,
>  	range.start = region->start_uaddr + page_offset * HV_HYP_PAGE_SIZE;
>  	range.end = range.start + page_count * HV_HYP_PAGE_SIZE;
>  
> +	/*
> +	 * Pin the mm while the remap gate still guarantees that the interval
> +	 * notifier is registered. Do not acquire mmap_lock while holding the
> +	 * gate: invalidation enters with mmap_lock held for write.
> +	 */
> +	down_read(&region->mreg_remap_lock);
> +	if (READ_ONCE(region->remap_blocked) ||
> +	    !mmget_not_zero(region->mreg_mni.mm)) {
> +		up_read(&region->mreg_remap_lock);
> +		ret = -ECANCELED;
> +		goto out;
> +	}
> +	mm = region->mreg_mni.mm;
> +	up_read(&region->mreg_remap_lock);
> +
> +	/* Lock order: mmap_lock -> mreg_remap_lock -> mreg_mutex. */
> +	mmap_read_lock(mm);
> +	down_read(&region->mreg_remap_lock);
> +	if (READ_ONCE(region->remap_blocked)) {
> +		ret = -ECANCELED;
> +		goto out_unlock_remap;
> +	}
> +
>  	do {
>  		ret = mshv_region_hmm_fault_and_lock(region, &range);
>  	} while (ret == -EBUSY);

[Severity: High]
Does holding mreg_remap_lock across this retry loop create an ABBA deadlock?

If a concurrent page fault and region unmap occur:
1. The fault handler acquires mreg_remap_lock for read and blocks here waiting
   for mmu_interval_read_retry() to clear.
2. The unmap thread queues for the mreg_remap_lock write lock in
   mshv_unmap_user_memory(), blocking new readers due to fair-queueing.
3. The MMU interval invalidation callback mshv_region_interval_invalidate()
   (which would clear the retry loop) attempts to acquire mreg_remap_lock for
   read, but gets stuck behind the queued writer.

Can the fault handler loop forever holding the read lock, freezing the host
kernel?

[ ... ]

> @@ -524,6 +558,17 @@ static bool mshv_region_interval_invalidate(struct mmu_interval_notifier *mni,
>  	unsigned long mstart, mend;
>  	int ret = -EPERM;
>  
> +	if (mmu_notifier_range_blockable(range))
> +		down_read(&region->mreg_remap_lock);
> +	else if (!down_read_trylock(&region->mreg_remap_lock))
> +		return false;
> +
> +	/* A completed checked unmap makes notifier remapping unnecessary. */
> +	if (READ_ONCE(region->remap_blocked)) {
> +		up_read(&region->mreg_remap_lock);
> +		return true;
> +	}

[Severity: Critical]
Does this early return falsely signal that guest access has been revoked before
the hypervisor unmap completes?

If mshv_unmap_user_memory() sets remap_blocked = true but has not yet acquired
the mreg_remap_lock write lock to issue the hypervisor unmap, a racing MMU
invalidation could acquire the read lock first, see remap_blocked == true, and
immediately return true.

Could the MM subsystem then free the page while the hypervisor unmap has not
yet executed, leading to a guest-to-host use-after-free?

[ ... ]

>  out_unlock:
>  	mutex_unlock(&region->mreg_mutex);
>  out_fail:
> +	up_read(&region->mreg_remap_lock);
>  	WARN_ONCE(ret,
>  		  "Failed to invalidate region %#llx-%#llx (range %#lx-%#lx, event: %u, pages %#llx-%#llx, mm: %#llx): %d\n",
>  		  region->start_uaddr,

[Severity: Critical]
This is a pre-existing issue, but does mshv_region_interval_invalidate()
return false on a blockable range if mshv_region_remap_pages() fails?

The core MM __mmu_notifier_invalidate_range_start() explicitly ignores the
return value of blockable notifiers. By returning false instead of ensuring
the mapping is torn down, the driver silently fails the invalidation contract.

Does this allow the core MM subsystem to free or reuse a host page while the
guest VM retains mapped access to it, resulting in a guest-to-host
use-after-free?

[ ... ]

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index cc2cfce2aefdb..1ca14660f9985 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);
>  		break;
>  	}

[Severity: Critical]
This is a pre-existing issue, but does mshv_map_user_memory() allow mapping of
arbitrary physical memory by passing an unadjusted MMIO PFN and unvalidated
size directly to the hypervisor?

By using vma->vm_pgoff directly without adding the intra-VMA offset
corresponding to the userspace_addr, and by failing to validate that the
requested size fits inside vma->vm_end:

drivers/hv/mshv_root_main.c:mshv_map_user_memory() {
	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;
    ...
}

Could a malicious userspace process set userspace_addr to an offset within a
legitimate MMIO VMA (or beyond it) and map arbitrary contiguous physical
memory into the guest VM?

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