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 <[email protected]> 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/[email protected]?part=9
