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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing MEMREMAP_DEC flag and shared GPA boundary masking when mapping hypervisor-shared SynIC pages.
- [High] Missing pointer nullification on the error path of mshv_synic_cpu_init leaves a dangling pointer in a per-cpu variable.
--

--- Patch [7]: [PATCH v3 7/7] mshv: set up own SynIC registers on a nested root partition ---
commit 04d92759c80f66439e3b646d824c274dc6edc45b
Author: Wei Hu <weh@microsoft.com>

mshv: set up own SynIC registers on a nested root partition

The commit fixes SynIC register setup for nested root partitions by programming
non-nested MSRs instead of relying on VMBus's nested MSR setup, restoring
correct intercept and async-hypercall delivery.

> 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
> @@ -472,7 +472,13 @@ static int mshv_synic_cpu_init(unsigned int cpu)
>  	 * VMBus owns SIMP/SIEFP/SCONTROL when it is active.
>  	 * See hv_hyp_synic_enable_regs() for that initialization.
>  	 */
> -	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;
>  
>  	/*
>  	 * Map the SYNIC message page. When VMBus is not active the
>  	 * hypervisor pre-provisions the SIMP GPA but may not set
>  	 * simp_enabled — enable it here.
>  	 */
>  	simp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIMP);
>  	if (!vmbus_active) {
>  		simp.simp_enabled = true;
>  		hv_set_non_nested_msr(HV_MSR_SIMP, simp.as_uint64);
>  	}
>  	*msg_page = memremap(simp.base_simp_gpa << HV_HYP_PAGE_SHIFT,
>  			     HV_HYP_PAGE_SIZE,
>  			     MEMREMAP_WB);

[Severity: High]
This isn't a bug introduced by this patch, but does this mapping need to use 
MEMREMAP_DEC and mask the shared_gpa_boundary bit?

If the root partition is running as a confidential VM (such as AMD SEV-SNP or 
Intel TDX), hypervisor-shared pages need to be mapped as decrypted.

Mapping them as encrypted memory could cause the driver to read garbage 
intercept messages and miss doorbell events.

[ ... ]

>  cleanup_simp:
>  	if (*msg_page)
>  		memunmap(*msg_page);

[Severity: High]
This is a pre-existing issue, but does this leave a dangling pointer in the 
per-cpu synic_pages structure?

If memremap() fails for the event flags or ring pages during initialization, 
the error path unmaps *msg_page but doesn't set the per-CPU pointer to NULL. 

Since the Hyper-V callback vector can be shared globally, a shared interrupt 
(like from VMBus) could fire on this CPU after a failure.

If the IDT handler unconditionally calls mshv_isr(), could it bypass a NULL 
check and dereference the unmapped pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825040505.826600-1-weh@linux.microsoft.com?part=7