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

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

Pre-existing issues:
- [Critical] A TOCTOU race condition between the HWC request timeout path and the response interrupt handler allows the handler to perform a `memcpy` into a deallocated stack frame.
- [High] Pre-existing use-after-free risk in `mana_hwc_destroy_channel` due to WQs (`txq` and `rxq`) being destroyed before the EQ is safely fenced.
- [High] If HWC channel establishment times out, hardware teardown is skipped, allowing the hardware to DMA into freed queue memory.
--

--- Patch [3]: [PATCH net v5 3/7] net: mana: free HWC comp_buf after destroying the EQ ---
commit 20a39692a14893099232cb3a9085ec258236f7dc
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: the EQ
interrupt handler reaches comp_buf via mana_hwc_comp_event() and the CQ
object (hwc->cq->gdma_cq) via mana_hwc_init_event_handler(), so a late
EQE dispatched after the free could touch freed memory.

> 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 while checking the teardown
ordering, could there be a similar use-after-free risk for the WQs?

In mana_hwc_destroy_channel(), the WQs are destroyed before the EQ is
safely fenced by mana_hwc_destroy_cq():

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
    ...
    mana_hwc_destroy_wq(hwc, hwc->rxq);
    mana_hwc_destroy_wq(hwc, hwc->txq);
    mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
    ...
}

If an in-flight EQ interrupt handler runs during this window, it will invoke
mana_hwc_comp_event(), which calls mana_hwc_rx_event_handler(). The handler
blindly dereferences the freed hwc->rxq:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_rx_event_handler() {
    ...
    if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
    ...
}

Will this cause a use-after-free if an interrupt arrives after the WQs are
freed but before mana_hwc_destroy_cq() fences the EQ?

[Severity: High]
This is a pre-existing issue, but does skipping hardware teardown on a timeout
lead to DMA into freed memory?

In mana_hwc_establish_channel(), if hardware initialization times out:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_establish_channel() {
    ...
    if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
        return -ETIMEDOUT;
    ...
}

The hardware channel remains active while gc->max_num_cqs remains 0. When the
error path calls mana_hwc_destroy_channel(), the teardown is skipped:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
    ...
    if (gc->max_num_cqs > 0)
        mana_smc_teardown_hwc(&gc->shm_channel, false);
    ...
}

Since the hardware is still active but the queue memory is subsequently freed,
can the device DMA into the freed queue memory?

[Severity: Critical]
This is a pre-existing issue, but could a race condition cause stack corruption
if a request times out?

In mana_hwc_send_request(), if wait_for_completion_timeout() times out, the
function clears the msg_id and returns, destroying the stack frame containing
ctx->output_buf:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() {
    ...
    if (!wait_for_completion_timeout(&ctx->comp_event, 40 * HZ)) {
        ...
        mana_hwc_put_msg_index(hwc, msg_id);
        return err;
    }
    ...
}

Concurrently, the interrupt handler does a lockless check of the inflight map:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_handle_resp() {
    ...
    if (!test_bit(msg_id, hwc->inflight_msg_res.map)) {
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
    ...
}

If the handler observes the bit as set just before the timeout clears it, it
will execute the memcpy(). Since the request function has already returned,
doesn't this write arbitrary device-controlled data into a deallocated stack
frame?

> +
> +	/* 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