netdev - Re: [PATCH] net: korina: remove busy skb free

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: <20201214130832.7bedb230@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Mon, 14 Dec 2020 13:08:32 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Julian Wiedmann <jwi@...ux.ibm.com>
Cc:     Vincent Stehlé <vincent.stehle@...oste.net>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Florian Fainelli <florian.fainelli@...ecomint.eu>
Subject: Re: [PATCH] net: korina: remove busy skb free

On Mon, 14 Dec 2020 11:03:12 +0100 Julian Wiedmann wrote:
> > diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
> > index bf48f0ded9c7d..9d84191de6824 100644
> > --- a/drivers/net/ethernet/korina.c
> > +++ b/drivers/net/ethernet/korina.c
> > @@ -216,7 +216,6 @@ static int korina_send_packet(struct sk_buff *skb, struct net_device *dev)
> >  			netif_stop_queue(dev);
> >  		else {
> >  			dev->stats.tx_dropped++;
> > -			dev_kfree_skb_any(skb);
> >  			spin_unlock_irqrestore(&lp->lock, flags);
> >  
> >  			return NETDEV_TX_BUSY;
> >   
> 
> As this skb is returned to the stack (and not dropped), the tx_dropped
> statistics increment looks bogus too.

Since this is clearly an ugly use after free, and nobody complained we
can assume that the driver correctly stops its TX queue ahead of time.
So perhaps we can change the return value to NETDEV_TX_OK instead.