netdev - Re: Very Minor Patch to loopback.c

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: <CAGVrzcaoJ4vdKoaMOh-XDKkOSSu6Q1W3axkzu6-B_e4-GZeu0g@mail.gmail.com>
Date:	Thu, 12 Jun 2014 13:46:24 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Zachary <zacharyw09264@...il.com>
Cc:	netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: Very Minor Patch to loopback.c

2014-06-12 13:30 GMT-07:00 Zachary <zacharyw09264@...il.com>:
> Should this be changed in dummy.c then to prevent the same bug?

dummy.c uses skb->len before calling dev_kfree_skb(), and in its
transmit path, which is a valid use case that will not introduce use
after free issues.

(BTW: we avoid top-posting on netdev and other linux mailing-lists)

>
> On Thu, Jun 12, 2014 at 4:27 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
>> 2014-06-12 12:53 GMT-07:00 Zachary <zacharyw09264@...il.com>:
>>> Very minor patch to loopback.c that I noticed while comparing the
>>> dummy.c code to loopback.c code (dummy.c was based on loopback.c).  I
>>> did not see a maintainer in MAINTAINERS in my cursory glance, so I
>>> apologize if this is to the wrong place.  I am testing this patch
>>> right now on my laptop and it appears to be working without issues.
>>> It also compiled without warnings.
>>
>> Your patch introduces a potential use after free type of bug. As soon
>> as the skb pointer has been passed to netif_rx(), the network stack is
>> allowed to free this skb as soon as it has determined it is unused,
>> this is the reason why this 'len' variable exists in the first place.
>>
>>>
>>> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
>>> index bb96409..f9f88d7 100644
>>> --- a/drivers/net/loopback.c
>>> +++ b/drivers/net/loopback.c
>>> @@ -22,6 +22,7 @@
>>>   *                                      interface.
>>>   *             Alexey Kuznetsov:       Potential hang under some extreme
>>>   *                                     cases removed.
>>> + *             Zachary Winnerman:  Extremely minor patch in loopback_xmit
>>>   *
>>>   *             This program is free software; you can redistribute it and/or
>>>   *             modify it under the terms of the GNU General Public License
>>> @@ -72,7 +73,6 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>>>                                  struct net_device *dev)
>>>  {
>>>         struct pcpu_lstats *lb_stats;
>>> -       int len;
>>>
>>>         skb_orphan(skb);
>>>
>>> @@ -86,10 +86,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>>>         /* it's OK to use per_cpu_ptr() because BHs are off */
>>>         lb_stats = this_cpu_ptr(dev->lstats);
>>>
>>> -       len = skb->len;
>>>         if (likely(netif_rx(skb) == NET_RX_SUCCESS)) {
>>>                 u64_stats_update_begin(&lb_
>>> stats->syncp);
>>> -               lb_stats->bytes += len;
>>> +               lb_stats->bytes += skb->len;
>>>                 lb_stats->packets++;
>>>                 u64_stats_update_end(&lb_stats->syncp);
>>>         }
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Florian



-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html