Skip to content

Commit 4063384

Browse files
edumazetdavem330
authored andcommitted
net: add vlan_get_protocol_and_depth() helper
Before blamed commit, pskb_may_pull() was used instead of skb_header_pointer() in __vlan_get_protocol() and friends. Few callers depended on skb->head being populated with MAC header, syzbot caught one of them (skb_mac_gso_segment()) Add vlan_get_protocol_and_depth() to make the intent clearer and use it where sensible. This is a more generic fix than commit e9d3f80 ("net/af_packet: make sure to pull mac header") which was dealing with a similar issue. kernel BUG at include/linux/skbuff.h:2655 ! invalid opcode: 0000 [#1] SMP KASAN CPU: 0 PID: 1441 Comm: syz-executor199 Not tainted 6.1.24-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/14/2023 RIP: 0010:__skb_pull include/linux/skbuff.h:2655 [inline] RIP: 0010:skb_mac_gso_segment+0x68f/0x6a0 net/core/gro.c:136 Code: fd 48 8b 5c 24 10 44 89 6b 70 48 c7 c7 c0 ae 0d 86 44 89 e6 e8 a1 91 d0 00 48 c7 c7 00 af 0d 86 48 89 de 31 d2 e8 d1 4a e9 ff <0f> 0b 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 RSP: 0018:ffffc90001bd7520 EFLAGS: 00010286 RAX: ffffffff8469736a RBX: ffff88810f31dac0 RCX: ffff888115a18b00 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000 RBP: ffffc90001bd75e8 R08: ffffffff84697183 R09: fffff5200037adf9 R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000012 R13: 000000000000fee5 R14: 0000000000005865 R15: 000000000000fed7 FS: 000055555633f300(0000) GS:ffff8881f6a00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020000000 CR3: 0000000116fea000 CR4: 00000000003506f0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> [<ffffffff847018dd>] __skb_gso_segment+0x32d/0x4c0 net/core/dev.c:3419 [<ffffffff8470398a>] skb_gso_segment include/linux/netdevice.h:4819 [inline] [<ffffffff8470398a>] validate_xmit_skb+0x3aa/0xee0 net/core/dev.c:3725 [<ffffffff84707042>] __dev_queue_xmit+0x1332/0x3300 net/core/dev.c:4313 [<ffffffff851a9ec7>] dev_queue_xmit+0x17/0x20 include/linux/netdevice.h:3029 [<ffffffff851b4a82>] packet_snd net/packet/af_packet.c:3111 [inline] [<ffffffff851b4a82>] packet_sendmsg+0x49d2/0x6470 net/packet/af_packet.c:3142 [<ffffffff84669a12>] sock_sendmsg_nosec net/socket.c:716 [inline] [<ffffffff84669a12>] sock_sendmsg net/socket.c:736 [inline] [<ffffffff84669a12>] __sys_sendto+0x472/0x5f0 net/socket.c:2139 [<ffffffff84669c75>] __do_sys_sendto net/socket.c:2151 [inline] [<ffffffff84669c75>] __se_sys_sendto net/socket.c:2147 [inline] [<ffffffff84669c75>] __x64_sys_sendto+0xe5/0x100 net/socket.c:2147 [<ffffffff8551d40f>] do_syscall_x64 arch/x86/entry/common.c:50 [inline] [<ffffffff8551d40f>] do_syscall_64+0x2f/0x50 arch/x86/entry/common.c:80 [<ffffffff85600087>] entry_SYSCALL_64_after_hwframe+0x63/0xcd Fixes: 469aced ("vlan: consolidate VLAN parsing code and limit max parsing depth") Reported-by: syzbot <syzkaller@googlegroups.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Toke Høiland-Jørgensen <toke@redhat.com> Cc: Willem de Bruijn <willemb@google.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent 43fb622 commit 4063384

5 files changed

Lines changed: 23 additions & 8 deletions

File tree

drivers/net/tap.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -742,7 +742,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
742742

743743
/* Move network header to the right position for VLAN tagged packets */
744744
if (eth_type_vlan(skb->protocol) &&
745-
__vlan_get_protocol(skb, skb->protocol, &depth) != 0)
745+
vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
746746
skb_set_network_header(skb, depth);
747747

748748
/* copy skb_ubuf_info for callback when skb has no error */
@@ -1197,7 +1197,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
11971197

11981198
/* Move network header to the right position for VLAN tagged packets */
11991199
if (eth_type_vlan(skb->protocol) &&
1200-
__vlan_get_protocol(skb, skb->protocol, &depth) != 0)
1200+
vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
12011201
skb_set_network_header(skb, depth);
12021202

12031203
rcu_read_lock();

include/linux/if_vlan.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,23 @@ static inline __be16 vlan_get_protocol(const struct sk_buff *skb)
637637
return __vlan_get_protocol(skb, skb->protocol, NULL);
638638
}
639639

640+
/* This version of __vlan_get_protocol() also pulls mac header in skb->head */
641+
static inline __be16 vlan_get_protocol_and_depth(struct sk_buff *skb,
642+
__be16 type, int *depth)
643+
{
644+
int maclen;
645+
646+
type = __vlan_get_protocol(skb, type, &maclen);
647+
648+
if (type) {
649+
if (!pskb_may_pull(skb, maclen))
650+
type = 0;
651+
else if (depth)
652+
*depth = maclen;
653+
}
654+
return type;
655+
}
656+
640657
/* A getter for the SKB protocol field which will handle VLAN tags consistently
641658
* whether VLAN acceleration is enabled or not.
642659
*/

net/bridge/br_forward.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb
4242
eth_type_vlan(skb->protocol)) {
4343
int depth;
4444

45-
if (!__vlan_get_protocol(skb, skb->protocol, &depth))
45+
if (!vlan_get_protocol_and_depth(skb, skb->protocol, &depth))
4646
goto drop;
4747

4848
skb_set_network_header(skb, depth);

net/core/dev.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3335,7 +3335,7 @@ __be16 skb_network_protocol(struct sk_buff *skb, int *depth)
33353335
type = eth->h_proto;
33363336
}
33373337

3338-
return __vlan_get_protocol(skb, type, depth);
3338+
return vlan_get_protocol_and_depth(skb, type, depth);
33393339
}
33403340

33413341
/* openvswitch calls this on rx path, so we need a different check.

net/packet/af_packet.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1934,10 +1934,8 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
19341934
/* Move network header to the right position for VLAN tagged packets */
19351935
if (likely(skb->dev->type == ARPHRD_ETHER) &&
19361936
eth_type_vlan(skb->protocol) &&
1937-
__vlan_get_protocol(skb, skb->protocol, &depth) != 0) {
1938-
if (pskb_may_pull(skb, depth))
1939-
skb_set_network_header(skb, depth);
1940-
}
1937+
vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
1938+
skb_set_network_header(skb, depth);
19411939

19421940
skb_probe_transport_header(skb);
19431941
}

0 commit comments

Comments
 (0)