netdev - Re: Commit 05cf0d1bf4 ("net: stmmac: free an skb first when there are no longer any descriptors using it") breaks stmmac?

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] [day] [month] [year] [list]

Date:   Fri, 16 Feb 2018 11:10:52 +0100
From:   Niklas Cassel <niklas.cassel@...s.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     Joao Pinto <Joao.Pinto@...opsys.com>,
        linux-netdev <netdev@...r.kernel.org>,
        Giuseppe CAVALLARO <peppe.cavallaro@...com>,
        alexandre.torgue@...com
Subject: Re: Commit 05cf0d1bf4 ("net: stmmac: free an skb first when there
 are no longer any descriptors using it") breaks stmmac?

On Fri, Feb 16, 2018 at 09:34:39AM +0000, Jose Abreu wrote:
> Hi Niklas,
> 
> Thank you for looking into this!
> 
> On 13-02-2018 13:33, Niklas Cassel wrote:
> > Hello Jose,
> >
> >
> > I remember that you had a problem
> > with a use after free in stmmac_tx_clean().
> > I still don't think that it is related to
> > commit 05cf0d1bf4, however, when comparing
> > the stmmac driver to the amd-xgbe driver
> > I realized that:
> >
> > xgbe_tx_poll() has both a smp_rmb() after fetching
> > cur_tx, and also a dma_rmb() after reading the own
> > bit, before reading any other descriptor fields.
> >
> > stmmac_tx_clean() has neither a smp_rmb() or a
> > dma_rmb().

This still applies as far as I can tell.

> >
> >
> > Also
> > xgbe_dev_xmit() has a dma_wmb() _before_ setting
> > the own bit, and a smp_wmb() after setting the own
> > bit.
> >
> > stmmac simply has a dma_wmb() _after_ setting the
> > own bit.

For dwmac4, stmmac actually has a another dma_wmb() hidden in
dwmac4_rd_prepare_tx_desc().

It might be more obvious, since there already is a dma_wmb()
in stmmac_xmit()/stmmac_tso_xmit(), if this was also placed
there. We already have a set_owner() abstraction.


xgbe also has a dma_wmb() in dwmac4_release_tx_desc(),
which we appear to be missing.
Pehaps we want it right after the
priv->hw->desc->release_tx_desc(p, priv->mode)
call, so we don't hide it in the different implementations.

> > If you can still reproduce your problem quite easily,
> > perhaps you could try to make stmmac look more like
> > xgbe in these regards, and see if that solves your
> > use after free crash in stmmac_tx_clean().
> 
> At the time it would be easily reproduced if I flooded the
> interface. I will check xgbe barriers and check if it should also
> be used in stmmac.

I've fixed the tx timeout for multiqueues when running iperf3
(mss was not set per tx queue, as reported in another thread).
Will send out a patch today.


Regards,
Niklas