Re: [PATCH v2 10/13] mshv: unmap SNP memory before state teardown

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

New issues:
- [Critical] The `destroy_work` delayed work is never explicitly canceled during module unload, leading to a potential use-after-free and kernel panic.
- [High] Retrying `destroy_partition` on failure causes partial unmap aborts, permanently leaving remaining chunks of a memory region mapped.
- [Low] Intentional early return in `mshv_region_destroy` permanently leaks the `mshv_mem_region` kernel tracking struct.

Pre-existing issues:
- [High] Direct kfree(vp) without waiting for RCU grace period allows Use-After-Free in concurrent ISRs.
--

--- Patch [10]: [PATCH v2 10/13] mshv: unmap SNP memory before state teardown ---
commit fe90624ce2dba03d1ad11c9e5fd88e8f71d8c3d0
Author: Wei Liu <[email protected]>

mshv: unmap SNP memory before state teardown

Unmap memory regions before destroying SNP partition state, and only
release regions after SNP state is torn down. Dropping regions first
attempts to regain host access while the partition is still secure and
can leave pages inaccessible.

> diff --git a/drivers/hv/mshv_regions.c b/drivers/hv/mshv_regions.c
> index 6d65e5b421529..62c49b4d2a33e 100644
> --- a/drivers/hv/mshv_regions.c
> +++ b/drivers/hv/mshv_regions.c
[ ... ]
> @@ -337,7 +337,7 @@ static int mshv_region_chunk_unmap(struct mshv_mem_region *region,
>                                        page_count, flags);
>  }
>  
> -static int mshv_region_unmap(struct mshv_mem_region *region)
> +int mshv_region_unmap(struct mshv_mem_region *region)
>  {
>         return mshv_region_process_range(region, 0,

[Severity: Low]
Does this error path leak the kernel tracking structure?

When tearing down an SNP encrypted partition and mshv_region_share()
fails, the function returns early to intentionally leave guest memory
pinned and prevent host crashes:

drivers/hv/mshv_regions.c:mshv_region_destroy() {
    ...
        if (ret) {
            pt_err(partition,
                   "Failed to regain access to memory... %d\n", ret);
            return;
        }
    }

    mshv_region_unmap(region);
    mshv_region_invalidate(region);
    vfree(region);
}

Since the mshv_mem_region structure is already removed from the partition
lists and its refcount is zero, could bypassing the vfree(region) call
result in permanently leaking the mreg_pages array and tracking structure?

[ ... ]
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index c32d3375af8c5..0dbf509351bb0 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -2208,10 +2208,8 @@ static void destroy_partition(struct mshv_partition *partition)
>                 if (mshv_partition_encrypted(partition)) {
>                         hlist_for_each_entry_safe(region, n,
>                                                   &partition->pt_mem_regions,
> -                                                 hnode) {
> -                               hlist_del(&region->hnode);
> -                               mshv_region_put(region);
> -                       }
> +                                                 hnode)
> +                               mshv_region_unmap(region);
>  
>                         ret = destroy_snp_partition_state(partition);
>                         if (ret) {

[Severity: High]
Can this retry logic lead to a partial unmap regression when a memory region
is processed a second time?

If destroy_snp_partition_state() fails, the teardown is retried via
destroy_work:

drivers/hv/mshv_root_main.c:destroy_partition() {
    ...
    ret = destroy_snp_partition_state(partition);
    if (ret) {
        ...
        schedule_delayed_work(&partition->destroy_work, HZ);
        return;
    }
}

On the subsequent attempt, mshv_region_unmap() will iterate through the
region chunks again starting from offset 0. Since mreg_pages isn't
cleared during the initial successful unmap, the hypervisor will likely
return an error when attempting to unmap the first chunk again.

mshv_region_process_range() aborts processing immediately on error:

drivers/hv/mshv_regions.c:mshv_region_process_range() {
    ...
    ret = mshv_region_process_chunk(region, flags, page_offset, ...);
    if (ret < 0)
        return ret;
}

Will this completely bypass the unmapping of any remaining chunks that
were not reached in the previous pass?

[Severity: High]
Is it safe to schedule a delayed work task that lacks cancelation during module
unload?

If destroy_snp_partition_state() fails, the teardown is retried via
delayed work. If the partition file descriptor is closed, the module
reference count drops to 0, which allows the administrator to unload the
mshv module.

Looking at the exit path:

drivers/hv/mshv_root_main.c:mshv_parent_partition_exit() {
    ...
    mshv_irqfd_wq_cleanup();
    root_scheduler_deinit();
    mshv_synic_exit();
}

Since mshv_parent_partition_exit() executes without calling
cancel_delayed_work_sync(&partition->destroy_work), can the timer fire
after the kernel module is unloaded and execute freed memory?

[Severity: High]
This is a pre-existing issue, but would this direct free cause a
use-after-free regression in concurrent interrupt service routines?

In destroy_partition(), vp structures are freed directly:

drivers/hv/mshv_root_main.c:destroy_partition() {
    ...
        kfree(vp);
        partition->pt_vp_array[i] = NULL;
    ...
}

Concurrently, the intercept ISR reads these pointers under an RCU lock:

drivers/hv/mshv_synic.c:mshv_intercept_isr() {
    ...
    vp = partition->pt_vp_array[vp_index];
    if (unlikely(!vp)) {
        ...
    }
    kick_vp(vp);
    ...
}

Since drain_all_vps() is skipped for non-root schedulers, intercept
messages can still arrive during partition teardown. If destroy_partition()
executes between the time mshv_intercept_isr() reads the vp pointer and
dereferences it in kick_vp(), could this result in accessing freed memory
because synchronize_rcu() or kfree_rcu() isn't used?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10