Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] Revoking host access to guest pages without invalidating them in the kernel's direct map allows stray kernel accesses to crash the host system. - [High] destroy_partition attempts to restore host access before unmapping memory for uninitialized SEV-SNP partitions, guaranteeing failure and permanent memory leak. - [Medium] Batched IOCTLs abort on failure midway through without reporting progress or unrolling completed operations, permanently bricking partition state. Pre-existing issues: - [Critical] Ignored return value of mshv_region_unmap() in mshv_region_destroy() allows guest access to freed host physical memory. -- --- Patch [4]: [PATCH v3 4/7] mshv: wire SEV-SNP partition ioctls --- commit 438c5ba1c23ba880c2f38d767dbf75dd5ce5c64b Author: Wei Liu <[email protected]> Subject: mshv: wire SEV-SNP partition ioctls Wire the SEV-SNP partition ioctls for GPA host-access changes, isolated-page import, PSP guest requests, AP creation, and ordered SNP partition teardown. Process userspace page arrays in bounded batches. > --- a/drivers/hv/mshv_regions.c > +++ b/drivers/hv/mshv_regions.c [ ... ] > @@ -354,17 +354,19 @@ static void mshv_region_destroy(struct kref *ref) > if (region->mreg_type == MSHV_REGION_TYPE_MEM_MOVABLE) > mshv_region_movable_fini(region); > > - if (mshv_partition_encrypted(partition)) { > + if (mshv_partition_encrypted(partition) && > + !partition->snp_host_access_restored) { > 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", > + "Failed to regain access to memory, retaining > pinned region: %d\n", > ret); > return; > } > } > > - mshv_region_unmap(region); > + if (!partition->snp_regions_unmapped) > + mshv_region_unmap(region); > > mshv_region_invalidate(region); [Severity: Critical] This is a pre-existing issue, but does mshv_region_destroy() ignore the return value of mshv_region_unmap()? If a user issues MSHV_SET_GUEST_MEMORY with the UNMAP flag on an active partition, mshv_unmap_user_memory() is triggered. This removes the region from the list and calls mshv_region_put(). When the refcount drops to 0, mshv_region_destroy() calls mshv_region_unmap() to perform HVCALL_UNMAP_GPA_PAGES. If this hypercall fails due to hypervisor state or invalid parameters, the pages remain mapped in the guest's GPA space. Because the return value is ignored, the code unconditionally proceeds to mshv_region_invalidate(), unpinning the pages and returning them to the host buddy allocator. Can the running guest then trivially read or write these freed pages, resulting in host memory corruption via use-after-free? > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c [ ... ] > @@ -1648,6 +1682,409 @@ mshv_partition_ioctl_initialize(struct mshv_partition > *partition) > return ret; > } > > +#ifdef HV_SUPPORTS_SEV_SNP_GUESTS [ ... ] > +static long > +mshv_partition_ioctl_modify_gpa_host_access(struct mshv_partition *partition, > + void __user *user_args) > +{ [ ... ] > + ret = hv_call_modify_spa_host_access(partition->pt_id, pages, > + count, host_access, flags, > + acquire); [Severity: Critical] When revoking host access from guest pages here, shouldn't we invalidate them in the kernel's direct map? MSHV_MODIFY_GPA_HOST_ACCESS calls hv_call_modify_spa_host_access() with HV_MODIFY_SPA_PAGE_HOST_ACCESS_MAKE_EXCLUSIVE to make pages exclusive to the guest. If these pages are not removed from the Linux kernel's direct map using set_direct_map_invalid_noflush(), they remain mapped on the host while hardware-level access is revoked by the hypervisor. Could speculative execution, prefetching, or background kernel activity accessing these virtual addresses trigger a hardware exception because the underlying physical memory is protected by SEV-SNP RMP rules, leading to a complete host panic? > + if (ret) > + break; [Severity: Medium] If the loop breaks midway due to a validation or hypercall failure, are the completed pages unrolled? Memory arrays are processed in chunks of 4096 pages. Validation checks like mshv_pages_are_contiguous_2m() and the hypercalls are performed inside this loop. If a check fails on a subsequent iteration, the loop breaks and returns an error code. Since the input arguments are passed by value, userspace receives a generic error and cannot determine which pages were modified. Does this leave the VM in a fragmented state where neither the kernel nor userspace can safely clean up or resume the operation? [ ... ] > +static long > +mshv_partition_ioctl_import_isolated_pages(struct mshv_partition *partition, > + void __user *user_args) > +{ [ ... ] > + ret = hv_call_import_isolated_pages(partition->pt_id, pages, > count, > + args.page_type, > args.page_size, > + mshv_async_hvcall_handler, > + partition); > + if (ret) > + break; [Severity: Medium] Does this loop share the same brittle chunking and abort logic as mshv_partition_ioctl_modify_gpa_host_access() mentioned above? [ ... ] > @@ -1805,6 +2324,9 @@ static void destroy_partition(struct mshv_partition > *partition) > struct mshv_mem_region *region; > struct hlist_node *n; > int i; > +#ifdef HV_SUPPORTS_SEV_SNP_GUESTS > + int ret; > +#endif > > if (refcount_read(&partition->pt_ref_count)) { > pt_err(partition, > @@ -1814,7 +2336,87 @@ static void destroy_partition(struct mshv_partition > *partition) > > trace_mshv_destroy_partition(partition->pt_id); > > +#ifdef HV_SUPPORTS_SEV_SNP_GUESTS > + if (!partition->pt_initialized && mshv_partition_encrypted(partition)) { > + int region_ret; > + > + ret = 0; > + hlist_for_each_entry_safe(region, n, > + &partition->pt_mem_regions, hnode) { > + region_ret = mshv_region_share(region); > + if (region_ret) { > + pt_err(partition, > + "Failed to restore host access at GFN > %#llx: %d\n", > + region->start_gfn, region_ret); > + ret = region_ret; > + } > + } > + if (ret) { > + pt_err(partition, > + "Retaining uninitialized partition with > inaccessible SNP pages\n"); > + mshv_quarantine_partition(partition); > + return; > + } [Severity: High] When tearing down an uninitialized SEV-SNP partition, does destroy_partition() fail to restore host access because the memory hasn't been unmapped yet? If a user creates an SNP partition, maps memory (which creates active GPA mappings via mshv_prepare_pinned_region()), and then closes the fd without initializing the partition, this path executes. The code explicitly loops over all memory regions and calls mshv_region_share() to restore host access before unmapping the GPAs, which happens later when mshv_region_destroy() is triggered. Will the hypervisor reject the share request since the pages are still actively mapped in the guest's SLAT? The resulting mshv_region_share() failure triggers mshv_quarantine_partition(), intentionally leaking the partition object and its pinned memory. Could an unprivileged user exploit this to cause host memory exhaustion by rapidly creating, mapping, and destroying uninitialized partitions? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
