Re: [PATCH net v4 3/7] net: mana: free HWC comp_buf after destroying the EQ

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] DMA-after-free and memory corruption due to skipped hardware teardown on timeout.
- [High] Use-After-Free of `hwc->txq` and `hwc->rxq` in Event Queue handlers.
--

--- Patch [3]: [PATCH net v4 3/7] net: mana: free HWC comp_buf after destroying the EQ ---
commit 3b2de5d23ee69b3df0c657484fe10e47ed38e264
Author: Long Li <[email protected]>

net: mana: free HWC comp_buf after destroying the EQ

mana_hwc_destroy_cq() freed hwc_cq->comp_buf and destroyed the CQ before
the EQ. That was unsafe while the EQ was still registered because a late
EQE dispatched after the free could touch freed memory. Destroy the EQ first,
which deregisters its IRQ and waits out in-flight handlers, fencing all
EQE dispatch.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3f011ebbe7b3e..19896bb5ce1a4 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,24 @@ static void mana_hwc_comp_event(void *ctx, struct gdma_queue *q_self)
>  
>  static void mana_hwc_destroy_cq(struct gdma_context *gc, struct hwc_cq *hwc_cq)
>  {
> -	kfree(hwc_cq->comp_buf);
> +	/* Destroy the EQ before the CQ.  mana_gd_destroy_queue() on the EQ
> +	 * deregisters its IRQ and waits out in-flight handlers, fencing all
> +	 * EQE dispatch — both the completion path and HWC init/reconfig
> +	 * events.  Freeing the CQ first would leave the EQ live and able to
> +	 * dispatch an event that dereferences hwc->cq->gdma_cq (e.g.
> +	 * mana_hwc_init_event_handler()) after it has been freed.
> +	 */
> +	if (hwc_cq->gdma_eq)
> +		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);

[Severity: High]
This isn't a bug introduced by this patch, but I see a similar vulnerability
unpatched in mana_hwc_destroy_channel() in the same file.

While this patch ensures the EQ is destroyed before the CQ,
mana_hwc_destroy_channel() calls mana_hwc_destroy_wq() for the TX and
RX queues before calling mana_hwc_destroy_cq().

If an interrupt fires between the WQ frees and the EQ fence inside
mana_hwc_destroy_cq(), the EQ handler can process completion or init events
that dereference the freed WQs:

mana_hwc_tx_event_handler() {
    ...
    struct hwc_wq *hwc_txq = hwc->txq;

    WARN_ON_ONCE(!hwc_txq || hwc_txq->gdma_wq->id != gdma_txq_id);
    ...
}

Could this order of operations leave a window where active EQ interrupts
can access freed WQ memory?


[Severity: Critical]
This is another pre-existing issue, but it looks like there might be a flaw
in the teardown logic when a timeout occurs.

In mana_hwc_establish_channel(), mana_smc_setup_hwc() provides the DMA
addresses to the hardware. If the wait for the init event times out, the
driver calls mana_hwc_destroy_channel().

However, mana_hwc_destroy_channel() will skip the hardware teardown if
the initialization didn't complete:

mana_hwc_destroy_channel() {
    ...
    /* gc->max_num_cqs is set in mana_hwc_init_event_handler(). If it's
     * non-zero, the HWC worked and we should tear down the HWC here.
     */
    if (gc->max_num_cqs > 0)
        mana_smc_teardown_hwc(&gc->shm_channel, false);
    ...
}

Because max_num_cqs is 0, it skips mana_smc_teardown_hwc(), but then
proceeds to free all queue DMA buffers. Since the hardware channel was
established, can it write to the freed DMA addresses if it processes
the delayed events later, corrupting whatever reused those pages?

>  
> +	/* comp_buf is reached only by mana_hwc_comp_event(), invoked from
> +	 * the now-fenced EQ handler, so it is safe to free once the EQ and
> +	 * CQ are gone.
> +	 */
>  	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);
> -
> +	kfree(hwc_cq->comp_buf);
>  	kfree(hwc_cq);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3