netdev - Re: [PATCH net v2 3/3] packet: fix tpacket

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 11 Nov 2015 09:56:56 -0500
From:	Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:	Daniel Borkmann <daniel@...earbox.net>
Cc:	Willem de Bruijn <willemdebruijn.kernel@...il.com>,
	David Miller <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Tobias Klauser <tklauser@...tanz.ch>,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2 3/3] packet: fix tpacket_snd max frame and vlan handling

On Tue, Nov 10, 2015 at 6:51 PM, Daniel Borkmann <daniel@...earbox.net> wrote:
> On 11/11/2015 12:24 AM, Willem de Bruijn wrote:
>>
>> On Tue, Nov 10, 2015 at 6:12 PM, Daniel Borkmann <daniel@...earbox.net>
>> wrote:
>>>
>>> On 11/10/2015 11:52 PM, Willem de Bruijn wrote:
>>>>>
>>>>>
>>>>>           if (sock->type == SOCK_DGRAM) {
>>>>> -               err = dev_hard_header(skb, dev, ntohs(proto), addr,
>>>>> -                               NULL, tp_len);
>>>>> +               /* In DGRAM sockets, we expect struct sockaddr_ll was
>>>>> filled
>>>>> +                * via struct msghdr, so we have dest mac and
>>>>> skb->protocol.
>>>>> +                * Otherwise there's not too much useful things we can
>>>>> do
>>>>> in
>>>>> +                * this flush run.
>>>>> +                */
>>>>> +               err = dev_hard_header(skb, dev, ntohs(skb->protocol),
>>>>> addr,
>>>>> +                                     NULL, tp_len);
>>>>
>>>>
>>>>
>>>> This change is not really necessary.
>>>
>>>
>>>
>>> Sure agreed, I found it helpful though. Don't mind removing it.
>>>
>>>>>                   if (unlikely(err < 0))
>>>>>                           return -EINVAL;
>>>>> -       } else if (dev->hard_header_len) {
>>>>
>>>>
>>>>
>>>> Why remove the check on hard_header_len?
>>>
>>>
>>>
>>> Hmm, the patch doesn't remove the check (it's moved further below).
>>>
>>>>> -               if (ll_header_truncated(dev, tp_len))
>>>>> -                       return -EINVAL;
>>>>> +       } else {
>>>>> +               /* If skb->protocol is still 0, try to infer/guess it.
>>>>> Might
>>>>> +                * not be fully reliable in the sense that a user could
>>>>> still
>>>>> +                * change/race data afterwards, but on the other hand
>>>>> the
>>>>> proto
>>>>
>>>>
>>>>
>>>> The race goes away when probing it after the copy in skb_store_bits.
>>>> Then it is also certain that tp_len is long enough to hold the entire
>>>> link layer header.
>>>
>>>
>>>
>>> The skb_store_bits() is only done in case we do have a
>>> dev->hard_header_len
>>> or in case where we run into a possible situation where we have the
>>> additional
>>> 4 bytes on a full frame. In that case we need to check them properly,
>>> which
>>> requires copying, otherwise we don't copy any header.
>>
>>
>> I assumed that hard_header_len has to be non-zero if there
>> is a link layer header to probe. If we only intend to implement
>> probing in the case of Ethernet, then it certainly holds.
>
>
> Yeah, I guess we only care about ether_setup() alike devices, so we'd have
> ETH_HLEN room as dev->hard_header_len. That will hold, yes.
>
>>>>> +                * can be set arbitrarily anyways. We only need to take
>>>>> care
>>>>> +                * in case of extra large VLAN frames.
>>>>> +                */
>>>>> +               if (!skb->protocol && tp_len >= ETH_HLEN)
>>>>> +                       skb->protocol = ((struct ethhdr
>>>>> *)data)->h_proto;
>>>>
>>>>
>>>>
>>>> Packet sockets are not restricted to link layer of type Ethernet.
>>>>
>>>> There are a few other points in this file that also cast mac header
>>>> to eth_hdr(skb).
>>>
>>>
>>>
>>> Ok, the set doesn't address this assumption which we have elsewhere, too.
>>> Do you suggest to also check on dev->type for these cases?
>>
>>
>> Yes. If I'm right, then the other cases have to be fixed, too. One of the
>> eth_hdr(skb) calls was introduced by patch 09effa67a18d, where the
>> deleted code shows how it is safely restricted to ethernet:
>>
>>    -       if (dev->type == ARPHRD_ETHER) {
>>    -               skb->protocol = eth_type_trans(skb, dev);
>>    -               if (skb->protocol == htons(ETH_P_8021Q))
>
>
> Saw that, so we need the check as one more fix for 57f89bfa2140 ("network:
> Allow af_packet to transmit +4 bytes for VLAN packets.") as well.
>
> I'll see to respin a v3 tomorrow.

Thanks, Daniel.

The three existing uses of eth_hdr were applied in 2.6.39, 3.12 and 3.15.
We probably need a patch per occurrence to allow backporting to the
relevant stable branches. I can create the fixes independent from your
patch set, if you prefer.

>
>
> Thanks,
> Daniel
> --
> 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
--
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