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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing memory decryption for locally allocated L1VH SIRBP in Confidential VMs.
--

commit 0a5809fa9d9b76912e599a03beec81fda2326b1b
Author: Wei Hu <weh@microsoft.com>

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

This patch modifies mshv_synic to set up its own SynIC registers on nested
root partitions instead of assuming VMBus has provisioned them. It also
updates the mapping of hypervisor-provided SIMP, SIEFP, and root SIRBP pages
to use MEMREMAP_DEC to support confidential hosts.

> diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> index 0fdbae1e053c3..8470cf958f21b 100644
> --- a/drivers/hv/mshv_synic.c
> +++ b/drivers/hv/mshv_synic.c

[ ... ]

> @@ -511,8 +524,8 @@ static int mshv_synic_cpu_init(unsigned int cpu)
>  	sirbp.as_uint64 = hv_get_non_nested_msr(HV_MSR_SIRBP);
>  
>  	if (hv_root_partition()) {
> -		event_ring_page = memremap(sirbp.base_sirbp_gpa << HV_HYP_PAGE_SHIFT,
> -					   HV_HYP_PAGE_SIZE, MEMREMAP_WB);
> +		event_ring_page =
> +			mshv_synic_map_shared_page(sirbp.base_sirbp_gpa);
>  
>  		if (!event_ring_page)
>  			goto cleanup_siefp;

[Severity: High]
Since this patch adds support for confidential hosts by mapping
hypervisor-provided pages with MEMREMAP_DEC, what happens to the locally
allocated SIRBP in the else branch for L1VH partitions?

If we look further down in mshv_synic_cpu_init():

	} else {
		/*
		 * On L1VH the hypervisor does not provide a SIRBP page.
		 * Allocate one and program its GPA into the MSR.
		 */
		event_ring_page = (struct hv_synic_event_ring_page *)
			get_zeroed_page(GFP_KERNEL);

In a Confidential VM, memory allocated via get_zeroed_page() is encrypted by
default. Because set_memory_decrypted() is never called here, will the
hypervisor be unable to access the event ring, causing a regression in event
delivery for L1VH?

For comparison, drivers/hv/hv.c decrypts its locally allocated SynIC pages
via hv_alloc_page(..., decrypt=true, ...).

[ ... ]

> @@ -595,8 +608,7 @@ static int mshv_synic_cpu_exit(unsigned int cpu)
>  	struct hv_message_page *msg_page;
>  	struct hv_synic_event_flags_page *event_flags_page;
>  	struct hv_synic_event_ring_page *event_ring_page;
> -	/* VMBus owns SIMP/SIEFP/SCONTROL when it is active */
> -	bool vmbus_active = hv_vmbus_exists();
> +	bool vmbus_active = mshv_synic_vmbus_owns_registers();
>  
>  	msg_page = READ_ONCE(spages->hyp_synic_message_page);
>  	event_flags_page = READ_ONCE(spages->synic_event_flags_page);

[Severity: High]
If the allocation in mshv_synic_cpu_init() is updated to decrypt the page,
does the cleanup path in mshv_synic_cpu_exit() need a corresponding change?

Looking at the cleanup path in mshv_synic_cpu_exit():

	if (event_ring_page) {
		if (hv_root_partition())
			memunmap(event_ring_page);
		else
			free_page((unsigned long)event_ring_page);
	}

If the page is decrypted during initialization, will freeing it here without
first calling set_memory_encrypted() return a decrypted page to the kernel
allocator and cause a memory corruption regression?

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