netdev - RE: [BUG] In af_packet.c::dev_parse_header() skb.network_header does not point to the network header

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: <64362359316d5_1b9cfb29415@willemb.c.googlers.com.notmuch>
Date:   Tue, 11 Apr 2023 23:19:53 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Aleksey Shumnik <ashumnik9@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, waltje@...lt.nl.mugnet.org,
        gw4pts@...pts.ampr.org, xeb@...l.ru, kuznet@....inr.ac.ru,
        rzsfl@...uni-sb.de, gnault@...hat.com
Subject: RE: [BUG] In af_packet.c::dev_parse_header() skb.network_header does
 not point to the network header

Aleksey Shumnik wrote:
> Dear maintainers,
> 
> I wrote the ip6gre_header_parser() function in ip6_gre.c, the
> implementation is similar to ipgre_header_parser() in ip_gre.c. (By
> the way, it is strange that this function is not implemented in
> ip6_gre.c)
> The implementation of the function is presented below.
> It should parse the ip6 header and take the source address and its
> length from there. To get a pointer to the ip header, it is logical to
> use skb_network_header(), but it does not work correctly and returns a
> pointer to payload (skb.data).
> Also in ip_gre.c::ipgre_header_parser() skb_mac_header() returns a
> pointer to the ip header and everything works correctly (although it
> seems that this is also an error, because the pointer to the mac
> header should have been returned, and logically the
> skb_network_header() function should be applied), 

For a device of type ARPHRD_IPGRE or ARPHRD_IP6GRE there is no other
MAC header. This is not ARPHRD_ETHER.

The link layer header can be seen by looking for header_ops.create
if it exists. This creates the header for packet sockets of type
SOCK_DGRAM.

> but in ip6_gre.c all
> skb_mac_header(), skb_network_header(), skb_tranport_header() returns
> a pointer to payload (skb.data).
> This function is called when receiving a packet and parsing it in
> af_packet.c::packet_rcv() in dev_parse_header().
> The problem is that there is no way to accurately determine the
> beginning of the ip header.

The issue happens when comparing packet_rcv on an ip_gre tunnel vs an
ip6_gre tunnel.

The packet_rcv call does the same in both cases, e.g., setting
skb->data at mac or network header depending on SOCK_DGRAM or
SOCK_RAW.

The issue then is likely with a difference in tunnel implementations.
Both implement header_ops and header_ops.create (which is used on
receive by dev_has_header, but on transmit by dev_hard_header). They
return different lengths: one with and one without the IP header.

We've seen inconsistency in this before between tunnels. See also
commit aab1e898c26c. ipgre_xmit has special logic to optionally pull
the headers, but only if header_ops is set, which it isn't for all
variants of GRE tunnels.

Probably particularly relevant is this section in __ipgre_rcv:

                /* Special case for ipgre_header_parse(), which expects the
                 * mac_header to point to the outer IP header.
                 */
                if (tunnel->dev->header_ops == &ipgre_header_ops)
                        skb_pop_mac_header(skb);
                else
                        skb_reset_mac_header(skb);

and see this comment in the mentioned commit:

    ipgre_header_parse() seems to be the only case that requires mac_header
    to point to the outer header. We can detect this case accurately by
    checking ->header_ops. For all other cases, we can reset mac_header.

> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 90565b8..0d0c37b 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -1404,8 +1404,16 @@ static int ip6gre_header(struct sk_buff *skb,
> struct net_device *dev,
>   return -t->hlen;
>  }
> 
> +static int ip6gre_header_parse(const struct sk_buff *skb, unsigned char *haddr)
> +{
> + const struct ipv6hdr *ipv6h = (const struct ipv6hdr *) skb_mac_header(skb);
> + memcpy(haddr, &ipv6h->saddr, 16);
> + return 16;
> +}
> +
>  static const struct header_ops ip6gre_header_ops = {
>   .create = ip6gre_header,
> + .parse = ip6gre_header_parse,
>  };
> 
>  static const struct net_device_ops ip6gre_netdev_ops = {
> 
> Would you answer whether this behavior is an error and why the
> behavior in ip_gre.c and ip6_gre.c differs?
> 
> Regards,
> Aleksey