Re: [PATCH v2 12/14] NTB: ntb_transport: Clear QP pointers when freeing an MW

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch addresses a serial Use-After-Free but leaves a concurrent TOCTOU (Time-Of-Check to Time-Of-Use) Use-After-Free unmitigated, as `WRITE_ONCE()` does not protect concurrent readers from accessing freed memory.
--

commit 83701cf77653ce575ecc537dc3582d73e7b2ebcf
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Clear QP pointers when freeing an MW

This commit clears the rx_buff and remote_rx_info pointers for all QPs using
an MW when the MW is freed. This addresses a serial use-after-free where a
link-down event could cause ntb_qp_link_down_reset() to write to previously
freed MW memory.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index b949f36a4f2d9..096be87e5ede9 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -781,10 +781,17 @@ static void ntb_free_mw(struct ntb_transport_ctx *nt, int num_mw)
>  {
>  	struct ntb_transport_mw *mw = &nt->mw_vec[num_mw];
>  	struct device *dma_dev = ntb_get_dma_dev(nt->ndev);
> +	unsigned int i;
>  
>  	if (!mw->virt_addr)
>  		return;
>  
> +	/* Drop references from every QP using this MW. */
> +	for (i = num_mw; i < nt->qp_count; i += nt->mw_count) {
> +		nt->qp_vec[i].rx_buff = NULL;
> +		WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this clearing mechanism leave a
concurrent Time-Of-Check to Time-Of-Use (TOCTOU) regression unmitigated?

While this patch addresses a serial use-after-free, WRITE_ONCE() does not
protect concurrent readers from accessing freed memory. If a client drops the
link concurrently with a hardware link-down event triggering
ntb_transport_link_cleanup(), the client thread can fetch the remote_rx_info
pointer locklessly:

drivers/ntb/ntb_transport.c:ntb_qp_link_down_reset() {
	...
	remote_rx_info = READ_ONCE(qp->remote_rx_info);
	if (remote_rx_info)
		remote_rx_info->entry = qp->rx_max_entry - 1;
	...
}

If the client thread is preempted after reading the pointer, and the cleanup
thread proceeds to free the memory in ntb_free_mw():

drivers/ntb/ntb_transport.c:ntb_free_mw() {
	...
	ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
	dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
		       mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);
	...
}

When the client thread resumes, wouldn't it write to the freed DMA memory?

This same lockless TOCTOU vulnerability appears to be reachable on the
transmit hot-path in ntb_transport_tx_free_entry() and via debugfs in
ntb_qp_debugfs_stats_show().

>  	ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
>  	dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
>  		       mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);

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