Re: [PATCH net v4 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated

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 [off-list ref]

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.

[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;

[ ... ]

[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/20260803234355.636038-1-longli@microsoft.com?part=7