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]

Message-ID: <CANn89i+Ms_vvuowjmgERsA3=rMdTm_Cux3XTVJ19ewTz-TtBOw@mail.gmail.com>
Date:   Thu, 30 Mar 2017 06:36:43 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Vincent Bernat <vincent@...nat.im>
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

On Wed, Mar 29, 2017 at 11:41 PM, Vincent Bernat <vincent@...nat.im> wrote:
>  ❦ 29 mars 2017 22:47 +0200, Vincent Bernat <vincent@...nat.im> :
>
>> 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;

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);