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

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

Pre-existing issues:
- [High] Infinite error loop on packet version mismatch in ntb_process_rxc.
- [High] Missing READ_ONCE() for `hdr->len` can lead to a double-fetch vulnerability, allowing a malicious peer to bypass length checks.
--

--- Patch [2]: [PATCH net-next v2 2/4] NTB: ntb_transport: Use little-endian shared fields ---
commit 1e93f678e1d03757c62cfd1f9dcbe502878847d8
Author: Koichiro Den <[email protected]>

NTB: ntb_transport: Use little-endian shared fields

This commit marks the shared payload headers and RX ring tail as __le32 and
converts the accesses to coherent memory using cpu_to_le32() and le32_to_cpu()
respectively. This fixes byte-swapping issues when the shared fields are read
on big-endian systems.

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index d458a8b1de114..967a5ad381646 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -1609,13 +1610,15 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  {
>  	struct ntb_payload_header *hdr;
>  	struct ntb_queue_entry *entry;
> -	unsigned int flags;
>  	void *offset;
> +	u32 flags;
> +	u32 len;
> +	u32 ver;
>  
>  	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
>  	hdr = offset + qp->rx_max_frame - sizeof(struct ntb_payload_header);
>  
> -	flags = READ_ONCE(hdr->flags);
> +	flags = le32_to_cpu(READ_ONCE(hdr->flags));
>  	if (!(flags & DESC_DONE_FLAG)) {
>  		dev_dbg(&qp->ndev->pdev->dev, "done flag not set\n");
>  		qp->rx_ring_empty++;
> @@ -1623,21 +1626,23 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  	}
>  
>  	dma_rmb();
> +	ver = le32_to_cpu(hdr->ver);
> +	len = le32_to_cpu(hdr->len);

[Severity: High]
This is a pre-existing issue, but missing READ_ONCE() for hdr->len could
lead to a double-fetch vulnerability.

Since hdr->len is located in DMA-coherent memory shared with the peer,
reading it without READ_ONCE() allows the compiler to rematerialize the read
later in the function (e.g., when setting entry->len = len). 

Could a malicious peer exploit this TOCTOU race by increasing hdr->len after
the bounds check passes, causing an out-of-bounds copy into the host's RX
buffer? Should we use READ_ONCE() for hdr->ver and hdr->len as we do for
flags?

>  
>  	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 does returning here cause an infinite
error loop?

If a remote peer sends a packet with a wrong sequence number (so
ver != qp->rx_pkts), ntb_process_rxc() returns -EIO without clearing
DESC_DONE_FLAG or advancing qp->rx_index.

Would this cause the host to repeatedly process the same stalled descriptor
on every subsequent tasklet execution, permanently stalling the RX queue?

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