Re: [PATCH net 1/2] net: ntb_netdev: Avoid double-accounting netif_rx() drops

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

Pre-existing issues:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in NTB receive processing allows a malicious remote peer to cause a massive kernel heap buffer overflow.
--

--- Patch [1]: [PATCH net 1/2] net: ntb_netdev: Avoid double-accounting netif_rx() drops ---
commit 002045303db9fc237bf287acbb8f01a9873c837d
Author: Koichiro Den <[email protected]>

net: ntb_netdev: Avoid double-accounting netif_rx() drops

netif_rx() already accounts packets it drops in the core rx_dropped
counter. ntb_netdev counts them again as both errors and drops.
Leave netif_rx() drops to the core. Count the packet and bytes
unconditionally since it was received successfully by the driver.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 029a4a532a104..4e53b00f016b0 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
> @@ -155,13 +155,9 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  	skb->ip_summed = CHECKSUM_NONE;
>  	skb_record_rx_queue(skb, q->qid);
>  
> -	if (netif_rx(skb) == NET_RX_DROP) {
> -		ndev->stats.rx_errors++;
> -		ndev->stats.rx_dropped++;
> -	} else {
> -		ndev->stats.rx_packets++;
> -		ndev->stats.rx_bytes += len;
> -	}
> +	netif_rx(skb);
> +	ndev->stats.rx_packets++;
> +	ndev->stats.rx_bytes += len;

[Severity: Critical]
This is a pre-existing issue, but does this expose a time-of-check to
time-of-use issue during ntb receive processing?

Looking at ntb_process_rxc() in drivers/ntb/ntb_transport.c, hdr directly
accesses the shared memory window which the remote peer can write to via
pcie. A bounds check is performed, but then hdr->len is re-read directly
from the shared memory:

drivers/ntb/ntb_transport.c:ntb_process_rxc() {
	if (hdr->len > entry->len) {
		...
	} else {
		...
		entry->len = hdr->len;

		ntb_async_rx(entry, offset);
	}
}

Can the remote peer modify hdr->len to a massive value after the bounds
check passes but before the second read?

If so, this bypassed length is then passed to ntb_async_rx() and
subsequently to ntb_memcpy_rx():

drivers/ntb/ntb_transport.c:ntb_memcpy_rx() {
	void *buf = entry->buf;
	size_t len = entry->len;

	memcpy(buf, offset, len);
}

Could this cause memcpy() to overflow the skb data buffer? Should the
payload header values be copied to local variables using READ_ONCE() before
validation to prevent this?

>  
>  	skb = new_skb;
>

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