Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Error recovery paths unconditionally attempt to re-acquire host access for all pages in a request or region, guaranteeing failure and triggering unwarranted partition quarantine. - [High] `MSHV_MODIFY_GPA_HOST_ACCESS` alters host access without updating the region's `host_access_released` flag, causing host access to be permanently leaked or double-acquired during teardown. - [High] Releasing host access fails to set the MAKE_EXCLUSIVE flag, causing the hypercall to use partition_id 0 and failing to assign the memory to the guest partition. --
commit f76d3f5042ec21e318f98c3b6d0b444fad2c8e2d Author: Wei Liu <[email protected]> mshv: wire SEV-SNP partition ioctls This commit wires the SEV-SNP UAPI into partition creation, memory ownership, isolated-page import, launch completion, PSP guest requests, AP creation, and teardown. It implements tracking for child mappings and released host access per region, with error handling for partial maps and teardowns. > diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c > index 4a15d4bfa925a..424b7da680c16 100644 > --- a/drivers/hv/mshv_root_main.c > +++ b/drivers/hv/mshv_root_main.c [ ... ] > @@ -1297,51 +1331,51 @@ static int mshv_prepare_pinned_region(struct > mshv_mem_region *region) [ ... ] > if (mshv_partition_encrypted(partition)) { > + int recovery_ret; > + > ret = mshv_region_unshare(region); > if (ret) { > - pt_err(partition, > - "Failed to unshare memory region (guest_pfn: > %llu): %d\n", > - region->start_gfn, ret); > + pt_err_ratelimited(partition, > + "Failed to release host access at > GFN %#llx: %d\n", > + region->start_gfn, ret); > + recovery_ret = mshv_region_share(region); [Severity: High] Does the error recovery path in mshv_prepare_pinned_region() (in drivers/hv/mshv_root_main.c) unconditionally attempt to re-acquire host access for all pages in the region? If mshv_region_unshare(region) fails partway through the region, mshv_region_share(region) will iterate over the entire region from 0 to nr_pages. Will this blindly hit the exact same page that caused the initial unshare to fail, predictably turning a localized failure into an unrecoverable failure that permanently quarantines the partition? > + if (recovery_ret) { > + pt_err(partition, > + "Failed to recover host access at GFN > %#llx: %d; retaining region\n", > + region->start_gfn, recovery_ret); > + return ret; > + } > goto invalidate_region; > } > } [ ... ] > @@ -1689,6 +1744,473 @@ mshv_partition_ioctl_initialize(struct mshv_partition > *partition) [ ... ] > +static long > +mshv_partition_ioctl_modify_gpa_host_access(struct mshv_partition *partition, > + void __user *user_args) > +{ [ ... ] > + while (offset < args.page_count) { > + u64 count = min_t(u64, args.page_count - offset, > + MSHV_USER_PAGE_BATCH_SIZE); > + u64 completed = 0; > + u64 i; [ ... ] > + ret = hv_call_modify_spa_host_access(partition->pt_id, pages, > + count, &completed, > + host_access, flags, > acquire); [Severity: High] Will releasing host access via mshv_partition_ioctl_modify_gpa_host_access() in drivers/hv/mshv_root_main.c fail to assign the memory to the guest partition? When userspace issues this ioctl with acquire=false to make pages exclusive to the guest, the flags variable lacks the HV_MODIFY_SPA_PAGE_HOST_ACCESS_MAKE_EXCLUSIVE bit. Because the MAKE_EXCLUSIVE flag is omitted, the underlying helper hv_call_modify_spa_host_access() will not populate input_page->partition_id and it will default to 0. Does this mistakenly assign the memory to the root partition instead of the guest? [Severity: High] Does mshv_partition_ioctl_modify_gpa_host_access() in drivers/hv/mshv_root_main.c alter host access without updating the region's host_access_released flag? When userspace successfully unshares pages here, the change in hypervisor state is not recorded in the kernel's region tracking. Because destroy_partition() relies on the host_access_released boolean, will it skip restoring host access during teardown, leading to host access being permanently leaked or double-acquired, and causing a host hardware memory encryption violation when the memory is later reused? > + if (completed > count) { > + ret = -EPROTO; > + completed = 0; > + } [ ... ] > +static long > +mshv_partition_ioctl_issue_psp_guest_request(struct mshv_partition > *partition, > + void __user *user_args) > +{ [ ... ] > + ret = hv_call_modify_spa_host_access(partition->pt_id, pages, > + ARRAY_SIZE(pages), &completed, > + 0, 0, false); [Severity: High] Is the HV_MODIFY_SPA_PAGE_HOST_ACCESS_MAKE_EXCLUSIVE flag similarly missing here in mshv_partition_ioctl_issue_psp_guest_request() (drivers/hv/mshv_root_main.c)? The flags parameter is passed as 0. Will this also cause the hypercall to use partition_id 0 and fail to assign the memory to the guest partition? > + if (ret) > + goto restore_host_access; > + > + ret = mshv_init_async_handler(partition); > + if (ret) > + goto restore_host_access; > + > + ret = hv_call_issue_psp_guest_request(partition->pt_id, > + HVPFN_DOWN(req.req_gpa), > + HVPFN_DOWN(req.rsp_gpa), > + mshv_async_hvcall_handler, > + partition); > + if (!ret) > + return 0; > + > +restore_host_access: > + { > + int restore_ret; > + > + restore_ret = hv_call_modify_spa_host_access(partition->pt_id, > + pages, > ARRAY_SIZE(pages), > + &completed, > host_access, > + 0, true); [Severity: High] Does this recovery path in mshv_partition_ioctl_issue_psp_guest_request() ignore the completed variable from the original failure? By passing ARRAY_SIZE(pages) instead of the number of successfully released pages, it attempts to restore pages that were never released (or failed to release), guaranteeing that the hypervisor will reject this operation again. Could this predictable secondary failure trigger an unwarranted partition quarantine and memory leak? > + if (restore_ret) { > + pt_err(partition, > + "Failed to restore PSP request page access: > %d\n", > + restore_ret); > + mshv_quarantine_partition(partition); > + if (!ret) > + ret = restore_ret; > + } > + } > + return ret; > +} [ ... ] > @@ -1901,6 +2498,34 @@ static void destroy_partition(struct mshv_partition > *partition) [ ... ] > +#ifdef HV_SUPPORTS_SEV_SNP_GUESTS > + if (mshv_partition_encrypted(partition)) { > + if (partition->pt_initialized) { > + ret = destroy_snp_partition_state(partition); > + if (ret) { > + pt_err(partition, > + "Failed to destroy SNP state: %d; > retaining partition\n", > + ret); > + mshv_quarantine_partition(partition); > + return; > + } > + } > + > + hlist_for_each_entry(region, &partition->pt_mem_regions, hnode) > { > + if (!region->host_access_released) > + continue; > + ret = mshv_region_share(region); [Severity: High] Does this teardown code in destroy_partition() (drivers/hv/mshv_root_main.c) rely entirely on the desynchronized host_access_released boolean? Because mshv_partition_ioctl_modify_gpa_host_access() alters host access without updating this flag, will this skip restoring host access for regions that were dynamically unshared by userspace, leading to improper cleanup and host kernel crashes upon page reuse? > + if (ret) { > + pt_err(partition, > + "Failed to restore host access at GFN > %#llx: %d; retaining partition\n", > + region->start_gfn, ret); > + mshv_quarantine_partition(partition); > + return; > + } > + } > + } > +#endif -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
