Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] An early return on HWC teardown failure leaves a stale pointer in `gc->cq_table`, which a malicious host can exploit to cause an out-of-bounds read. - [Medium] Redundant HWC teardown in `mana_hwc_establish_channel()` causes a double hardware timeout (up to 120s stall) and masks the original initialization error code. Pre-existing issues: - [High] During HWC teardown, the CQ is destroyed and its memory freed before the EQ is deregistered, leading to Use-After-Free and WARN splats if an interrupt occurs. -- --- Patch [5]: [PATCH net v3 5/6] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering --- commit cb8bd3f89f96cf2e7fd5d41275858b076d281d38 Author: Long Li <[email protected]> net: mana: fix HWC teardown safety with setup_active flag and destroy ordering Three teardown hazards let the hardware touch memory the driver freed. [ ... ] > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -768,20 +769,33 @@ static int mana_hwc_establish_channel(struct > gdma_context *gc, u16 *q_depth, [ ... ] > +teardown: > + { > + int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false); > + > + if (!td_err) > + hwc->setup_active = false; > + > + return td_err ? td_err : err; > + } > } [Severity: Medium] Does this teardown logic mask the original error code and cause a redundant teardown attempt? If mana_smc_teardown_hwc() fails, it returns td_err and leaves hwc->setup_active = true. The caller mana_hwc_create_channel() handles the returned error like this: mana_hwc_create_channel() { ... err = mana_hwc_establish_channel(...); if (err) { dev_err(hwc->dev, "Failed to establish HWC: %d\n", err); goto out; } ... out: mana_hwc_destroy_channel(gc); return err; } Since hwc->setup_active is still true, mana_hwc_destroy_channel() will attempt to tear down the hardware a second time. This can cause the driver to stall for twice the duration of a hardware timeout, while the original initialization error code is lost. > @@ -907,11 +931,38 @@ void mana_hwc_destroy_channel(struct gdma_context *gc) [ ... ] > + if (hwc->setup_active) { > + int td_err = mana_smc_teardown_hwc(&gc->shm_channel, false); > + > + if (td_err) { > + dev_err(gc->dev, > + "HWC teardown failed: %d, leaking resources\n", > + td_err); > + return; > + } [Severity: High] Could this early return leave a stale pointer in gc->cq_table? If mana_smc_teardown_hwc() fails, this returns early without clearing gc->cq_table. On resume or re-initialization, mana_hwc_create_channel() creates a new hwc but reuses the same gc. During mana_hwc_establish_channel(), while waiting for the initialization event, the hardware could send an EQE updating gc->max_num_cqs to a large value. Because the new cq_table is not assigned until after the wait completes, gc->cq_table still points to the old, leaked table. A malicious host could then inject a completion EQE with an out-of-bounds cq_id. This would pass validation against the newly inflated gc->max_num_cqs but read out of bounds from the smaller, old cq_table. > + > + hwc->setup_active = false; > + } > + > + /* Tear down the HWC CQ object first — mana_hwc_destroy_cq() > + * both unpublishes the CQ from cq_table (+synchronize_rcu) and > + * deregisters the HWC EQ from the interrupt handler list (via > + * mana_gd_deregister_irq + synchronize_rcu), guaranteeing no > + * interrupt handler can access RQ/TXQ buffers after this point. > + */ > + if (hwc->cq) > + mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq); [Severity: High] This is a pre-existing issue, but is there a use-after-free risk during CQ destruction? Looking at mana_hwc_destroy_cq(), the CQ is destroyed and its memory freed before the EQ: mana_hwc_destroy_cq() { ... if (hwc_cq->gdma_cq) mana_gd_destroy_queue(gc, hwc_cq->gdma_cq); ... if (hwc_cq->gdma_eq) mana_gd_destroy_queue(gc, hwc_cq->gdma_eq); } Although mana_gd_destroy_queue() on the CQ clears the cq_table entry and calls synchronize_rcu(), the EQ is still fully registered and on the interrupt dispatch list. A pending interrupt could fire immediately after the CQ is freed. The interrupt handler (mana_gd_process_eqe) might then trigger a WARN_ON_ONCE(!cq) for completions, or call mana_hwc_init_event_handler() which accesses the freed memory: mana_hwc_init_event_handler() { ... case HWC_INIT_DATA_CQID: hwc->cq->gdma_cq->id = val; break; ... } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
