net: stmmac: Prevent NULL deref when RX memory exhausted

The CPU receives frames from the MAC through conventional DMA: the CPU allocates buffers for the MAC, then the MAC fills them and returns ownership to the CPU. For each hardware RX queue, the CPU and MAC coordinate through a shared ring array of DMA descriptors: one descriptor per DMA buffer. Each descriptor includes the buffer's physical address and a status flag ("OWN") indicating which side owns the buffer: OWN=0 for CPU, OWN=1 for MAC. The CPU is only allowed to set the flag and the MAC is only allowed to clear it, and both must move through the ring in sequence: thus the ring is used for both "submissions" and "completions."

In the stmmac driver, stmmac_rx() bookmarks its position in the ring with the cur_rx index. The main receive loop in that function checks for rx_descs[cur_rx].own=0, gives the corresponding buffer to the network stack (NULLing the pointer), and increments cur_rx modulo the ring size. After the loop exits, stmmac_rx_refill(), which bookmarks its position with dirty_rx, allocates fresh buffers and rearms the descriptors (setting OWN=1). If it fails any allocation, it simply stops early (leaving OWN=0) and will retry where it left off when next called.

This means descriptors have a three-stage lifecycle (terms my own):
- empty (OWN=1, buffer valid)
- full (OWN=0, buffer valid and populated)
- dirty (OWN=0, buffer NULL)

But because stmmac_rx() only checks OWN, it confuses full/dirty. In the past (see 'Fixes:'), there was a bug where the loop could cycle cur_rx all the way back to the first descriptor it dirtied, resulting in a NULL dereference when mistaken for full. The aforementioned commit resolved that *specific* failure by capping the loop's iteration limit at dma_rx_size - 1, but this is only a partial fix: if the previous stmmac_rx_refill() didn't complete, then there are leftover dirty descriptors that the loop might encounter without needing to cycle fully around. The current code therefore panics (see 'Closes:') when stmmac_rx_refill() is memory-starved long enough for cur_rx to catch up to dirty_rx.

Fix this by further tightening the clamp from dma_rx_size - 1 to dma_rx_size - stmmac_rx_dirty() - 1, subtracting any remnant dirty entries and limiting the loop so that cur_rx cannot catch back up to dirty_rx. This carries no risk of arithmetic underflow: since the maximum possible return value of stmmac_rx_dirty() is dma_rx_size - 1, the worst the clamp can do is prevent the loop from running at all.

Fixes b6cb4541853c7 ("net: stmmac: avoid rx queue overrun")

Closes https://bugzilla.kernel.org/show_bug.cgi?id=221010

Cc [email protected]

Signed-off-by Sam Edwards <[email protected]>

Quoted 44 lines · Sam Edwards

The CPU receives frames from the MAC through conventional DMA: the CPU allocates buffers for the MAC, then the MAC fills them and returns ownership to the CPU. For each hardware RX queue, the CPU and MAC coordinate through a shared ring array of DMA descriptors: one descriptor per DMA buffer. Each descriptor includes the buffer's physical address and a status flag ("OWN") indicating which side owns the buffer: OWN=0 for CPU, OWN=1 for MAC. The CPU is only allowed to set the flag and the MAC is only allowed to clear it, and both must move through the ring in sequence: thus the ring is used for both "submissions" and "completions."

In the stmmac driver, stmmac_rx() bookmarks its position in the ring with the cur_rx index. The main receive loop in that function checks for rx_descs[cur_rx].own=0, gives the corresponding buffer to the network stack (NULLing the pointer), and increments cur_rx modulo the ring size. After the loop exits, stmmac_rx_refill(), which bookmarks its position with dirty_rx, allocates fresh buffers and rearms the descriptors (setting OWN=1). If it fails any allocation, it simply stops early (leaving OWN=0) and will retry where it left off when next called.

This means descriptors have a three-stage lifecycle (terms my own):
- empty (OWN=1, buffer valid)
- full (OWN=0, buffer valid and populated)
- dirty (OWN=0, buffer NULL)

But because stmmac_rx() only checks OWN, it confuses full/dirty. In the past (see 'Fixes:'), there was a bug where the loop could cycle cur_rx all the way back to the first descriptor it dirtied, resulting in a NULL dereference when mistaken for full. The aforementioned commit resolved that *specific* failure by capping the loop's iteration limit at dma_rx_size - 1, but this is only a partial fix: if the previous stmmac_rx_refill() didn't complete, then there are leftover dirty descriptors that the loop might encounter without needing to cycle fully around. The current code therefore panics (see 'Closes:') when stmmac_rx_refill() is memory-starved long enough for cur_rx to catch up to dirty_rx.

