Re: [PATCH net-next v3 2/4] NTB: ntb_transport: Use little-endian shared fields

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Permanent RX queue stall on version mismatch error.
- [High] Missing bounds check against `rx_max_frame` allows an attacker-controlled peer to trigger an out-of-bounds read of kernel memory.
- [Medium] The queue size calculations do not guarantee that `rx_size` and `tx_size` are appropriately aligned, leading to unaligned MMIO writes and coherent memory accesses.
--

commit 8632f1194f420fac4999a48cdd0e2e9e59e54697
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Use little-endian shared fields

ntb_transport writes payload headers and the RX ring tail with
iowrite32(), but reads peer-written copies from coherent memory as native
integers. The values are therefore byte-swapped when read on a big-endian
system.

Mark the shared fields as __le32 and convert coherent-memory accesses
accordingly. Read hdr->ver and hdr->len once so their checks and later
uses see the same values.

Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/r/20260818064951.7EA231F000E9@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 74f4f8c1c7be8..3f497a62673fd 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -633,7 +634,7 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
>  		qp->rx_alloc_entry++;
>  	}
>  
> -	qp->remote_rx_info->entry = qp->rx_max_entry - 1;
> +	qp->remote_rx_info->entry = cpu_to_le32(qp->rx_max_entry - 1);

[Severity: Medium]
This isn't a bug introduced by this patch, but the local receive buffer slice
size rx_size is calculated earlier in ntb_transport_setup_qp_mw() as:
 
	rx_size = (unsigned int)mw->xlat_size / num_qps_mw;
 
If num_qps_mw does not cleanly divide the window size, the resulting rx_size
can be unaligned.
 
Does this misalign the qp->remote_rx_info pointer which resides at the end
of this buffer, as well as the frame boundaries qp->rx_max_frame? If so, on
strict architectures, could the unaligned READ_ONCE() and iowrite32()
accesses to the payload headers and shared fields trigger an alignment fault?

>  
>  	/* setup the hdr offsets with 0's */
>  	for (i = 0; i < qp->rx_max_entry; i++) {

[ ... ]

> @@ -1623,21 +1626,23 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  	}
>  
>  	dma_rmb();
> +	ver = le32_to_cpu(READ_ONCE(hdr->ver));
> +	len = le32_to_cpu(READ_ONCE(hdr->len));
>  
>  	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
> -		qp->qp_num, hdr->ver, hdr->len, flags);
> +		qp->qp_num, ver, len, flags);
>  
>  	if (flags & LINK_DOWN_FLAG) {
>  		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");
>  		ntb_qp_link_down(qp);
> -		hdr->flags = 0;
> +		hdr->flags = cpu_to_le32(0);
>  		return -EAGAIN;
>  	}
>  
> -	if (hdr->ver != (u32)qp->rx_pkts) {
> +	if (ver != (u32)qp->rx_pkts) {
>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"version mismatch, expected %llu - got %u\n",
> -			qp->rx_pkts, hdr->ver);
> +			qp->rx_pkts, ver);
>  		qp->rx_err_ver++;
>  		return -EIO;
>  	}

[Severity: High]
This is a pre-existing issue, but if the received payload header version does
not match the expected qp->rx_pkts in ntb_process_rxc(), the function
increments the error counter and returns -EIO without clearing the
DESC_DONE_FLAG or advancing qp->rx_index.

Since the caller aborts processing upon the error, the next time the queue is
processed, it will attempt to process the same packet at the same index, fail
the version check again, and return -EIO.

Can this lead to a permanent stall of the RX queue until the link is
physically reset?

> @@ -1652,10 +1657,10 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  	entry->rx_hdr = hdr;
>  	entry->rx_index = qp->rx_index;
>  
> -	if (hdr->len > entry->len) {
> +	if (len > entry->len) {

[Severity: High]
This isn't a bug introduced by this patch, but this bounds check in
ntb_process_rxc() only validates that the received length len fits within the
client-provided buffer (entry->len).

Because entry->len is derived from the local memory window size
(tx_max_frame), it can be larger than the receive frame size (rx_max_frame)
negotiated by the peer.

If a malicious peer provides a large length that is greater than the
negotiated rx_max_frame but still less than entry->len, could this bypass the
check and cause ntb_memcpy_rx() or ntb_async_rx() below to copy memory
out-of-bounds from the qp->rx_buff DMA region, leaking adjacent kernel memory
to the network stack?

>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"receive buffer overflow! Wanted %d got %d\n",
> -			hdr->len, entry->len);
> +			len, entry->len);
>  		qp->rx_err_oflow++;
>  
>  		entry->len = -EIO;

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