Re: [PATCH v4 6/9] mshv: wire SEV-SNP partition ioctls

[sashiko-bot@kernel.org]

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 <wei.liu@kernel.org>

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/20260831112704.2851147-1-weh@linux.microsoft.com?part=6