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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Nov 2015 00:12:31 +0100
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:	David Miller <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, 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 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.

>> +                * 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?

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