^
ffff8801cf0d7780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8801cf0d7800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
^
ffff8801ce17f400: fb fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc
ffff8801ce17f480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
This is an 8 byte packet with a virtio_net_hdr describing it as
VIRTIO_NET_HDR_GSO_TCPV4, but with protocol ETH_P_NSH. That
matches the occurrence of nsh_gso_segment in the stack trace.
This pulls the struct nshhdr of 8B, passing a packet with skb->len 0
to skb_mac_gso_segment. That is the only GRO function that
unconditionally calls _skb_pull without first checking pskb_may_pull.
Adding that check does catch this:
+ if (unlikely(!pskb_may_pull(skb, vlan_depth)))
+ return ERR_PTR(-EINVAL);
This is an 8 byte packet with a virtio_net_hdr describing it as
VIRTIO_NET_HDR_GSO_TCPV4, but with protocol ETH_P_NSH. That
matches the occurrence of nsh_gso_segment in the stack trace.
This pulls the struct nshhdr of 8B, passing a packet with skb->len 0
to skb_mac_gso_segment. That is the only GRO function that
unconditionally calls _skb_pull without first checking pskb_may_pull.
Adding that check does catch this:
+ if (unlikely(!pskb_may_pull(skb, vlan_depth)))
+ return ERR_PTR(-EINVAL);
vlan_depth is 65528 when entering skb_mac_gso_segment due to
overflow of skb->mac_len in nsh_gso_segment. For this packet, the
outer mac len is zero, so skb->data == skb_mac_header(skb). Then
skb_reset_network_header(skb);
[...]
__skb_pull(skb, nsh_len);
skb_reset_mac_header(skb); // now mac hdr starts nsh_len == 8B
after net hdr
skb_reset_mac_len(skb); // mac len = net hdr - mac hdr ==
(u16) -8 == 65528
[..]
skb_mac_gso_segment(skb, ..)
Setting skb->mac_len to 0, similar to mpls_gs_segment,
is sufficient if the encapsulated packet is not ETH_P_TEB.
If the packet is encapsulated at L2, __skb_pull(skb, vlan_depth)
has to pull the inner mac header before passing to l3 handlers like
inet_gso_segment.
If that header includes VLAN tags, skb_network_protocol will
parse then and update the mac length in vlan_depth. So
hardcoding to ETH_HLEN should be fine:
This is an 8 byte packet with a virtio_net_hdr describing it as
VIRTIO_NET_HDR_GSO_TCPV4, but with protocol ETH_P_NSH. That
matches the occurrence of nsh_gso_segment in the stack trace.
This pulls the struct nshhdr of 8B, passing a packet with skb->len 0
to skb_mac_gso_segment. That is the only GRO function that
unconditionally calls _skb_pull without first checking pskb_may_pull.
Adding that check does catch this:
+ if (unlikely(!pskb_may_pull(skb, vlan_depth)))
+ return ERR_PTR(-EINVAL);
vlan_depth is 65528 when entering skb_mac_gso_segment due to
overflow of skb->mac_len in nsh_gso_segment. For this packet, the
outer mac len is zero, so skb->data == skb_mac_header(skb). Then
skb_reset_network_header(skb);
[...]
__skb_pull(skb, nsh_len);
skb_reset_mac_header(skb); // now mac hdr starts nsh_len == 8B
after net hdr
skb_reset_mac_len(skb); // mac len = net hdr - mac hdr ==
(u16) -8 == 65528
[..]
skb_mac_gso_segment(skb, ..)
Setting skb->mac_len to 0, similar to mpls_gs_segment,
is sufficient if the encapsulated packet is not ETH_P_TEB.
If the packet is encapsulated at L2, __skb_pull(skb, vlan_depth)
has to pull the inner mac header before passing to l3 handlers like
inet_gso_segment.
If that header includes VLAN tags, skb_network_protocol will
parse then and update the mac length in vlan_depth. So
hardcoding to ETH_HLEN should be fine:
Well, htons(ETH_P_TEB)
but after this patch the reproducer no longer triggers.
ipv6_gso_segment will catch the zero length inner packet with
pskb_may_pull.
skb_mac_gso_segment itself does not strictly need a
pskb_may_pull, as skb_network_protocol calls pskb_may_pull
when processing ETH_P_TEB and VLAN headers.
Still, adding this would help catch bugs in callers sooner:
- if (unlikely(!type))
+ if (unlikely(!type || !pskb_may_pull(skb, vlan_depth)))
return ERR_PTR(-EINVAL);
Sorry for the delayed reply, I'm working through a pile of stuff after
being off.
On Sun, 8 Jul 2018 18:58:14 -0400, Willem de Bruijn wrote:
quoted hunk
Setting skb->mac_len to 0, similar to mpls_gs_segment,
is sufficient if the encapsulated packet is not ETH_P_TEB.
If the packet is encapsulated at L2, __skb_pull(skb, vlan_depth)
has to pull the inner mac header before passing to l3 handlers like
inet_gso_segment.
If that header includes VLAN tags, skb_network_protocol will
parse then and update the mac length in vlan_depth. So
hardcoding to ETH_HLEN should be fine:
I agree. I think my original intention was to set mac_len to 0. Which
is obviously not done by calling skb_reset_mac_len...
Strangely, skb_network_protocol does not set *depth to ETH_HLEN if it
is 0 and the type is ETH_P_TEB, which is something I would expect it to
do. Thus we indeed have to differentiate between the two cases before
calling skb_mac_gso_segment.
Willem, will you send the patch formally (with the htons fix)? Thanks a
lot for the analysis and the patch!
Jiri
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: 2018-07-11 16:09:15
On Wed, Jul 11, 2018 at 6:07 AM Jiri Benc [off-list ref] wrote:
Sorry for the delayed reply, I'm working through a pile of stuff after
being off.
On Sun, 8 Jul 2018 18:58:14 -0400, Willem de Bruijn wrote:
quoted
Setting skb->mac_len to 0, similar to mpls_gs_segment,
is sufficient if the encapsulated packet is not ETH_P_TEB.
If the packet is encapsulated at L2, __skb_pull(skb, vlan_depth)
has to pull the inner mac header before passing to l3 handlers like
inet_gso_segment.
If that header includes VLAN tags, skb_network_protocol will
parse then and update the mac length in vlan_depth. So
hardcoding to ETH_HLEN should be fine:
I agree. I think my original intention was to set mac_len to 0. Which
is obviously not done by calling skb_reset_mac_len...
Strangely, skb_network_protocol does not set *depth to ETH_HLEN if it
is 0 and the type is ETH_P_TEB, which is something I would expect it to
do. Thus we indeed have to differentiate between the two cases before
calling skb_mac_gso_segment.
Willem, will you send the patch formally (with the htons fix)? Thanks a
lot for the analysis and the patch!