netdev - Re: [PATCH v2] smsc95xx: Use skb_cow

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:   Wed, 19 Apr 2017 10:40:18 +0100
From:   James Hughes <james.hughes@...pberrypi.org>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Woojung.Huh@...rochip.com, netdev@...r.kernel.org,
        Steve Glendinning <steve.glendinning@...well.net>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs

On 19 April 2017 at 09:33, James Hughes <james.hughes@...pberrypi.org> wrote:
> On 18 April 2017 at 23:46, Eric Dumazet <eric.dumazet@...il.com> wrote:
>> On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@...rochip.com wrote:
>>> > > @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
>>> > usbnet *dev,
>>> > >   /* We do not advertise SG, so skbs should be already linearized */
>>> > >   BUG_ON(skb_shinfo(skb)->nr_frags);
>>> > >
>>> > > - if (skb_headroom(skb) < overhead) {
>>> > > -         struct sk_buff *skb2 = skb_copy_expand(skb,
>>> > > -                 overhead, 0, flags);
>>> > > -         dev_kfree_skb_any(skb);
>>> > > -         skb = skb2;
>>> > > -         if (!skb)
>>> > > -                 return NULL;
>>> > > + /* Make writable and expand header space by overhead if required
>>> > */
>>> > > + if (skb_cow_head(skb, overhead)) {
>>> >
>>> > I believe you still need to
>>> >             dev_kfree_skb_any(skb);
>>> >
>>> I think caller of usbnet_start_xmit() takes care of free when return NULL.
>>
>> I do not think so. Here is the code in usbnet_start_xmit() :
>>
>> if (info->tx_fixup) {
>>    skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>>
>>    if (!skb) { // Note that skb is NULL now
>>
>>        if (info->flags & FLAG_MULTI_PACKET)
>>              goto not_drop;
>>         netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
>>         goto drop;
>>
>>
>> If you really think about it (even before double checking the code in
>> usbnet_start_xmit()), that would have been a very serious bug.
>>
>> Calling dev_kfree_skb_any(skb) in this fixup code, then later from
>> usbnet_start_xmit() would have been a double free, or use after free.
>>
>>
>
> So, I still need to return NULL (as per the code this is replacing) to
> indicate failure, but need to free the skb prior to return, as per
> fragment below.
>
> /* Make writable and expand header space by overhead if required */
> if (skb_cow_head(skb, overhead)) {
>    dev_kfree_skb_any(skb);
>    return NULL;
> }
>
> Once confirmed, I'll do a another patch.

Actually, having checked the code path, doing the free here does look
like the correct operation. Returning NULL indicates to the caller
that the call has failed, but also means the skb is not deallocated
later on. It's seems rather a clunky mechanism for the driver to have
to deallocate, but I guess that's 'just the way it is'.

I'll do another patch.