Re: [PATCH] net: stmmac: fix integer underflow in chain mode jumbo_frm

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Mar 24, 2026 at 01:07:47AM -0500, Tyllis Xu wrote:
> I will try to change the code to consolidate with the ring-mode
> code, avoid whitespace diff, and resubmit the patch with a new
> correct subject line. Thank you for your feedback!

I mentioned the possibilities of other problems. How about this:

static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
{
        unsigned int nopaged_len = skb_headlen(skb);
...
        enh_desc = priv->plat->enh_desc;
        /* To program the descriptors according to the size of the frame */
        if (enh_desc)
                is_jumbo = stmmac_is_jumbo_frm(priv, skb->len, enh_desc);

1. enh_desc is set if priv->dma_cap.enh_desc is set and DMA capabilities
   exist (dmwac1000 and more recent.)

2. in chain mode, is_jumbo will be true if skb->len > 8KiB.
   in ring mode, is_jumbo will be true if skb->len >= 4KiB
   (note the >= vs > there which is suspicious.)

Let's consider the case where is_jumbo is false. So this could be a
packet with skb->len up to 4KiB-1 or 8KiB.

        if (unlikely(is_jumbo)) {
        } else {
                dma_addr = dma_map_single(priv->device, skb->data,
                                          nopaged_len, DMA_TO_DEVICE);

                stmmac_set_desc_addr(priv, first_desc, dma_addr);

                /* Prepare the first descriptor without setting the OWN bit */
                stmmac_prepare_tx_desc(priv, first_desc, 1, nopaged_len,
                                       csum_insertion, priv->descriptor_mode,
                                       0, last_segment, skb->len);

This can call one of several functions.

ndesc_prepare_tx_desc():
        if (descriptor_mode == STMMAC_CHAIN_MODE)
                norm_set_tx_desc_len_on_chain(p, len);
        else
                norm_set_tx_desc_len_on_ring(p, len);

static inline void norm_set_tx_desc_len_on_chain(struct dma_desc *p, int len)
{
        p->des1 |= cpu_to_le32(len & TDES1_BUFFER1_SIZE_MASK);
}

#define TDES1_BUFFER1_SIZE_MASK         GENMASK(10, 0)

So, this masks the length with 0x7ff, meaning this can represent
buffers up to 2047 bytes _max_ for normal descriptors in chain
mode.

static inline void norm_set_tx_desc_len_on_ring(struct dma_desc *p, int len)
{
        unsigned int buffer1_max_length = BUF_SIZE_2KiB - 1;

        if (unlikely(len > buffer1_max_length)) {
                p->des1 |= cpu_to_le32(FIELD_PREP(TDES1_BUFFER2_SIZE_MASK,
                                                  len - buffer1_max_length) |
                                       FIELD_PREP(TDES1_BUFFER1_SIZE_MASK,
                                                  buffer1_max_length));
        } else {
                p->des1 |= cpu_to_le32(FIELD_PREP(TDES1_BUFFER1_SIZE_MASK,
                                                  len));
        }
}

#define TDES1_BUFFER2_SIZE_MASK         GENMASK(21, 11)

This works around the 2KiB limitation, and fills buffer 1 with
2047 bytes and buffer 2 with the remainder... but there is _no_ code
that writes buffer 2's address in this case. This means we will
transmit garbage - and unknowingly of transmit COE is enabled (because
the checksums will be based on the data the NIC read from memory.)

However, normal descriptors in ring mode can correctly transmit up
to 2047 bytes correctly, or garbage after that up to 4094 bytes.

Now, this in the probe function:

                ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);

equates to:

   PAGE_SIZE - NET_SKB_PAD - NET_IP_ALIGN - aligned skb_shared_info.

Notice that this doesn't limit by what the hardware can actually do.

Consider the implications where PAGE_SIZE is 16KiB or 64KiB and the
stmmac hardware only supports normal descriptors. max_mtu gets set
to something close to PAGE_SIZE. Even for 4KiB, it'll be close to that.
If skb_headlen(skb) can approach max_mtu, then we have the very real
possibility of overflowing the first descriptor - which in normal
mode can only really handle up to 2047 bytes.

This has been on my list of issues to try and sort out at some point,
at the moment I don't have any patches, but I'm instead trying to
clean up the code to make it more understandable so I can start
thinking about possible solutions. The code here has been a total
trainwreck, and some of those cleanups have recently gone in to
net-next.

I think the driver has only really been tested with the standard
1500 byte MTU, and maybe briefly with the 9KiB jumbo MTU, but honestly
I feel like saying that the transmit paths need thrown away and totally
rewritten - including that absolutely wrong max_mtu setting in the
probe function.

However, the problem is... I don't have hardware that uses normal nor
enhanced descriptors - my hardware is a nVidia Xavier, which is a
dwmac4/5 implementation using a different descriptor format, so I
can only make changes through sets of simple transformations.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!