Re: [PATCH net-next v5 02/12] net: airoha: Reserve RX headroom to avoid skb reallocation

On May 10, sashiko-bot@kernel.org wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing bounds check adjustment for RX packet length allows buffer overflows
--

commit f34e8812cb62dd63f96801a39b0d65ee905cd258
Author: Lorenzo Bianconi [off-list ref]

net: airoha: Reserve RX headroom to avoid skb reallocation

This commit reserves NET_SKB_PAD + NET_IP_ALIGN bytes of headroom for
received packets to avoid skb head reallocation when pushing protocol
headers into the skb.

quoted

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c

[ ... ]

quoted

@@ -627,10 +629,12 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
 
 		port = eth->ports[p];
 		if (!q->skb) { /* first buffer */
-			q->skb = napi_build_skb(e->buf, q->buf_size);
+			q->skb = napi_build_skb(e->buf - AIROHA_RX_HEADROOM,
+						q->buf_size);
 			if (!q->skb)
 				goto free_frag;
 
+			skb_reserve(q->skb, AIROHA_RX_HEADROOM);
 			__skb_put(q->skb, len);

Does this code overlook adjusting the bounds check for the RX packet
length?

Earlier in airoha_qdma_rx_process(), the software validation check
uses the unadjusted lengths:

        data_len = q->skb ? q->buf_size
                          : SKB_WITH_OVERHEAD(q->buf_size);
        if (!len || data_len < len)
                goto free_frag;

If the hardware descriptor returns a len that is larger than the newly
adjusted dma_len but smaller than or equal to the old data_len, the
check will pass.

Since skb_reserve() shifts the skb->tail forward by AIROHA_RX_HEADROOM,
will calling __skb_put(q->skb, len) with this unvalidated oversized len
push skb->tail past skb->end and overflow the skb_shared_info structure?

Similarly for scattered fragments, could an oversized len passed to
skb_add_rx_frag() cause out-of-bounds memory accesses beyond the allocated
page fragment?

ack, I will fix it in v6.

Regards,
Lorenzo

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-eth-multi-serdes-v5-0-805e38edc2aa@kernel.org?part=2