Re: [PATCH v2 04/13] mshv: wire SEV-SNP partition ioctls

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

New issues:
- [Critical] Guest memory regions are unpinned and freed back to the host allocator before the hypervisor restores host access, leading to fatal RMP violations.
- [High] Inconsistent handling of the user-provided guest_pfns array between SEV-SNP ioctls, causing correct input to be corrupted.
- [High] Potential CPU stall in hv_call_import_isolated_pages() due to unbounded loop over user-provided page count without cond_resched().
- [High] Unbounded retry loop in destroy_partition() if destroy_snp_partition_state() fails.
- [High] Module unload race with the delayed_work 'destroy_work' causes a Use-After-Free kernel panic.
- [High] hv_call_set_partition_property compares the raw 64-bit status to HV_STATUS_CALL_PENDING without masking, breaking the completion handler.

Pre-existing issues:
- [High] Inverted logic in mshv_init_async_handler()'s completion check permanently bricks the partition on stray completions.
--

--- Patch [4]: [PATCH v2 04/13] mshv: wire SEV-SNP partition ioctls ---
commit 903e3452cece23d95187a14a4beb770df83a5add
Author: Wei Liu <[email protected]>

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 SNP partition
teardown.

The GPA host-access ioctl converts the guest addresses to GFNs before
issuing the hypercall. The PSP guest-request path restores host access
to the request/response pages if the hypercall fails, so a failed request
does not leave guest pages inaccessible to the host.

> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
> index 4e48c1a9a6308..6d42fdd2c573f 100644
> --- a/drivers/hv/mshv_root_hv_call.c
> +++ b/drivers/hv/mshv_root_hv_call.c
[ ... ]
> @@ -1014,6 +1014,41 @@ int hv_unmap_stats_page(enum hv_stats_object_type type,
>  }
>  
>  #ifdef HV_SUPPORTS_SEV_SNP_GUESTS
> +int hv_call_set_partition_property(u64 partition_id, u64 property_code,
> +				   u64 property_value,
> +				   void (*completion_handler)(void *, u64 *),
> +				   void *completion_data)
> +{
[ ... ]
> +	status = hv_do_hypercall(HVCALL_SET_PARTITION_PROPERTY, input, NULL);
> +	local_irq_restore(flags);
> +
> +	if (unlikely(status == HV_STATUS_CALL_PENDING))

[Severity: High]
Does this code incorrectly compare the raw 64-bit status to
HV_STATUS_CALL_PENDING without masking it?

All other hypercalls use the macro hv_result(status) == HV_STATUS_CALL_PENDING
to strip diagnostic and reserved bits. If this comparison fails because of
upper bits being set, the hypercall completion handler is bypassed, causing
the function to fail with an error when it should wait. This would lead the
partition destruction into an infinite retry loop.

> +		completion_handler(completion_data, &status);
[ ... ]
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 146726cc4e9ba..8c87dd8e0378f 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
[ ... ]
> @@ -1625,6 +1668,289 @@ 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)
> +{
[ ... ]
> +	for (i = 0; i < args.page_count; i++)
> +		gpfns[i] = HVPFN_DOWN(gpfns[i]);
> +

[Severity: High]
Are we inconsistent in how we handle the user-provided guest_pfns array
between SEV-SNP ioctls?

Here, the code applies HVPFN_DOWN() to a field named guest_pfns, treating it
as a GPA instead of a PFN. This shifts the PFNs right by 12 bits, corrupting
the values. 

However, in mshv_partition_ioctl_import_isolated_pages(), the identically
named guest_pfns array is passed directly to the hypercall without
downshifting. This mismatch breaks the API contract and can lead to memory
corruption or hypercall failures when incorrect physical addresses are
provided to the hypervisor.

[ ... ]
> +static long
> +mshv_partition_ioctl_import_isolated_pages(struct mshv_partition *partition,
> +					   void __user *user_args)
> +{
[ ... ]
> +	ret = mshv_init_async_handler(partition);

[Severity: High]
This is a pre-existing issue, but does mshv_init_async_handler() have inverted
logic in its completion check that permanently bricks the partition?

mshv_init_async_handler() has this check:

    if (completion_done(&partition->async_hypercall)) {
        pt_err(partition, "Cannot issue async hypercall while another one in progress!\n");
        return -EPERM;
    }

completion_done() returns true if a completion is completed (done > 0). The
function incorrectly assumes another operation is in progress, when in fact it
means the completion is fully resolved. If a spurious completion fires, it
permanently denies service for any further asynchronous hypercalls for the
partition.

> +	if (ret)
> +		goto out;
> +
> +	ret = hv_call_import_isolated_pages(partition->pt_id, pages,
> +					    args.page_count, args.page_type,
> +					    HV_ISOLATED_PAGE_SIZE_4KB,
> +					    mshv_async_hvcall_handler,
> +					    partition);

[Severity: High]
Can this lead to a CPU stall and kernel watchdog panic?

Userspace can invoke MSHV_IMPORT_ISOLATED_PAGES with a massive page_count.
The while (remaining) loop inside hv_call_import_isolated_pages() will
continuously issue synchronous hypercalls based on this unbounded
user-supplied page count without ever calling cond_resched(), monopolizing
the processor.

[ ... ]
> @@ -1782,6 +2191,9 @@ static void destroy_partition(struct mshv_partition *partition)
[ ... ]
>  	if (partition->pt_initialized) {
> +#ifdef HV_SUPPORTS_SEV_SNP_GUESTS
> +		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);
> +			}

[Severity: Critical]
Does this unpin and free guest memory regions back to the host allocator
before the hypervisor restores host access?

The code calls mshv_region_put() to free the memory to the host buddy
allocator BEFORE destroy_snp_partition_state() successfully transitions the
partition state to INSECURE_DIRTY. If the host reallocates and writes to
these pages before the transition completes, it triggers a fatal RMP violation
and a Machine Check Exception (MCE).

> +
> +			ret = destroy_snp_partition_state(partition);
> +			if (ret) {
> +				pt_err(partition,
> +				       "Failed to destroy SNP state: %d; retrying\n",
> +				       ret);
> +				schedule_delayed_work(&partition->destroy_work, HZ);
> +				return;

[Severity: High]
Could this error path cause an unbounded infinite retry loop?

If the hypervisor rejects an idempotent state transition during partition
destruction, the error path unconditionally schedules a retry without a maximum
retry limit. Because it reschedules destroy_partition() entirely, previously
successful state transitions will be repeated, potentially failing
indefinitely.

[ ... ]
> @@ -2051,6 +2485,8 @@ mshv_ioctl_create_partition(void __user *user_arg, struct device *module_dev)
>  	mutex_init(&partition->pt_irq_lock);
>  
>  	init_completion(&partition->async_hypercall);
> +	INIT_DELAYED_WORK(&partition->destroy_work,
> +			  mshv_partition_destroy_work);

[Severity: High]
Might this delayed work cause a Use-After-Free kernel panic on module unload?

If partition destruction fails and defers via this destroy_work, and userspace
closes the file descriptor, the module reference count will reach zero. The
module exit routine mshv_parent_partition_exit() fails to iterate over
remaining partitions or call cancel_delayed_work_sync(). If an administrator
unloads the module before the delayed work fires, the workqueue will execute
unmapped text.

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