Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing reader-side acquire barrier and missing reset of `gc->max_num_cqs` on teardown allows a malicious host to bypass the `cq_table` bounds check on resume, leading to an out-of-bounds read and potential control flow hijack. - [High] A spoofed `GDMA_EQE_HWC_INIT_EQ_ID_DB` event overwrites the hardware channel doorbell ID without bounds checking, leading to a guest kernel panic. -- --- Patch [7]: [PATCH net v4 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated --- commit cb30244d24849d9dc1bb09cc690c55b40f642e06 Author: Long Li <[email protected]> net: mana: keep max_num_cqs immutable once cq_table is allocated This commit prevents post-init HWC events from inflating the CQ count bound past the cq_table allocation size. It stores the reported max_num_cqs in the channel context and commits it once during channel establishment. This ensures the allocation size and the bounds check remain consistent. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 2f0dae3539552..03b5e2f02e352 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -246,7 +246,15 @@ static void mana_hwc_init_event_handler(void *ctx, > struct gdma_queue *q_self, > break; > > case HWC_INIT_DATA_MAX_NUM_CQS: [Severity: High] This is a pre-existing issue, but can a spoofed GDMA_EQE_HWC_INIT_EQ_ID_DB event cause an out-of-bounds MMIO write? Looking at mana_hwc_init_event_handler(), it blindly trusts the doorbell ID provided by the host: case GDMA_EQE_HWC_INIT_EQ_ID_DB: eq_db.as_uint32 = event->details[0]; hwc->cq->gdma_eq->id = eq_db.eq_id; gd->doorbell = eq_db.doorbell; break; The next time the driver rings the doorbell in mana_gd_ring_doorbell(), it uses this unvalidated spoofed ID to calculate the MMIO address: void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index; ... writeq(e.as_uint64, addr); Could this result in writing outside the mapped bar0_va region and cause a guest kernel panic? > - gd->gdma_context->max_num_cqs = val; > + /* Store, don't apply: mana_hwc_establish_channel() > + * commits this to gc->max_num_cqs once, together > + * with sizing cq_table, so a spoofed post-init event > + * cannot inflate the bound past the allocation. > + * WRITE_ONCE() pairs with the READ_ONCE() there: > + * this store runs in EQ interrupt context, > + * concurrently with that process-context read. > + */ > + WRITE_ONCE(hwc->hwc_init_max_num_cqs, val); > break; [ ... ] > @@ -881,15 +891,29 @@ static int mana_hwc_establish_channel(struct > gdma_context *gc, u16 *q_depth, > *max_req_msg_size = hwc->hwc_init_max_req_msg_size; > *max_resp_msg_size = hwc->hwc_init_max_resp_msg_size; > > - /* Both were set in mana_hwc_init_event_handler(). */ > - if (WARN_ON(cq->id >= gc->max_num_cqs)) > + /* Snapshot the device-reported CQ count and CQ id into locals and > + * use only the locals below, so the same value that sizes cq_table > + * also bounds and indexes it -- even across the sleeping vcalloc(). > + * Both fields are written by mana_hwc_init_event_handler() from EQ > + * interrupt context: hwc_init_max_num_cqs under WRITE_ONCE() (paired > + * here), and cq->id as an ordinary store. READ_ONCE() keeps each > + * read tear-free and, crucially, non-reloadable, so a spoofed > + * post-init event cannot make the WARN_ON() pass against one value > + * while the allocation or the index uses another. > + */ > + num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs); > + cq_id = READ_ONCE(cq->id); > + > + if (WARN_ON(cq_id >= num_cqs)) > return -EPROTO; > > - cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table)); > + cq_table = vcalloc(num_cqs, sizeof(*cq_table)); > if (!cq_table) > return -ENOMEM; > > - rcu_assign_pointer(cq_table[cq->id], cq); > + gc->max_num_cqs = num_cqs; > + [Severity: High] This is a pre-existing issue, but does teardown leave gc->max_num_cqs at a stale value? Looking at mana_hwc_destroy_channel() in drivers/net/ethernet/microsoft/mana/hw_channel.c, it replaces the cq_table pointer but doesn't reset gc->max_num_cqs: old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true); synchronize_rcu(); During a device resume, mana_hwc_establish_channel() will publish a new, potentially smaller cq_table and update gc->max_num_cqs. If a malicious host sends a GDMA_EQE_COMPLETION event concurrently, the reader in mana_gd_process_eqe() in drivers/net/ethernet/microsoft/mana/gdma_main.c checks the bounds: cq_table = rcu_dereference(gc->cq_table); if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table)) break; Since there is no acquire barrier between reading gc->cq_table and gc->max_num_cqs, could weakly ordered CPUs read the stale, large max_num_cqs bound and the new, smaller cq_table pointer? This seems like it could allow a crafted cq_id to pass the bounds check and cause an out-of-bounds memory read. > + rcu_assign_pointer(cq_table[cq_id], cq); > /* Publish the fully-initialised table last; pairs with the > * rcu_dereference(gc->cq_table) in mana_gd_process_eqe(). > */ -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
