netdev - Re: [PATCH net v1] net: stmmac: TSO: Fix unbalanced DMA map/unmap for non-paged SKB data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z1CFz7GpeIzkDro1@shell.armlinux.org.uk>
Date: Wed, 4 Dec 2024 16:39:43 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Thierry Reding <thierry.reding@...il.com>
Cc: Robin Murphy <robin.murphy@....com>, Furong Xu <0x1207@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Jon Hunter <jonathanh@...dia.com>,
	netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, xfr@...look.com,
	Suraj Jaiswal <quic_jsuraj@...cinc.com>,
	Thierry Reding <treding@...dia.com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Will Deacon <will@...nel.org>
Subject: Re: [PATCH net v1] net: stmmac: TSO: Fix unbalanced DMA map/unmap
 for non-paged SKB data

On Wed, Dec 04, 2024 at 04:58:34PM +0100, Thierry Reding wrote:
> This doesn't match the location from earlier, but at least there's
> something afoot here that needs fixing. I suppose this could simply be
> hiding any subsequent errors, so once this is fixed we might see other
> similar issues.

Well, having a quick look at this, the first thing which stands out is:

In stmmac_tx_clean(), we have:

                if (likely(tx_q->tx_skbuff_dma[entry].buf &&
                           tx_q->tx_skbuff_dma[entry].buf_type != STMMAC_TXBUF_T
_XDP_TX)) {
                        if (tx_q->tx_skbuff_dma[entry].map_as_page)
                                dma_unmap_page(priv->device,
                                               tx_q->tx_skbuff_dma[entry].buf,
                                               tx_q->tx_skbuff_dma[entry].len,
                                               DMA_TO_DEVICE);
                        else
                                dma_unmap_single(priv->device,
                                                 tx_q->tx_skbuff_dma[entry].buf,
                                                 tx_q->tx_skbuff_dma[entry].len,
                                                 DMA_TO_DEVICE);
                        tx_q->tx_skbuff_dma[entry].buf = 0;
                        tx_q->tx_skbuff_dma[entry].len = 0;
                        tx_q->tx_skbuff_dma[entry].map_as_page = false;
                }

So, tx_skbuff_dma[entry].buf is expected to point appropriately to the
DMA region.

Now if we look at stmmac_tso_xmit():

        des = dma_map_single(priv->device, skb->data, skb_headlen(skb),
                             DMA_TO_DEVICE);
        if (dma_mapping_error(priv->device, des))
                goto dma_map_err;

        if (priv->dma_cap.addr64 <= 32) {
...
        } else {
...
                des += proto_hdr_len;
...
	}

        tx_q->tx_skbuff_dma[tx_q->cur_tx].buf = des;
        tx_q->tx_skbuff_dma[tx_q->cur_tx].len = skb_headlen(skb);
        tx_q->tx_skbuff_dma[tx_q->cur_tx].map_as_page = false;
        tx_q->tx_skbuff_dma[tx_q->cur_tx].buf_type = STMMAC_TXBUF_T_SKB;

This will result in stmmac_tx_clean() calling dma_unmap_single() using
"des" and "skb_headlen(skb)" as the buffer start and length.

One of the requirements of the DMA mapping API is that the DMA handle
returned by the map operation will be passed into the unmap function.
Not something that was offset. The length will also be the same.

We can clearly see above that there is a case where the DMA handle has
been offset by proto_hdr_len, and when this is so, the value that is
passed into the unmap operation no longer matches this requirement.

So, a question to the reporter - what is the value of
priv->dma_cap.addr64 in your failing case? You should see the value
in the "Using %d/%d bits DMA host/device width" kernel message.

Thanks.

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