> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Monday, August 31, 2026 7:49 PM
> To: Wei Hu <[email protected]>
> Cc: [email protected]
> Subject: [EXTERNAL] Re: [PATCH v4 1/9] mshv: retain memory regions until
> unmap succeeds
>
> 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.
Hello Sashiko,
The pre-existing issues will be addressed in a separate patch set. They will
not be resolved in the future revisions of this patch set.
Thanks,
Wei
> --
>
> commit bdfaa07680341c254374f6fa979c3fcd550c4840
> Author: Wei Hu <[email protected]>
>
> 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(®ion->mreg_remap_lock);
> > + if (READ_ONCE(region->remap_blocked) ||
> > + !mmget_not_zero(region->mreg_mni.mm)) {
> > + up_read(®ion->mreg_remap_lock);
> > + ret = -ECANCELED;
> > + goto out;
> > + }
> > + mm = region->mreg_mni.mm;
> > + up_read(®ion->mreg_remap_lock);
> > +
> > + /* Lock order: mmap_lock -> mreg_remap_lock -> mreg_mutex. */
> > + mmap_read_lock(mm);
> > + down_read(®ion->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(®ion->mreg_remap_lock);
> > + else if (!down_read_trylock(®ion->mreg_remap_lock))
> > + return false;
> > +
> > + /* A completed checked unmap makes notifier remapping
> unnecessary. */
> > + if (READ_ONCE(region->remap_blocked)) {
> > + up_read(®ion->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(®ion->mreg_mutex);
> > out_fail:
> > + up_read(®ion->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,
> > + ®ion->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,
> > + ®ion->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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Fsashiko.dev%2F%23%2Fpatchset%2F20260831112704.2851147-1-
> weh%40linux.microsoft.com%3Fpart%3D1&data=05%7C02%7Cweh%40micr
> osoft.com%7Cf41ea7e6afe44d8b0d6208df0755cacd%7C72f988bf86f141af
> 91ab2d7cd011db47%7C1%7C0%7C639237737234695849%7CUnknown%
> 7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAi
> OiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdat
> a=nKKIfw5wbEV33vK8LJGwQNDyA3zm%2F5DYBZgHWDH0x2U%3D&reserve
> d=0
