| 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
| ||
|
Message-ID: <87wnohqty1.wl-maz@kernel.org>
Date: Thu, 19 Aug 2021 17:29:26 +0100
From: Marc Zyngier <maz@...nel.org>
To: Matteo Croce <mcroce@...ux.microsoft.com>
Cc: Eric Dumazet <eric.dumazet@...il.com>,
Thierry Reding <thierry.reding@...il.com>,
netdev@...r.kernel.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-riscv <linux-riscv@...ts.infradead.org>,
Giuseppe Cavallaro <peppe.cavallaro@...com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Drew Fustini <drew@...gleboard.org>,
Emil Renner Berthing <kernel@...il.dk>,
Jon Hunter <jonathanh@...dia.com>,
Will Deacon <will@...nel.org>
Subject: Re: [PATCH net-next] stmmac: align RX buffers
On Thu, 12 Aug 2021 12:18:48 +0100,
Matteo Croce <mcroce@...ux.microsoft.com> wrote:
>
> [1 <text/plain; UTF-8 (7bit)>]
> On Thu, Aug 12, 2021 at 1:05 PM Marc Zyngier <maz@...nel.org> wrote:
> >
> > On Thu, 12 Aug 2021 11:18:35 +0100,
> > Matteo Croce <mcroce@...ux.microsoft.com> wrote:
> > >
> > > On Thu, 12 Aug 2021 10:48:03 +0200
> > > Eric Dumazet <eric.dumazet@...il.com> wrote:
> > >
> > > >
> > > >
> > > > On 8/11/21 4:16 PM, Marc Zyngier wrote:
> > > > > On Wed, 11 Aug 2021 13:53:59 +0100,
> > > > > Eric Dumazet <eric.dumazet@...il.com> wrote:
> > > > >
> > > > >> Are you sure you do not need to adjust stmmac_set_bfsize(),
> > > > >> stmmac_rx_buf1_len() and stmmac_rx_buf2_len() ?
> > > > >>
> > > > >> Presumably DEFAULT_BUFSIZE also want to be increased by NET_SKB_PAD
> > > > >>
> > > > >> Patch for stmmac_rx_buf1_len() :
> > > > >>
> > > > >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index
> > > > >> 7b8404a21544cf29668e8a14240c3971e6bce0c3..041a74e7efca3436bfe3e17f972dd156173957a9
> > > > >> 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++
> > > > >> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -4508,12
> > > > >> +4508,12 @@ static unsigned int stmmac_rx_buf1_len(struct
> > > > >> stmmac_priv *priv, /* First descriptor, not last descriptor and
> > > > >> not split header */ if (status & rx_not_ls)
> > > > >> - return priv->dma_buf_sz;
> > > > >> + return priv->dma_buf_sz - NET_SKB_PAD -
> > > > >> NET_IP_ALIGN;
> > > > >> plen = stmmac_get_rx_frame_len(priv, p, coe);
> > > > >>
> > > > >> /* First descriptor and last descriptor and not split
> > > > >> header */
> > > > >> - return min_t(unsigned int, priv->dma_buf_sz, plen);
> > > > >> + return min_t(unsigned int, priv->dma_buf_sz - NET_SKB_PAD
> > > > >> - NET_IP_ALIGN, plen); }
> > > > >>
> > > > >> static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> > > > >
> > > > > Feels like a major deficiency of the original patch. Happy to test a
> > > > > more complete patch if/when you have one.
> > > >
> > > > I wont have time in the immediate future.
> > > >
> > > > Matteo, if you do not work on a fix, I suggest we revert
> > > > a955318fe67ec0d962760b5ee58e74bffaf649b8 stmmac: align RX buffers
> > > >
> > > > before a more polished version can be submitted.
> > > >
> > >
> > > Better to use stmmac_rx_offset() so to have the correct length when
> > > using XDP. Also, when XDP is enabled, the offset was
> > > XDP_PACKET_HEADROOM (i.e. 256 bytes) even before the change, so it
> > > could be already broken. Mark, can you try on the Jetson TX2 by
> > > attaching an XDP program and see if it works without my change?
> >
> > Sorry, you'll have to hold my hand here, as I know exactly nothing
> > about XDP....
> >
>
> Attach the attached object with:
>
> ip link set eth0 xdp object passall.o
>
> This is an empty XDP program, its source:
>
> __attribute__((section("prog"), used))
> int xdp_main(struct xdp_md *ctx)
> {
> return XDP_PASS;
> }
>
> Every packet will pass untouched, but the offset will be shifted from
> 0 to 256 bytes, which could trigger the problem anyway:
Nope. On 5.13, which doesn't have the issue, adding this payload
doesn't result in any problem and the whole thing is rock solid.
>
> > > A possible fix, which takes in account also the XDP headroom for
> > > stmmac_rx_buf1_len() only could be (only compile tested, I don't have
> > > the hardware now):
> >
> > However, this doesn't fix my issue. I still get all sort of
> > corruption. Probably stmmac_rx_buf2_len() also need adjusting (it has
> > a similar logic as its buf1 counterpart...)
> >
> > Unless you can fix it very quickly, and given that we're towards the
> > end of the cycle, I'd be more comfortable if we reverted this patch.
> >
>
> Can it be that the HW can't do DMA on an address which is not word aligned?
> What if you replace NET_SKB_PAD with, let's say, 8?
With this:
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index fcdb1d20389b..244aa6579ef4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -341,7 +341,7 @@ static inline unsigned int stmmac_rx_offset(struct stmmac_priv *priv)
if (stmmac_xdp_is_enabled(priv))
return XDP_PACKET_HEADROOM + NET_IP_ALIGN;
- return NET_SKB_PAD + NET_IP_ALIGN;
+ return 8 + NET_IP_ALIGN;
}
void stmmac_disable_rx_queue(struct stmmac_priv *priv, u32 queue);
I don't see the system corrupting packets anymore. Is that exactly
what you had in mind? This really seems to point to a basic buffer
overflow.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists