Re: [PATCH net-next v2 04/10] net: niu: check for failure when pulling in the RX header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: niu: BUG() in eth_type_trans due to insufficient pskb_may_pull length
--

commit 19ab5666684c0028709e6a0efad246e9460b1000
Author: Josef Bacik <josef@toxicpanda.com>

net: niu: check for failure when pulling in the RX header

This commit replaces __pskb_pull_tail() with pskb_may_pull() in
niu_process_rx_pkt() and adds error handling. If pulling the header fails,
the packet is dropped instead of crashing.

> diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
> index c74a97fe54641..d1c0e868004d1 100644
> --- a/drivers/net/ethernet/sun/niu.c
> +++ b/drivers/net/ethernet/sun/niu.c
> @@ -3488,7 +3488,11 @@ static int niu_process_rx_pkt(struct napi_struct *napi, struct niu *np,
>  
>  	len += sizeof(*rh);
>  	len = min_t(int, len, sizeof(*rh) + VLAN_ETH_HLEN);
> -	__pskb_pull_tail(skb, len);
> +	if (unlikely(!pskb_may_pull(skb, len))) {

[Severity: High]
Is it possible for a malformed hardware descriptor to provide a small
RCR_ENTRY_L2_LEN, resulting in a BUG_ON assertion in __skb_pull() later
when eth_type_trans() is called?

If the hardware reports an unusually small RCR_ENTRY_L2_LEN (e.g. 4 bytes),
then len evaluates to 12. pskb_may_pull(skb, 12) would only ensure 12 bytes
in the linear area.

Later in this function, skb_pull(skb, sizeof(*rh)) consumes the 8-byte
hardware header, leaving 4 linear bytes. The packet is then passed to
eth_type_trans(), which attempts to pull ETH_HLEN (14) bytes:

drivers/net/ethernet/sun/niu.c:niu_process_rx_pkt() {
    ...
    skb_pull(skb, sizeof(*rh));
    ...
    skb->protocol = eth_type_trans(skb, np->dev);
    ...
}

If the total skb->len is greater than 14 due to fragments, __skb_pull()
will execute and subtract 14 from skb->len. This could drop skb->len below
skb->data_len and trigger a BUG_ON(skb->len < skb->data_len) in __skb_pull().

Should pskb_may_pull() ensure at least sizeof(*rh) + ETH_HLEN bytes to
protect the downstream eth_type_trans() call?

> +		rp->rx_dropped++;
> +		kfree_skb(skb);
> +		return num_rcr;
> +	}
>  
>  	rh = (struct rx_pkt_hdr1 *) skb->data;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-b4-pskb-pull-tail-drivers-v2-0-8f2bd9bee138@toxicpanda.com?part=4