Re: [PATCH net-next v2 4/8] net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: UAF and double-free in axienet exposed by skb_to_sgvec error return
- [Low] mt76: memory leak of skb on skb_to_sgvec() error in mt76u_tx_queue_skb()
--

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

net: skbuff: don't BUG() on leftover length in skb_checksum() and friends

This patch replaces system-halting BUG_ON calls with error returns in
skb_checksum, skb_crc32c, and __skb_to_sgvec when encountering leftover
length. It returns -EINVAL for __skb_to_sgvec and 0 for checksum functions,
allowing the system to recover gracefully.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
[ ... ]
> @@ -5339,7 +5345,10 @@ __skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len,
>  		}
>  		start = end;
>  	}
> -	BUG_ON(len);
> +	if (unlikely(len)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return -EINVAL;
> +	}

[Severity: High]
By replacing the BUG_ON with an error return, callers are now expected to
cleanly handle the failure, but does this expose an existing memory
corruption path in the axienet driver?

Looking at axienet_start_xmit_dmaengine in
drivers/net/ethernet/xilinx/xilinx_axienet_main.c, the TX ring head is
incremented before calling skb_to_sgvec:

    lp->tx_ring_head++;
    sg_init_table(skbuf_dma->sgl, sg_len);
    ret = skb_to_sgvec(skb, skbuf_dma->sgl, 0, skb->len);
    if (ret < 0)
        goto xmit_error_drop_skb;

On error, it jumps to xmit_error_drop_skb, frees the SKB, but fails to
decrement lp->tx_ring_head or submit the descriptor to the DMA engine:

xmit_error_drop_skb:
    dev_kfree_skb_any(skb);
    return NETDEV_TX_OK;

When subsequent successful transmissions complete, the DMA callback
axienet_dma_tx_cb monotonically increments lp->tx_ring_tail for each
completion. Will it process the abandoned ring slot and double-free the
stale skb pointer?

axienet_dma_tx_cb() {
    ...
    skbuf_dma = axienet_get_tx_desc(lp, lp->tx_ring_tail++);
    ...
    dev_consume_skb_any(skbuf_dma->skb);
}

[Severity: Low]
Does this error return also cause a memory leak in the mt76 driver?

In mt76u_tx_setup_buffers from drivers/net/wireless/mediatek/mt76/usb.c,
the negative error bypasses the zero check:

    urb->num_sgs = skb_to_sgvec(skb, urb->sg, 0, skb->len);
    if (!urb->num_sgs)
        return -ENOMEM;

    return urb->num_sgs;

In mt76u_tx_queue_skb, this negative error is caught and returned upstream:

    err = mt76u_tx_setup_buffers(dev, tx_info.skb, q->entry[idx].urb);
    if (err < 0)
        return err;

However, the caller mt76_tx_process_mcu_queue unlinks the skb before
attempting to queue it:

    __skb_unlink(skb, head);
    spin_unlock(&head->lock);

    sta = wcid_to_sta(wcid);
    spin_lock(&q->lock);
    __mt76_tx_queue_skb(phy, qid, skb, wcid, sta, NULL);

Since mt76u_tx_queue_skb aborts without calling dev_kfree_skb or
ieee80211_tx_status, will the unlinked skb be permanently leaked?

> 	return elt;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-b4-skbuff-bug-on-v2-0-b9a5f732895b@toxicpanda.com?part=4