netdev - Re: [net-next v2] vxlan: fix ND proxy when skb doesn't have transport header offset

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:   Thu, 30 Mar 2017 16:56:18 +0200
From:   Vincent Bernat <vincent@...nat.im>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Roopa Prabhu <roopa@...ulusnetworks.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jiri Benc <jbenc@...hat.com>, netdev <netdev@...r.kernel.org>
Subject: Re: [net-next v2] vxlan: fix ND proxy when skb doesn't have transport header offset

 ❦ 30 mars 2017 06:36 -0700, Eric Dumazet <edumazet@...gle.com> :

>>> Parsing of neighbor discovery options is done earlier to ignore the
>>> whole packet in case of a malformed option. Moreover, the assumption the
>>> skb was linear is removed and options are extracted with
>>> skb_header_pointer() as well. The check on the source link-layer address
>>> option is also more strict (for Ethernet, we expect the length field to
>>> be 1).
>>
>> There is some parsing implemented in net/ipv6/ndisc.c, notably
>> ndisc_parse_options(). I don't know if this is a good idea to reuse
>> that: it may have the expectation that some IP processing has already
>> been done (for example, the IPv6 length has already been checked, the
>> SKB is expected to be linear).
>
> Forcing ICMP being linear is probably fine.
>
> Prior to 91269e390d062b52 ("vxlan: using pskb_may_pull as early as possible")
> this was happening (at the wrong place) in neigh_reduce() doing a :
> if (!pskb_may_pull(skb, skb->len))
>      goto out;

OK, I'll simplify the patch then.

> So the following would be ok (while incomplete of course)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index bdb6ae16d4a85bf9539199e189011bce104ba51a..cd032819d4a36d5ca94739f20f947f3f5a31aba3
> 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -2243,12 +2243,13 @@ static netdev_tx_t vxlan_xmit(struct sk_buff
> *skb, struct net_device *dev)
>                         return arp_reduce(dev, skb, vni);
>  #if IS_ENABLED(CONFIG_IPV6)
>                 else if (ntohs(eth->h_proto) == ETH_P_IPV6 &&
> -                        pskb_may_pull(skb, sizeof(struct ipv6hdr)
> -                                      + sizeof(struct nd_msg)) &&
> +                        skb->len >= sizeof(struct ipv6hdr) +
> +                                    sizeof(struct nd_msg) &&
> +                        pskb_may_pull(skb, skb->len) &&
>                          ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6) {
>                                 struct nd_msg *msg;
>
> -                               msg = (struct nd_msg
> *)skb_transport_header(skb);
> +                               msg = (struct nd_msg *)(ipv6_hdr(skb) + 1);
>                                 if (msg->icmph.icmp6_code == 0 &&
>                                     msg->icmph.icmp6_type ==
> NDISC_NEIGHBOUR_SOLICITATION)
>                                         return neigh_reduce(dev, skb, vni);

pskb_may_pull() is called while we only know this is an IPv6 packet, not
an ICMPv6 one. I'll keep skb_header_pointer to handle IPv6 header, then
I'll pull the whole ICMP packet (unless I am mistaken, of course).
-- 
The devil can cite Scripture for his purpose.
		-- William Shakespeare, "The Merchant of Venice"