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 <[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://sashiko.dev/#/patchset/[email protected]?part=1
