Re: net: GPF in eth_header

[Andrey Konovalov]

On Tue, Nov 29, 2016 at 3:58 PM, Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> On Tue, 2016-11-29 at 11:26 +0100, Andrey Konovalov wrote:
>> On Sat, Nov 26, 2016 at 9:05 PM, Eric Dumazet <erdlkml@xxxxxxxxx> 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.

The issue is not with skb_network_offset(), but with __skb_pull()
using skb_network_offset() as an argument.

I'm not sure what would be the beast way to fix this, to add a check
before every __skb_pull(skb_network_offset()), to fix __skb_pull() to
work with signed ints, to add BUG_ON()'s in __skb_pull, or something
else.

What I meant is that you fixed this very instance of the bug, and I'm
pointing out that a similar one might hit us again.

>
> 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))
>
>
>
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@xxxxxxxxxxxxxxxxx
> For more options, visit https://groups.google.com/d/optout.