On Thu, May 07, 2026 at 03:43:09PM +0000, Stanislav Kinsburskii wrote:
> mshv_prepare_pinned_region() returns 0 (success) when mshv_region_map()
> fails on an unencrypted partition. The condition on the error path:
>
> if (ret && mshv_partition_encrypted(partition))
>
> only handles map failures for encrypted partitions — if the partition is
> not encrypted and the map fails, execution falls through to 'return 0',
> silently ignoring the error.
>
> Additionally, calling mshv_region_invalidate() inline on map failure
> zeroes the mreg_pages array before the caller's cleanup path
> (mshv_region_destroy) can call mshv_region_unmap(). Since unmap skips
> pages where mreg_pages[offset] is NULL, this can leave stale SLAT
> mappings for partially-mapped pages.
>
> Fix by returning immediately on success and falling through to error
> return on failure. For unencrypted partitions, the caller's
> mshv_region_destroy() handles unmap followed by invalidate in the
> correct order. For encrypted partitions where re-sharing fails, zero
> the page array without unpinning — the pages are inaccessible to the
> host and must not be unpinned, but zeroing prevents
> mshv_region_destroy() from attempting to unpin them.
mshv_region_destroy() calls mshv_region_invalidate() which calls
mshv_region_invalidate_pages() which just does:
static void mshv_region_invalidate_pages(struct mshv_mem_region *region,
u64 page_offset, u64
page_count)
{
if (region->mreg_type == MSHV_REGION_TYPE_MEM_PINNED)
unpin_user_pages(region->mreg_pages + page_offset,
page_count);
memset(region->mreg_pages + page_offset, 0,
page_count * sizeof(struct page *));
}
It doesn't checked for zeroed pages before unpinning.
>
> Fixes: 621191d709b14 ("Drivers: hv: Introduce mshv_root module to expose
> /dev/mshv to VMMs")
> Signed-off-by: Stanislav Kinsburskii <[email protected]>
> ---
> drivers/hv/mshv_root_main.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 665d565899c15..7e4252b6bc65c 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1360,32 +1360,38 @@ static int mshv_prepare_pinned_region(struct
> mshv_mem_region *region)
> pt_err(partition,
> "Failed to unshare memory region (guest_pfn:
> %llu): %d\n",
> region->start_gfn, ret);
> - goto invalidate_region;
> + goto err_out;
> }
> }
>
> ret = mshv_region_map(region);
> - if (ret && mshv_partition_encrypted(partition)) {
> + if (ret)
> + goto share_region;
> +
> + return 0;
> +
> +share_region:
> + if (mshv_partition_encrypted(partition)) {
> int shrc;
>
> shrc = mshv_region_share(region);
> if (!shrc)
> - goto invalidate_region;
> + goto err_out;
>
> pt_err(partition,
> "Failed to share memory region (guest_pfn: %llu): %d\n",
> region->start_gfn, shrc);
> /*
> - * Don't unpin if marking shared failed because pages are no
> - * longer mapped in the host, ie root, anymore.
> + * Re-sharing failed — the pages remain inaccessible to the
> + * host. Zero the page array so that mshv_region_destroy()
> + * won't attempt to unpin them (leaking the page references
> + * is intentional; unpinning host-inaccessible pages would be
> + * unsafe).
> */
Actually, mshv_region_destroy() also does mshv_region_share(). Maybe we
can remove it from here altogether. Either way, should this zeroing
logic be added there too so as not to crash the host by unpinning pages
that weren't marked shared?
>From mshv_region_destroy():
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;
}
}
Thanks,
Anirudh.
