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
