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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

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 <lorenzo@kernel.org>
> 
> 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.
> 
> > 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
> [ ... ]
> > @@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]