Re: [PATCH net v7 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:
- [High] Incomplete Fix / Use-After-Free of WQ structures due to incorrect channel teardown order.
- [High] Stack Memory Corruption and Logic Error due to missing reinitialization after request timeout.
- [High] Hardware DMA Use-After-Free when channel establishment times out.
--

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

net: mana: free HWC comp_buf after destroying the EQ

mana_hwc_destroy_cq() freed comp_buf and the CQ before the EQ. While the
EQ was still registered its handler could reach comp_buf and the CQ, so
a late EQE could touch freed memory. This commit fixes the teardown order.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index ccef9bf9c6bfd..7e01596df11b6 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -384,14 +384,17 @@ 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 first: it deregisters the IRQ and drains in-flight
> +	 * handlers, so none can touch the CQ after it is freed.
> +	 */
> +	if (hwc_cq->gdma_eq)
> +		mana_gd_destroy_queue(gc, hwc_cq->gdma_eq);
>  
> +	/* Safe to free now that the EQ handler is fenced. */
>  	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);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown order in
mana_hwc_destroy_channel() leave a use-after-free for the WQ structures?

In mana_hwc_destroy_channel(), the txq and rxq wrappers are destroyed
before mana_hwc_destroy_cq() fences the EQ:

mana_hwc_destroy_channel() {
    ...
    if (hwc->txq)
        mana_hwc_destroy_wq(hwc, hwc->txq);
        
    if (hwc->rxq)
        mana_hwc_destroy_wq(hwc, hwc->rxq);
        
    if (hwc->cq)
        mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
    ...
}

Since mana_hwc_destroy_wq() frees the wq structure, can an unfenced EQ 
handler dereference the freed hwc->rxq pointer in 
mana_hwc_rx_event_handler() while the EQ is still active?

[Severity: High]
This is a pre-existing issue, but does mana_hwc_send_request() have a stack
corruption bug if a request times out?

If wait_for_completion_timeout() times out, the function jumps to out and 
releases the msg_id:

mana_hwc_send_request() {
    ...
    if (!wait_for_completion_timeout(&ctx->comp_event, ...)) {
        err = -ETIMEDOUT;
        goto out;
    }
    ...
}

A new request could then allocate the same msg_id without reinitializing
ctx->comp_event. If a late response arrives for the original request, 
mana_hwc_handle_resp() will copy the stale payload into the new caller's 
stack buffer and complete the event:

mana_hwc_handle_resp() {
    ...
    memcpy(ctx->output_buf, resp_msg, resp_len);
out:
    ctx->error = err;
    ...
    complete(&ctx->comp_event);
}

Could this cause the new request to receive stale data and continue 
prematurely?

[Severity: High]
This is a pre-existing issue, but does a timeout during channel 
establishment result in a DMA use-after-free?

In mana_hwc_establish_channel(), if the hardware takes too long to reply:

mana_hwc_establish_channel() {
    ...
    if (!wait_for_completion_timeout(&hwc->hwc_init_eqe_comp, 60 * HZ))
        return -ETIMEDOUT;
    ...
}

The error path calls mana_hwc_destroy_channel(), which checks max_num_cqs
before tearing down the hardware channel:

mana_hwc_destroy_channel() {
    ...
    if (gc->max_num_cqs > 0)
        mana_smc_teardown_hwc(&gc->shm_channel, false);
    ...
}

Since max_num_cqs remains 0 during an init timeout, mana_smc_teardown_hwc() 
is skipped. Are the DMA buffers then freed while the hardware channel is 
still active and capable of writing to them?

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