Re: [PATCH 11/11] NTB: ntb_transport: Drain RX tasklets during link cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In-flight RX DMA transfers are not drained during link cleanup before freeing Memory Window (MW) buffers, leading to DMA Use-After-Free, Read-After-Free, and potential information leaks.
--

commit 0ffb444fbfb867ed6e5c68ad186229e763417ba5
Author: Koichiro Den <den@valinux.co.jp>
NTB: ntb_transport: Drain RX tasklets during link cleanup

This commit modifies the queue pair link cleanup process to explicitly drain
and kill the receive tasklet. It temporarily disables the queue pair link
work to prevent concurrent reactivations while resetting the queue pair.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -969,11 +969,16 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  
>  	dev_info(&pdev->dev, "qp %d: Link Cleanup\n", qp->qp_num);
>  
> -	cancel_delayed_work_sync(&qp->link_work);
> +	disable_delayed_work_sync(&qp->link_work);
> +	ntb_transport_set_qp_active(qp, false);
> +	tasklet_kill(&qp->rxc_db_work);

[Severity: High]
This isn't a bug introduced by this patch, but while we are draining the
tasklet here, does this code still leave in-flight RX DMA transfers active
when the memory window is subsequently freed?

When a link down event occurs, ntb_transport_link_cleanup() calls
ntb_qp_link_cleanup(), which now successfully kills the rxc_db_work tasklet.
However, it does not appear to wait for or terminate pending DMA transfers on
qp->rx_dma_chan. After this function returns, ntb_transport_link_cleanup()
proceeds to call ntb_free_mw(), which returns the physical memory to the
system via dma_free_attrs().

If the hardware DMA engine is executing asynchronously, couldn't it
continue reading from or writing to this freed physical memory, potentially
causing a DMA use-after-free or data leak?

Furthermore, when the DMA asynchronous transfer completes, the
ntb_rx_copy_callback() function could be invoked:

ntb_rx_copy_callback() {
	...
	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
	...
}

Since ntb_free_mw() sets qp->rx_buff to NULL, could this result in a NULL
pointer dereference? Additionally, on success, ntb_complete_rxc() attempts
to write to entry->rx_hdr, which points into the now-freed memory window,
which could cause a software use-after-free.

Is it possible we also need to drain or explicitly terminate the DMA channel
transfers before resetting the link context and returning?

> +
>  	ntb_qp_link_down_reset(qp);
>  
>  	if (qp->event_handler)
>  		qp->event_handler(qp->cb_data, qp->link_is_up);
> +
> +	enable_delayed_work(&qp->link_work);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907142429.951930-1-den@valinux.co.jp?part=11