Re: [PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Skipping ntb_qp_link_cleanup() during transport teardown fails to drain rx_post_q, causing freed memory to be accessed by in-flight IOAT DMA.

Pre-existing issues:
- [Critical] Double-fetch of `hdr->len` from shared memory allows buffer overflow via TOCTOU race.
- [High] Racy read-modify-write of `qp_bitmap_free` bitmask allows queue state corruption.
--

commit 68ca6cad9a88600dd4607f3c4ccd6bd32385b0e7
Author: Koichiro Den <den@valinux.co.jp>
Subject: NTB: ntb_transport: Remove clients before freeing transport resources

Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
is still bound. This patch reorders the teardown process to unregister
clients before freeing resources, intending to fix unsafe memory accesses
during intensive TX loads.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index c1e3a077c45ec..f24555b38d1b5 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>  
>  	debugfs_remove_recursive(nt->debugfs_node_dir);
>  
> -	ntb_transport_link_cleanup(nt);
> -	cancel_work_sync(&nt->link_cleanup);
> -	cancel_delayed_work_sync(&nt->link_work);
> +	/* Stop transport work before clients release their QPs. */
> +	disable_delayed_work_sync(&nt->link_work);
> +	disable_work_sync(&nt->link_cleanup);
> +
> +	ntb_bus_remove(nt);

[Severity: Critical]
By moving ntb_bus_remove() before ntb_transport_link_cleanup(), does this
introduce a DMA use-after-free regression?

Calling ntb_bus_remove() first prompts client drivers to free their QPs via
ntb_transport_free_queue(), which blindly frees all entries in rx_post_q
without polling:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
	while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
		dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
		kfree(entry);
	}
    ...
}

Because IOAT DMA engines could still be actively copying into those buffers,
freeing them allows the hardware DMA engine to write to freed kernel memory.
Furthermore, the DMA drain wait in ntb_qp_link_cleanup() is subsequently
skipped entirely because qp_bitmap_alloc evaluates to 0 during the later
call to ntb_transport_link_cleanup().

[ ... ]

> @@ -1736,17 +1736,17 @@ static void ntb_process_rxc(struct ntb_transport_qp *qp)
>  
>  	if (hdr->len > entry->len) {
>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"receive buffer overflow! Wanted %d got %d\n",
>  			hdr->len, entry->len);
>  		qp->rx_err_oflow++;
>  
>  		entry->len = -EIO;
>  		entry->flags |= DESC_DONE_FLAG;
>  
>  		ntb_complete_rxc(qp);
>  	} else {
>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"RX OK index %u ver %u size %d into buf size %d\n",
>  			qp->rx_index, hdr->ver, hdr->len, entry->len);
>  
>  		qp->rx_bytes += hdr->len;
>  		qp->rx_pkts++;
>  
>  		entry->len = hdr->len;

[Severity: Critical]
This is a pre-existing issue, but is there a time-of-check to time-of-use
race condition in this bounds check on hdr->len?

Since hdr points to a payload header located inside a memory window writable
by the remote NTB peer (shared memory), hdr->len is read directly from
shared memory twice (once for the bounds check, once for the assignment).

Can a remote peer swap hdr->len to a much larger value immediately after
the bounds check but before the assignment, bypassing the bounds check and
causing a buffer overflow when copied?

[ ... ]

> @@ -2130,7 +2130,7 @@ struct ntb_transport_qp *
>  ntb_transport_create_queue(void *data, struct device *client_dev,
>  			   const struct ntb_queue_handlers *handlers)
>  {
[ ... ]
>  	free_queue--;
>  
>  	qp = &nt->qp_vec[free_queue];
>  	qp_bit = BIT_ULL(qp->qp_num);
>  
>  	nt->qp_bitmap_free &= ~qp_bit;

[Severity: High]
This isn't a bug introduced by this patch, but could this read-modify-write
of nt->qp_bitmap_free lead to queue state corruption?

The qp_bitmap_free bitmask is modified here using non-atomic bitwise
operations without holding any lock. It is also modified concurrently during
driver unbind:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
	qp->transport->qp_bitmap_free |= qp_bit;
    ...
}

Since these are exported functions called by NTB client drivers, could
concurrent probe and remove operations race and lose bit updates, leading
to a leaked queue or two clients being assigned the same queue?

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