Fix this by further tightening the clamp from dma_rx_size - 1 to dma_rx_size - stmmac_rx_dirty() - 1, subtracting any remnant dirty entries and limiting the loop so that cur_rx cannot catch back up to dirty_rx. This carries no risk of arithmetic underflow: since the maximum possible return value of stmmac_rx_dirty() is dma_rx_size - 1, the worst the clamp can do is prevent the loop from running at all.

Quoted 4 lines

Fixes b6cb4541853c7 ("net: stmmac: avoid rx queue overrun")

Closes https://bugzilla.kernel.org/show_bug.cgi?id=221010

Cc [email protected]

Signed-off-by Sam Edwards <[email protected]>

Locally, while debugging my issues, I used this to prevent cur_rx catching up with dirty_rx:

                status = stmmac_rx_status(priv, &priv->xstats, p);
                /* check if managed by the DMA otherwise go ahead */
                if (unlikely(status & dma_own))
                        break;

                next_entry = STMMAC_NEXT_ENTRY(rx_q->cur_rx,
                                               priv->dma_conf.dma_rx_size);
                if (unlikely(next_entry == rx_q->dirty_rx))
                        break;

                rx_q->cur_rx = next_entry;

If we care about the cost of reloading rx_q->dirty_rx on every iteration, then I'd suggest that the cost we already incur reading and writing rx_q->cur_rx is something that should be addressed, and eliminating that would counter the cost of reading rx_q->dirty_rx. I suspect, however, that the cost is minimal, as cur_tx and dirty_rx are likely in the same cache line.

It looks like any fix to stmmac_rx() will also need a corresponding fix for stmmac_rx_zc().

Quoted 13 lines · Russell King (Oracle)

Locally, while debugging my issues, I used this to prevent cur_rx catching up with dirty_rx:

                status = stmmac_rx_status(priv, &priv->xstats, p);
                /* check if managed by the DMA otherwise go ahead */
                if (unlikely(status & dma_own))
                        break;

                next_entry = STMMAC_NEXT_ENTRY(rx_q->cur_rx,
                                               priv->dma_conf.dma_rx_size);
                if (unlikely(next_entry == rx_q->dirty_rx))
                        break;

Quoted 8 lines

                rx_q->cur_rx = next_entry;

If we care about the cost of reloading rx_q->dirty_rx on every iteration, then I'd suggest that the cost we already incur reading and writing rx_q->cur_rx is something that should be addressed, and eliminating that would counter the cost of reading rx_q->dirty_rx. I suspect, however, that the cost is minimal, as cur_tx and dirty_rx are likely in the same cache line.

Quoted 2 lines

It looks like any fix to stmmac_rx() will also need a corresponding fix for stmmac_rx_zc().

I have some further information, but a new curveball has just been chucked... and I've no idea what this will mean at this stage. Just take it that I won't be responding for a while.

Quoted 15 lines · Russell King (Oracle)

Quoted 13 lines · Russell King (Oracle)

Locally, while debugging my issues, I used this to prevent cur_rx catching up with dirty_rx:

                status = stmmac_rx_status(priv, &priv->xstats, p);
                /* check if managed by the DMA otherwise go ahead */
                if (unlikely(status & dma_own))
                        break;

                next_entry = STMMAC_NEXT_ENTRY(rx_q->cur_rx,
                                               priv->dma_conf.dma_rx_size);
                if (unlikely(next_entry == rx_q->dirty_rx))
                        break;

Quoted 8 lines

Quoted 8 lines

                rx_q->cur_rx = next_entry;

If we care about the cost of reloading rx_q->dirty_rx on every iteration, then I'd suggest that the cost we already incur reading and writing rx_q->cur_rx is something that should be addressed, and eliminating that would counter the cost of reading rx_q->dirty_rx. I suspect, however, that the cost is minimal, as cur_tx and dirty_rx are likely in the same cache line.

No, no, I like your approach better. :) It also removes the need for the limit clamp at the top of the function, so later code can assume limit==budget.

Quoted 2 lines

Quoted 2 lines

It looks like any fix to stmmac_rx() will also need a corresponding fix for stmmac_rx_zc().

I agree that stmmac_rx_zc() is likely also broken (in a similar way, but not similar enough to permit a "corresponding" fix), but I don't agree that there's a dependency relationship here. This patch is addressing #221010, which affects the generic/non-ZC codepath; I'm afraid the ZC codepath warrants its own investigation.

Quoted 3 lines

I have some further information, but a new curveball has just been chucked... and I've no idea what this will mean at this stage. Just take it that I won't be responding for a while.

I think I follow your meaning. Good luck getting it straightened out!