Re: [PATCH v1 13/13] mshv: set up own SynIC registers on a nested root partition

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

Pre-existing issues:
- [Critical] Use-after-free in `mshv_portid_lookup()` due to dereferencing an RCU-protected pointer outside the RCU read-side critical section.
- [High] Use-after-free of `mshv_vp` in `mshv_intercept_isr` and `mshv_scheduler_isr` due to premature `kfree(vp)` in `destroy_partition`.
--

--- Patch [13]: [PATCH v1 13/13] mshv: set up own SynIC registers on a nested root partition ---
commit e9472d7549df2c05d78706a10839e3ca1d79d52c
Author: Wei Hu <[email protected]>

mshv: set up own SynIC registers on a nested root partition
    
Upstream mshv_synic_cpu_init()/_exit() skip programming the SynIC
SIMP/SIEFP/SCONTROL registers when VMBus is active (hv_vmbus_exists()),
assuming VMBus's hv_hyp_synic_enable_regs() already provisioned them.

That assumption is wrong for a *nested* root partition. There, VMBus
programs the *nested* SynIC MSRs (HV_X64_MSR_NESTED_SIMP, ...) via
hv_set_msr()'s nested remap, while mshv_synic reads/writes the
*non-nested* SynIC MSRs (hv_get/set_non_nested_msr). As a result mshv
maps the wrong message page, never receives the SYNIC intercept message
that carries the async hypercall completion, and mshv_async_hvcall_handler
blocks forever -- hanging SEV-SNP guest creation from user space.

Gate the "VMBus owns the SynIC registers" optimization on !hv_nested so
that a nested root partition programs its own non-nested SynIC registers,
restoring correct intercept/async-hypercall delivery. No change for a
non-nested (bare-metal) root.

> diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> index 7c168e5a740dd..7d714ee5d2267 100644
> --- a/drivers/hv/mshv_synic.c
> +++ b/drivers/hv/mshv_synic.c
[ ... ]
> @@ -593,7 +599,13 @@ static int mshv_synic_cpu_exit(unsigned int cpu)
>  	struct hv_synic_event_ring_page **event_ring_page =
>  		&spages->synic_event_ring_page;
>  	/* VMBus owns SIMP/SIEFP/SCONTROL when it is active */
> -	bool vmbus_active = hv_vmbus_exists();
> +	/*
> +	 * On a nested root partition VMBus programs the *nested* SynIC MSRs
> +	 * (SIMP/SIEFP/SCONTROL) via hv_set_msr()'s nested remap, whereas the
> +	 * code below reads/writes the non-nested SynIC MSRs. So when nested,
> +	 * do not defer SynIC setup to VMBus -- set up our own registers.
> +	 */
> +	bool vmbus_active = hv_vmbus_exists() && !hv_nested;
>  
>  	/* Disable the interrupt */
>  	sint.as_uint64 = hv_get_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a pre-existing
use-after-free pattern regarding mshv_vp in mshv_intercept_isr() and
mshv_scheduler_isr().

When userspace initiates partition destruction, destroy_partition() frees
virtual processors before removing the partition from the RCU-protected hash
list:

drivers/hv/mshv_root_main.c:destroy_partition() {
...
			kfree(vp);

			partition->pt_vp_array[i] = NULL;
		}
...
}

Concurrently, hypervisor messages can trigger mshv_intercept_isr() which
reads the VP pointer while holding the rcu_read_lock:

drivers/hv/mshv_synic.c:mshv_intercept_isr() {
...
	vp = partition->pt_vp_array[vp_index];
	if (unlikely(!vp)) {
		pr_debug("failed to find VP %u\n", vp_index);
		goto unlock_out;
	}

	kick_vp(vp);
...
}

Is it possible for kick_vp() to write to freed memory if the read happens
just before kfree(vp) in destroy_partition()?


[Severity: Critical]
This is a pre-existing issue, but does mshv_portid_lookup() have a
use-after-free if it drops the RCU lock before dereferencing the protected
pointer?

drivers/hv/mshv_portid_table.c:mshv_portid_lookup() {
...
	rcu_read_lock();
	_info = idr_find(&port_table_idr, port_id);
	rcu_read_unlock();

	if (_info) {
		*info = *_info;
...
}

Concurrently, mshv_portid_free() can remove the port ID, call
synchronize_rcu(), and free the memory:

drivers/hv/mshv_portid_table.c:mshv_portid_free() {
...
	idr_unlock(&port_table_idr);

	synchronize_rcu();
	kfree(info);
...
}

Since rcu_read_unlock() is called prematurely, synchronize_rcu() wouldn't
wait for the dereference.

This is concerning because mshv_doorbell_isr() uses this returned struct to
invoke a callback:

drivers/hv/mshv_synic.c:mshv_doorbell_isr() {
...
		/* Invoke the callback */
		ptinfo.hv_port_doorbell.doorbell_cb(port,
						 ptinfo.hv_port_doorbell.data);
...
}

Could an attacker trigger port freeing to race with the doorbell interrupt
and control the function pointer?

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