netdev - RE: Invalid transport_offset with AF

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]

Message-ID: <HE1PR0502MB389931B7EECAB63373ACCC4DD1D10@HE1PR0502MB3899.eurprd05.prod.outlook.com>
Date:   Wed, 28 Nov 2018 11:08:07 +0000
From:   Maxim Mikityanskiy <maximmi@...lanox.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     Network Development <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Jason Wang <jasowang@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: RE: Invalid transport_offset with AF_PACKET socket

Hi Willem,

> That is not what offset_hint 0 does. It also sets the transport header
> to the same as the network header.

No, it's not accurate. With offset_hint == 0 I have the transport header set to
the offset of the mac header. It's impossible that both offset_hint == 0 and 14
set skb->transport_header to the same value.

> This does leave the question whether the transport header would
> be better of not being set if it cannot be probed. I have not checked
> whether anything in the stack requires it to be set (i.e., not ~0U).

Well, at least we can craft a packet without the transport header at all using
AF_PACKET, and if some code expects transport_header to be set, it probably
wants it to be some reasonable value, not just pointing to some random bytes.
It's difficult to track down every usage of skb_transport_header, but if someone
expects it to be set when it can be set to a wrong value, they are already
having some trouble.

To have it set to ~0U looks totally reasonable if offset == 0 is not really a
special value, as I thought.

However, it would be good to have two special values for the header offset: one
would indicate it wasn't set, and the other would indicate that it has been
probed, but no transport header exists. It would allow to avoid probing a second
time, just because we don't know whether the transport header has been probed
or not.

Also, there are some places in kernel where skb_probe_transport_header is called
with offset_hint == 0, probably meaning that there is no hint, but the offset is
set anyway. Maybe it would be a good idea to allow probing without the offset
hint, so that if probing fails, transport_header is not set to some buggy value?

> Instrumenting the functions to see the offsets, I observe the
> following offsets from skb->head:
>
> packet_snd, sock_raw:       data=2, nh=16, th=16
> packet_snd, sock_dgram:   data=2, nh=16, th=2
> tpacket_snd, sock_raw:      data=2, nh=16, th=2
>
> Recall that data has to point to the link layer header at this
> point, so the ETH_HLEN difference between data and nh is correct.
>
> Of these, only the first looks remotely correct to me.

IMO, in all of these cases, the transport offset shouldn't be set to the network
or mac header.

> Yet this
> is the (only?) one that you observed causing problems for the
> device. Perhaps because offset 0 is treated as a special valid
> case in mlx5e_calc_min_inline.

Yes, the second and third cases don't cause the bug for the device, exactly for
this reason. Still, either some special value indicating "have probed, but no
transport header" should be legalized, or ~0U should be used.

Thanks,
Max