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