linux-kernel - Re: net: GPF in eth

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:   Tue, 29 Nov 2016 06:58:21 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     syzkaller <syzkaller@...glegroups.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        David Miller <davem@...emloft.net>,
        Tom Herbert <tom@...bertland.com>,
        Alexander Duyck <aduyck@...antis.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Jiri Benc <jbenc@...hat.com>,
        Sabrina Dubroca <sd@...asysnail.net>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: net: GPF in eth_header

On Tue, 2016-11-29 at 11:26 +0100, Andrey Konovalov wrote:
> On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet <erdlkml@...il.com> wrote:
> >> I actually see multiple places where skb_network_offset() is used as
> >> an argument to skb_pull().
> >> So I guess every place can potentially be buggy.
> >
> > Well, I think the intent is to accept a negative number.
> 
> I'm not sure that was the intent since it results in a signedness
> issue which leads to an out-of-bounds.
> 

Hey, I already mentioned where was the bug.

You missed the investigation where I pointed it to FLorian ?

> A quick grep shows that the same issue can potentially happen in
> multiple places across the kernel:
> 
> net/ipv6/ip6_output.c:1655: __skb_pull(skb, skb_network_offset(skb));
> net/packet/af_packet.c:2043: skb_pull(skb, skb_network_offset(skb));
> net/packet/af_packet.c:2165: skb_pull(skb, skb_network_offset(skb));
> net/core/neighbour.c:1301: __skb_pull(skb, skb_network_offset(skb));
> net/core/neighbour.c:1331: __skb_pull(skb, skb_network_offset(skb));
> net/core/dev.c:3157: __skb_pull(skb, skb_network_offset(skb));
> net/sched/sch_teql.c:337: __skb_pull(skb, skb_network_offset(skb));
> net/sched/sch_atm.c:479: skb_pull(skb, skb_network_offset(skb));
> net/ipv4/ip_output.c:1385: __skb_pull(skb, skb_network_offset(skb));
> net/ipv4/ip_fragment.c:391: if (!pskb_pull(skb, skb_network_offset(skb) + ihl))
> drivers/net/vxlan.c:1440: __skb_pull(reply, skb_network_offset(reply));
> drivers/net/vxlan.c:1902: __skb_pull(skb, skb_network_offset(skb));
> drivers/net/vrf.c:220: __skb_pull(skb, skb_network_offset(skb));
> drivers/net/vrf.c:314: __skb_pull(skb, skb_network_offset(skb));
> 
> A similar thing also happened to somebody else (on a receive path!):
> https://forums.grsecurity.net/viewtopic.php?f=3&t=4550
> 
> Does it make sense to check skb_network_offset() before passing it to
> skb_pull() everywhere?

Well, sure, we could add safety checks everywhere and slow the kernel
when debugging is requested.

But skb_network_offset() is not the problem here. Why are you focusing
on it ?

The real problem is in __skb_pull() or __skb_push() and all similar
helpers. Lots of added checks and slowdowns.

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c535fbccf2c7dbfae04cee393460e86d588c26b..d6116e37d054fc1536114347ed3c41fc7dc7a882 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1923,6 +1923,7 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
 unsigned char *skb_push(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	skb->data -= len;
 	skb->len  += len;
 	return skb->data;
@@ -1931,6 +1932,7 @@ static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
 unsigned char *skb_pull(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	skb->len -= len;
 	BUG_ON(skb->len < skb->data_len);
 	return skb->data += len;
@@ -1938,6 +1940,7 @@ static inline unsigned char *__skb_pull(struct sk_buff *skb, unsigned int len)
 
 static inline unsigned char *skb_pull_inline(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d1d1a5a5ad24ded523fc12ffba8c602b03bd0830..7bf098c848fd857ba5d287fc91d43f62f381bd55 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1450,6 +1450,7 @@ EXPORT_SYMBOL(skb_put);
  */
 unsigned char *skb_push(struct sk_buff *skb, unsigned int len)
 {
+	BUG_ON((int)len < 0);
 	skb->data -= len;
 	skb->len  += len;
 	if (unlikely(skb->data<skb->head))