| Message ID | 20231126151652.372783-1-syoshida@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit() | expand |
Shigeru Yoshida wrote: > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > true. For example, applications can create a malformed packet that causes > this problem with PF_PACKET. It may fail because because pskb_inet_may_pull does not account for tunnel->hlen. Is that what you are referring to with malformed packet? Can you eloborate a bit on in which way the packet has to be malformed to reach this? FYI: I had a quick look at the IPv6 equivalent code. ip6gre_tunnel_xmit is sufficiently different. It makes sense that this is an IPv4 only patch. > This patch fixes the problem by dropping skb and returning from the > function if skb_pull() fails. > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > --- > net/ipv4/ip_gre.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index 22a26d1d29a0..95efa97cb84b 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > * to gre header. > */ > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) > + goto free_skb; > skb_reset_mac_header(skb); > > if (skb->ip_summed == CHECKSUM_PARTIAL && > -- > 2.41.0 >
On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote: > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > true. For example, applications can create a malformed packet that causes > this problem with PF_PACKET. > > This patch fixes the problem by dropping skb and returning from the > function if skb_pull() fails. > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > --- > net/ipv4/ip_gre.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index 22a26d1d29a0..95efa97cb84b 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > * to gre header. > */ > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) > + goto free_skb; > skb_reset_mac_header(skb); > > if (skb->ip_summed == CHECKSUM_PARTIAL && > -- I have syszbot reports with an actual repro for this one. I do not think your patch is correct, something should be fixed earlier (before we hit this point)
On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote: > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote: > > > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > > true. For example, applications can create a malformed packet that causes > > this problem with PF_PACKET. > > > > This patch fixes the problem by dropping skb and returning from the > > function if skb_pull() fails. > > > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > > --- > > net/ipv4/ip_gre.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > > index 22a26d1d29a0..95efa97cb84b 100644 > > --- a/net/ipv4/ip_gre.c > > +++ b/net/ipv4/ip_gre.c > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > > /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > > * to gre header. > > */ > > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > > + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) > > + goto free_skb; > > skb_reset_mac_header(skb); > > > > if (skb->ip_summed == CHECKSUM_PARTIAL && > > -- > > > I have syszbot reports with an actual repro for this one. Could you please share them? I could not find easily the reports in https://syzkaller.appspot.com/upstream Thanks, Paolo
On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <pabeni@redhat.com> wrote: > > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote: > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote: > > > > > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > > > true. For example, applications can create a malformed packet that causes > > > this problem with PF_PACKET. > > > > > > This patch fixes the problem by dropping skb and returning from the > > > function if skb_pull() fails. > > > > > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > > > --- > > > net/ipv4/ip_gre.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > > > index 22a26d1d29a0..95efa97cb84b 100644 > > > --- a/net/ipv4/ip_gre.c > > > +++ b/net/ipv4/ip_gre.c > > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > > > /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > > > * to gre header. > > > */ > > > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > > > + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) > > > + goto free_skb; > > > skb_reset_mac_header(skb); > > > > > > if (skb->ip_summed == CHECKSUM_PARTIAL && > > > -- > > > > > > I have syszbot reports with an actual repro for this one. > > Could you please share them? I could not find easily the reports in > https://syzkaller.appspot.com/upstream Stack trace looks like the following: skbuff: skb_under_panic: text:ffffffff845f50a0 len:920 put:20 head:ffff888171931000 data:ffff888171930ff8 tail:0x390 end:0x680 dev:gre4 ------------[ cut here ]------------ kernel BUG at net/core/skbuff.c:120 ! invalid opcode: 0000 [#1] PREEMPT SMP KASAN CPU: 0 PID: 12705 Comm: kworker/0:0 Not tainted 6.1.43-syzkaller-00022-g8f46c3493178 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023 Workqueue: mld mld_ifc_work RIP: 0010:skb_panic net/core/skbuff.c:120 [inline] RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130 Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45 d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41 RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286 RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000 RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000 RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390 R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8 FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> skb_push+0xf3/0x120 net/core/skbuff.c:2181 iptunnel_xmit+0x2d0/0x940 net/ipv4/ip_tunnel_core.c:67 ip_tunnel_xmit+0x218f/0x2ae0 net/ipv4/ip_tunnel.c:813 __gre_xmit net/ipv4/ip_gre.c:469 [inline] ipgre_xmit+0x7ac/0xaa0 net/ipv4/ip_gre.c:661 __netdev_start_xmit include/linux/netdevice.h:4908 [inline] netdev_start_xmit include/linux/netdevice.h:4922 [inline] xmit_one net/core/dev.c:3602 [inline] dev_hard_start_xmit+0x1de/0x630 net/core/dev.c:3618 __dev_queue_xmit+0x18c0/0x3700 net/core/dev.c:4268 dev_queue_xmit include/linux/netdevice.h:3076 [inline] neigh_direct_output+0x17/0x20 net/core/neighbour.c:1592 neigh_output include/net/neighbour.h:552 [inline] ip6_finish_output2+0x104a/0x1820 net/ipv6/ip6_output.c:134 __ip6_finish_output net/ipv6/ip6_output.c:195 [inline] ip6_finish_output+0x5d9/0xb60 net/ipv6/ip6_output.c:206 NF_HOOK_COND include/linux/netfilter.h:294 [inline] ip6_output+0x1f7/0x4d0 net/ipv6/ip6_output.c:227 dst_output include/net/dst.h:444 [inline] NF_HOOK include/linux/netfilter.h:305 [inline] mld_sendpack+0x803/0xe40 net/ipv6/mcast.c:1820 mld_send_cr net/ipv6/mcast.c:2121 [inline] mld_ifc_work+0x7dc/0xba0 net/ipv6/mcast.c:2653 process_one_work+0x73d/0xcb0 kernel/workqueue.c:2299 worker_thread+0xa60/0x1260 kernel/workqueue.c:2446 kthread+0x26d/0x300 kernel/kthread.c:376 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306 </TASK> Modules linked in: ---[ end trace 0000000000000000 ]--- RIP: 0010:skb_panic net/core/skbuff.c:120 [inline] RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130 Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45 d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41 RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286 RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000 RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000 RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390 R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8 FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
On Tue, Nov 28, 2023 at 5:05 PM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <pabeni@redhat.com> wrote: > > > > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote: > > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote: > > > > > > > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > > > > true. For example, applications can create a malformed packet that causes > > > > this problem with PF_PACKET. > > > > > > > > This patch fixes the problem by dropping skb and returning from the > > > > function if skb_pull() fails. > > > > > > > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") > > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > > > > --- > > > > net/ipv4/ip_gre.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > > > > index 22a26d1d29a0..95efa97cb84b 100644 > > > > --- a/net/ipv4/ip_gre.c > > > > +++ b/net/ipv4/ip_gre.c > > > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > > > > /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > > > > * to gre header. > > > > */ > > > > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > > > > + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) > > > > + goto free_skb; > > > > skb_reset_mac_header(skb); > > > > > > > > if (skb->ip_summed == CHECKSUM_PARTIAL && > > > > -- > > > > > > > > > I have syszbot reports with an actual repro for this one. > > > > Could you please share them? I could not find easily the reports in > > https://syzkaller.appspot.com/upstream > > Stack trace looks like the following: > > skbuff: skb_under_panic: text:ffffffff845f50a0 len:920 put:20 > head:ffff888171931000 data:ffff888171930ff8 tail:0x390 end:0x680 > dev:gre4 > ------------[ cut here ]------------ > kernel BUG at net/core/skbuff.c:120 ! > invalid opcode: 0000 [#1] PREEMPT SMP KASAN > CPU: 0 PID: 12705 Comm: kworker/0:0 Not tainted > 6.1.43-syzkaller-00022-g8f46c3493178 #0 > Hardware name: Google Google Compute Engine/Google Compute Engine, > BIOS Google 10/09/2023 > Workqueue: mld mld_ifc_work > RIP: 0010:skb_panic net/core/skbuff.c:120 [inline] > RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130 > Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45 > d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b > 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41 > RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286 > RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000 > RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000 > RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390 > R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8 > FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > Call Trace: > <TASK> > skb_push+0xf3/0x120 net/core/skbuff.c:2181 > iptunnel_xmit+0x2d0/0x940 net/ipv4/ip_tunnel_core.c:67 > ip_tunnel_xmit+0x218f/0x2ae0 net/ipv4/ip_tunnel.c:813 > __gre_xmit net/ipv4/ip_gre.c:469 [inline] > ipgre_xmit+0x7ac/0xaa0 net/ipv4/ip_gre.c:661 > __netdev_start_xmit include/linux/netdevice.h:4908 [inline] > netdev_start_xmit include/linux/netdevice.h:4922 [inline] > xmit_one net/core/dev.c:3602 [inline] > dev_hard_start_xmit+0x1de/0x630 net/core/dev.c:3618 > __dev_queue_xmit+0x18c0/0x3700 net/core/dev.c:4268 > dev_queue_xmit include/linux/netdevice.h:3076 [inline] > neigh_direct_output+0x17/0x20 net/core/neighbour.c:1592 > neigh_output include/net/neighbour.h:552 [inline] > ip6_finish_output2+0x104a/0x1820 net/ipv6/ip6_output.c:134 > __ip6_finish_output net/ipv6/ip6_output.c:195 [inline] > ip6_finish_output+0x5d9/0xb60 net/ipv6/ip6_output.c:206 > NF_HOOK_COND include/linux/netfilter.h:294 [inline] > ip6_output+0x1f7/0x4d0 net/ipv6/ip6_output.c:227 > dst_output include/net/dst.h:444 [inline] > NF_HOOK include/linux/netfilter.h:305 [inline] > mld_sendpack+0x803/0xe40 net/ipv6/mcast.c:1820 > mld_send_cr net/ipv6/mcast.c:2121 [inline] > mld_ifc_work+0x7dc/0xba0 net/ipv6/mcast.c:2653 > process_one_work+0x73d/0xcb0 kernel/workqueue.c:2299 > worker_thread+0xa60/0x1260 kernel/workqueue.c:2446 > kthread+0x26d/0x300 kernel/kthread.c:376 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306 > </TASK> > Modules linked in: > ---[ end trace 0000000000000000 ]--- > RIP: 0010:skb_panic net/core/skbuff.c:120 [inline] > RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130 > Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45 > d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b > 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41 > RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286 > RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000 > RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000 > RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390 > R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8 > FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0 > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 It looks like the repro I had was fixed by "#syz fix: bonding: stop the device in bond_setup_by_slave()" (I am not sure, I have to double check) # See https://goo.gl/kgGztJ for information about syzkaller reproducers. #{"repeat":true,"procs":1,"slowdown":1,"sandbox":"","sandbox_arg":0,"close_fds":false} socket$inet(0x2, 0x2, 0x0) r0 = socket(0x200000000000011, 0x4000000000080002, 0x0) r1 = socket$netlink(0x10, 0x3, 0x0) r2 = socket$netlink(0x10, 0x3, 0x0) r3 = socket(0x10, 0x803, 0x0) sendmsg$NL80211_CMD_CRIT_PROTOCOL_START(r3, &(0x7f0000000580)={0x0, 0x0, &(0x7f0000000540)={0x0, 0x1c}}, 0x0) getsockname$packet(r3, &(0x7f0000000600)={0x11, 0x0, <r4=>0x0, 0x1, 0x0, 0x6, @broadcast}, &(0x7f0000000080)=0x14) sendmsg$nl_route(r2, &(0x7f0000000040)={0x0, 0x0, &(0x7f0000000000)={&(0x7f0000000340)=ANY=[@ANYBLOB="3c0000001000850600002000fe612233ca000800", @ANYRES32=r4, @ANYBLOB="2377f29e252155b21c0012000c000100626f6e64000000000c000200080001000134e7307075a7cc6d2dba6e4dce25f18968dd3d6f77199cd06d7a4cfcdc99dcfd5ec3f3e3d98be8a8bac2dcc414b58dda48b3ea35411d5b112c26f31b352982f55be446b3dd47e435954252213828ba98a1bc363278f8bd13ad746bb8edad619162f5d1892e9fa42e4fe2b60f5fe2bb963f08d6696820ade9cff2b2deb91ce5657168a90dc5230e33b8c26cd925c31366a2ae339f12ba8966be1439cec635b08c0a97490b133a5b7360b59347833fc95a7bf3dc9bc64741de1a6e83c9bdfdfd0baabec981099bb3dbd64a7e7979cfb7935affbcda49190b7ec9bc1e89d6ccedec20f91b571e6fc049ba82821b26ca4f85f4b03f70b176b43de915bec76e405bce49a4b46ec745b51f36282916b77d7f913a6afd6813df2c"], 0x3c}}, 0x0) sendmsg$nl_route(r1, &(0x7f0000000240)={0x0, 0x0, &(0x7f0000000180)={&(0x7f0000000780)=@newlink={0x58, 0x10, 0xffffff1f, 0x0, 0x0, {0x0, 0x0, 0x0, 0x0, 0x800}, [@IFLA_LINKINFO={0x28, 0x12, 0x0, 0x1, @gre={{0x8}, {0x1c, 0x2, 0x0, 0x1, [@IFLA_GRE_LOCAL={0x8, 0x6, @broadcast}, @IFLA_GRE_TOS={0x5, 0x9, 0x8}, @IFLA_GRE_OKEY={0x8, 0x5, 0x8}]}}}, @IFLA_MASTER={0x8, 0xa, r4}, @IFLA_GROUP={0x8, 0x1b, 0x8000}]}, 0x58}}, 0x4004000) bind$packet(r0, &(0x7f00000000c0)={0x11, 0x0, r4, 0x1, 0x0, 0x6, @remote}, 0x14) sendmsg$nl_route(r0, &(0x7f0000000300)={0x0, 0x0, &(0x7f00000002c0)={0x0}}, 0x0)
On Tue, Nov 28, 2023 at 5:13 PM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Nov 28, 2023 at 5:05 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <pabeni@redhat.com> wrote: > > > > > > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote: > > > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote: > > > > > > > > > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > > > > > true. For example, applications can create a malformed packet that causes > > > > > this problem with PF_PACKET. > > > > > > > > > > This patch fixes the problem by dropping skb and returning from the > > > > > function if skb_pull() fails. > > > > > > > > > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") > > > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> > > > > > --- > > > > > net/ipv4/ip_gre.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > > > > > index 22a26d1d29a0..95efa97cb84b 100644 > > > > > --- a/net/ipv4/ip_gre.c > > > > > +++ b/net/ipv4/ip_gre.c > > > > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > > > > > /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > > > > > * to gre header. > > > > > */ > > > > > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > > > > > + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) > > > > > + goto free_skb; > > > > > skb_reset_mac_header(skb); > > > > > > > > > > if (skb->ip_summed == CHECKSUM_PARTIAL && > > > > > -- > > > > > > > > > > > > I have syszbot reports with an actual repro for this one. > > > > > > Could you please share them? I could not find easily the reports in > > > https://syzkaller.appspot.com/upstream > > > > Stack trace looks like the following: > > > > skbuff: skb_under_panic: text:ffffffff845f50a0 len:920 put:20 > > head:ffff888171931000 data:ffff888171930ff8 tail:0x390 end:0x680 > > dev:gre4 > > ------------[ cut here ]------------ > > kernel BUG at net/core/skbuff.c:120 ! > > invalid opcode: 0000 [#1] PREEMPT SMP KASAN > > CPU: 0 PID: 12705 Comm: kworker/0:0 Not tainted > > 6.1.43-syzkaller-00022-g8f46c3493178 #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, > > BIOS Google 10/09/2023 > > Workqueue: mld mld_ifc_work > > RIP: 0010:skb_panic net/core/skbuff.c:120 [inline] > > RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130 > > Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45 > > d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b > > 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41 > > RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286 > > RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000 > > RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000 > > RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad > > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390 > > R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8 > > FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > Call Trace: > > <TASK> > > skb_push+0xf3/0x120 net/core/skbuff.c:2181 > > iptunnel_xmit+0x2d0/0x940 net/ipv4/ip_tunnel_core.c:67 > > ip_tunnel_xmit+0x218f/0x2ae0 net/ipv4/ip_tunnel.c:813 > > __gre_xmit net/ipv4/ip_gre.c:469 [inline] > > ipgre_xmit+0x7ac/0xaa0 net/ipv4/ip_gre.c:661 > > __netdev_start_xmit include/linux/netdevice.h:4908 [inline] > > netdev_start_xmit include/linux/netdevice.h:4922 [inline] > > xmit_one net/core/dev.c:3602 [inline] > > dev_hard_start_xmit+0x1de/0x630 net/core/dev.c:3618 > > __dev_queue_xmit+0x18c0/0x3700 net/core/dev.c:4268 > > dev_queue_xmit include/linux/netdevice.h:3076 [inline] > > neigh_direct_output+0x17/0x20 net/core/neighbour.c:1592 > > neigh_output include/net/neighbour.h:552 [inline] > > ip6_finish_output2+0x104a/0x1820 net/ipv6/ip6_output.c:134 > > __ip6_finish_output net/ipv6/ip6_output.c:195 [inline] > > ip6_finish_output+0x5d9/0xb60 net/ipv6/ip6_output.c:206 > > NF_HOOK_COND include/linux/netfilter.h:294 [inline] > > ip6_output+0x1f7/0x4d0 net/ipv6/ip6_output.c:227 > > dst_output include/net/dst.h:444 [inline] > > NF_HOOK include/linux/netfilter.h:305 [inline] > > mld_sendpack+0x803/0xe40 net/ipv6/mcast.c:1820 > > mld_send_cr net/ipv6/mcast.c:2121 [inline] > > mld_ifc_work+0x7dc/0xba0 net/ipv6/mcast.c:2653 > > process_one_work+0x73d/0xcb0 kernel/workqueue.c:2299 > > worker_thread+0xa60/0x1260 kernel/workqueue.c:2446 > > kthread+0x26d/0x300 kernel/kthread.c:376 > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306 > > </TASK> > > Modules linked in: > > ---[ end trace 0000000000000000 ]--- > > RIP: 0010:skb_panic net/core/skbuff.c:120 [inline] > > RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130 > > Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45 > > d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b > > 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41 > > RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286 > > RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000 > > RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000 > > RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad > > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390 > > R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8 > > FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0 > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > > It looks like the repro I had was fixed by "#syz fix: bonding: stop > the device in bond_setup_by_slave()" > > (I am not sure, I have to double check) This is the syzbot link : https://syzkaller.appspot.com/bug?extid=802070ddd12342f07fce > > # See https://goo.gl/kgGztJ for information about syzkaller reproducers. > #{"repeat":true,"procs":1,"slowdown":1,"sandbox":"","sandbox_arg":0,"close_fds":false} > socket$inet(0x2, 0x2, 0x0) > r0 = socket(0x200000000000011, 0x4000000000080002, 0x0) > r1 = socket$netlink(0x10, 0x3, 0x0) > r2 = socket$netlink(0x10, 0x3, 0x0) > r3 = socket(0x10, 0x803, 0x0) > sendmsg$NL80211_CMD_CRIT_PROTOCOL_START(r3, &(0x7f0000000580)={0x0, > 0x0, &(0x7f0000000540)={0x0, 0x1c}}, 0x0) > getsockname$packet(r3, &(0x7f0000000600)={0x11, 0x0, <r4=>0x0, 0x1, > 0x0, 0x6, @broadcast}, &(0x7f0000000080)=0x14) > sendmsg$nl_route(r2, &(0x7f0000000040)={0x0, 0x0, > &(0x7f0000000000)={&(0x7f0000000340)=ANY=[@ANYBLOB="3c0000001000850600002000fe612233ca000800", > @ANYRES32=r4, @ANYBLOB="2377f29e252155b21c0012000c000100626f6e64000000000c000200080001000134e7307075a7cc6d2dba6e4dce25f18968dd3d6f77199cd06d7a4cfcdc99dcfd5ec3f3e3d98be8a8bac2dcc414b58dda48b3ea35411d5b112c26f31b352982f55be446b3dd47e435954252213828ba98a1bc363278f8bd13ad746bb8edad619162f5d1892e9fa42e4fe2b60f5fe2bb963f08d6696820ade9cff2b2deb91ce5657168a90dc5230e33b8c26cd925c31366a2ae339f12ba8966be1439cec635b08c0a97490b133a5b7360b59347833fc95a7bf3dc9bc64741de1a6e83c9bdfdfd0baabec981099bb3dbd64a7e7979cfb7935affbcda49190b7ec9bc1e89d6ccedec20f91b571e6fc049ba82821b26ca4f85f4b03f70b176b43de915bec76e405bce49a4b46ec745b51f36282916b77d7f913a6afd6813df2c"], > 0x3c}}, 0x0) > sendmsg$nl_route(r1, &(0x7f0000000240)={0x0, 0x0, > &(0x7f0000000180)={&(0x7f0000000780)=@newlink={0x58, 0x10, 0xffffff1f, > 0x0, 0x0, {0x0, 0x0, 0x0, 0x0, 0x800}, [@IFLA_LINKINFO={0x28, 0x12, > 0x0, 0x1, @gre={{0x8}, {0x1c, 0x2, 0x0, 0x1, [@IFLA_GRE_LOCAL={0x8, > 0x6, @broadcast}, @IFLA_GRE_TOS={0x5, 0x9, 0x8}, @IFLA_GRE_OKEY={0x8, > 0x5, 0x8}]}}}, @IFLA_MASTER={0x8, 0xa, r4}, @IFLA_GROUP={0x8, 0x1b, > 0x8000}]}, 0x58}}, 0x4004000) > bind$packet(r0, &(0x7f00000000c0)={0x11, 0x0, r4, 0x1, 0x0, 0x6, @remote}, 0x14) > sendmsg$nl_route(r0, &(0x7f0000000300)={0x0, 0x0, &(0x7f00000002c0)={0x0}}, 0x0)
On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote: > Shigeru Yoshida wrote: >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns >> true. For example, applications can create a malformed packet that causes >> this problem with PF_PACKET. > > It may fail because because pskb_inet_may_pull does not account for > tunnel->hlen. > > Is that what you are referring to with malformed packet? Can you > eloborate a bit on in which way the packet has to be malformed to > reach this? Thank you very much for your prompt feedback. Actually, I found this problem by running syzkaller. Syzkaller reported the following uninit-value issue (I think the root cause of this issue is the same as the one Eric mentioned): ===================================================== BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline] BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662 __gre_xmit net/ipv4/ip_gre.c:469 [inline] ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662 __netdev_start_xmit include/linux/netdevice.h:4918 [inline] netdev_start_xmit include/linux/netdevice.h:4932 [inline] xmit_one net/core/dev.c:3543 [inline] dev_hard_start_xmit+0x24a/0xa10 net/core/dev.c:3559 __dev_queue_xmit+0x32f6/0x50e0 net/core/dev.c:4344 dev_queue_xmit include/linux/netdevice.h:3112 [inline] packet_xmit+0x8f/0x6b0 net/packet/af_packet.c:276 packet_snd net/packet/af_packet.c:3087 [inline] packet_sendmsg+0x8c24/0x9aa0 net/packet/af_packet.c:3119 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg net/socket.c:745 [inline] __sys_sendto+0x717/0xa00 net/socket.c:2194 __do_sys_sendto net/socket.c:2206 [inline] __se_sys_sendto net/socket.c:2202 [inline] __x64_sys_sendto+0x130/0x200 net/socket.c:2202 do_syscall_x64 arch/x86/entry/common.c:51 [inline] do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82 entry_SYSCALL_64_after_hwframe+0x63/0x6b Uninit was stored to memory at: __gre_xmit net/ipv4/ip_gre.c:469 [inline] ipgre_xmit+0xded/0xe70 net/ipv4/ip_gre.c:662 __netdev_start_xmit include/linux/netdevice.h:4918 [inline] netdev_start_xmit include/linux/netdevice.h:4932 [inline] xmit_one net/core/dev.c:3543 [inline] dev_hard_start_xmit+0x24a/0xa10 net/core/dev.c:3559 __dev_queue_xmit+0x32f6/0x50e0 net/core/dev.c:4344 dev_queue_xmit include/linux/netdevice.h:3112 [inline] packet_xmit+0x8f/0x6b0 net/packet/af_packet.c:276 packet_snd net/packet/af_packet.c:3087 [inline] packet_sendmsg+0x8c24/0x9aa0 net/packet/af_packet.c:3119 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg net/socket.c:745 [inline] __sys_sendto+0x717/0xa00 net/socket.c:2194 __do_sys_sendto net/socket.c:2206 [inline] __se_sys_sendto net/socket.c:2202 [inline] __x64_sys_sendto+0x130/0x200 net/socket.c:2202 do_syscall_x64 arch/x86/entry/common.c:51 [inline] do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82 entry_SYSCALL_64_after_hwframe+0x63/0x6b Uninit was created at: slab_post_alloc_hook+0x103/0x9e0 mm/slab.h:768 slab_alloc_node mm/slub.c:3478 [inline] kmem_cache_alloc_node+0x5f7/0xb50 mm/slub.c:3523 kmalloc_reserve+0x13c/0x4a0 net/core/skbuff.c:560 pskb_expand_head+0x20b/0x19a0 net/core/skbuff.c:2098 __skb_cow include/linux/skbuff.h:3586 [inline] skb_cow_head include/linux/skbuff.h:3620 [inline] ipgre_xmit+0x73c/0xe70 net/ipv4/ip_gre.c:638 __netdev_start_xmit include/linux/netdevice.h:4918 [inline] netdev_start_xmit include/linux/netdevice.h:4932 [inline] xmit_one net/core/dev.c:3543 [inline] dev_hard_start_xmit+0x24a/0xa10 net/core/dev.c:3559 __dev_queue_xmit+0x32f6/0x50e0 net/core/dev.c:4344 dev_queue_xmit include/linux/netdevice.h:3112 [inline] packet_xmit+0x8f/0x6b0 net/packet/af_packet.c:276 packet_snd net/packet/af_packet.c:3087 [inline] packet_sendmsg+0x8c24/0x9aa0 net/packet/af_packet.c:3119 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg net/socket.c:745 [inline] __sys_sendto+0x717/0xa00 net/socket.c:2194 __do_sys_sendto net/socket.c:2206 [inline] __se_sys_sendto net/socket.c:2202 [inline] __x64_sys_sendto+0x130/0x200 net/socket.c:2202 do_syscall_x64 arch/x86/entry/common.c:51 [inline] do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82 entry_SYSCALL_64_after_hwframe+0x63/0x6b CPU: 1 PID: 11318 Comm: syz-executor.7 Not tainted 6.6.0-14500-g1c41041124bd #10 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014 ===================================================== The simplified version of the repro is shown below: #include <linux/if_ether.h> #include <sys/ioctl.h> #include <netinet/ether.h> #include <net/if.h> #include <sys/socket.h> #include <netinet/in.h> #include <string.h> #include <unistd.h> #include <stdio.h> #include <stdlib.h> #include <linux/if_packet.h> int main(void) { int s, s1, s2, data = 0; struct ifreq ifr; struct sockaddr_ll addr = { 0 }; unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6}; s = socket(AF_PACKET, SOCK_DGRAM, 0x300); s1 = socket(AF_PACKET, SOCK_RAW, 0x300); s2 = socket(AF_NETLINK, SOCK_RAW, 0); strcpy(ifr.ifr_name, "gre0"); ioctl(s2, SIOCGIFINDEX, &ifr); addr.sll_family = AF_PACKET; addr.sll_ifindex = ifr.ifr_ifindex; addr.sll_protocol = htons(0); addr.sll_hatype = ARPHRD_ETHER; addr.sll_pkttype = PACKET_HOST; addr.sll_halen = ETH_ALEN; memcpy(addr.sll_addr, mac_addr, ETH_ALEN); sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr)); return 0; } The repro sends a 1-byte packet that doesn't have the correct IP header. I meant this as "malformed pachet", but that might be a bit confusing, sorry. I think the cause of the uninit-value access is that ipgre_xmit() reallocates the skb with skb_cow_head() and copies only the 1-byte data, so any IP header access through `tnl_params` can cause the problem. At first I tried to modify pskb_inet_may_pull() to detect this type of packet, but I ended up doing this patch. Any advice is welcome! Thanks, Shigeru > > FYI: I had a quick look at the IPv6 equivalent code. > ip6gre_tunnel_xmit is sufficiently different. It makes sense that this > is an IPv4 only patch. > >> This patch fixes the problem by dropping skb and returning from the >> function if skb_pull() fails. >> >> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") >> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> >> --- >> net/ipv4/ip_gre.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c >> index 22a26d1d29a0..95efa97cb84b 100644 >> --- a/net/ipv4/ip_gre.c >> +++ b/net/ipv4/ip_gre.c >> @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, >> /* Pull skb since ip_tunnel_xmit() needs skb->data pointing >> * to gre header. >> */ >> - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); >> + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) >> + goto free_skb; >> skb_reset_mac_header(skb); >> >> if (skb->ip_summed == CHECKSUM_PARTIAL && >> -- >> 2.41.0 >> > >
On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote: > > On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote: > > Shigeru Yoshida wrote: > >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > >> true. For example, applications can create a malformed packet that causes > >> this problem with PF_PACKET. > > > > It may fail because because pskb_inet_may_pull does not account for > > tunnel->hlen. > > > > Is that what you are referring to with malformed packet? Can you > > eloborate a bit on in which way the packet has to be malformed to > > reach this? > > Thank you very much for your prompt feedback. > > Actually, I found this problem by running syzkaller. Syzkaller > reported the following uninit-value issue (I think the root cause of > this issue is the same as the one Eric mentioned): Yes, I also have a similar syzbot report (but no repro yet) I am releasing it right now. > > ===================================================== > BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline] > BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662 > __gre_xmit net/ipv4/ip_gre.c:469 [inline] > > The simplified version of the repro is shown below: > > #include <linux/if_ether.h> > #include <sys/ioctl.h> > #include <netinet/ether.h> > #include <net/if.h> > #include <sys/socket.h> > #include <netinet/in.h> > #include <string.h> > #include <unistd.h> > #include <stdio.h> > #include <stdlib.h> > #include <linux/if_packet.h> > > int main(void) > { > int s, s1, s2, data = 0; > struct ifreq ifr; > struct sockaddr_ll addr = { 0 }; > unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6}; > > s = socket(AF_PACKET, SOCK_DGRAM, 0x300); > s1 = socket(AF_PACKET, SOCK_RAW, 0x300); > s2 = socket(AF_NETLINK, SOCK_RAW, 0); > > strcpy(ifr.ifr_name, "gre0"); > ioctl(s2, SIOCGIFINDEX, &ifr); > > addr.sll_family = AF_PACKET; > addr.sll_ifindex = ifr.ifr_ifindex; > addr.sll_protocol = htons(0); > addr.sll_hatype = ARPHRD_ETHER; > addr.sll_pkttype = PACKET_HOST; > addr.sll_halen = ETH_ALEN; > memcpy(addr.sll_addr, mac_addr, ETH_ALEN); > > sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr)); > > return 0; > } > > The repro sends a 1-byte packet that doesn't have the correct IP > header. I meant this as "malformed pachet", but that might be a bit > confusing, sorry. > > I think the cause of the uninit-value access is that ipgre_xmit() > reallocates the skb with skb_cow_head() and copies only the 1-byte > data, so any IP header access through `tnl_params` can cause the > problem. > > At first I tried to modify pskb_inet_may_pull() to detect this type of > packet, but I ended up doing this patch. Even after your patch, __skb_pull() could call BUG() and crash. I would suggest using this fix instead. diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 22a26d1d29a09d234f18ce3b0f329e5047c0c046..5169c3c72cffe49cef613e69889d139db867ff74 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, } if (dev->header_ops) { + int pull_len = tunnel->hlen + sizeof(struct iphdr); + if (skb_cow_head(skb, 0)) goto free_skb; tnl_params = (const struct iphdr *)skb->data; - /* Pull skb since ip_tunnel_xmit() needs skb->data pointing - * to gre header. - */ - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); + if (!pskb_network_may_pull(skb, pull_len)) + goto free_skb; + + /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ + skb_pull(skb, pull_len); skb_reset_mac_header(skb); if (skb->ip_summed == CHECKSUM_PARTIAL &&
On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote: > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote: >> >> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote: >> > Shigeru Yoshida wrote: >> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns >> >> true. For example, applications can create a malformed packet that causes >> >> this problem with PF_PACKET. >> > >> > It may fail because because pskb_inet_may_pull does not account for >> > tunnel->hlen. >> > >> > Is that what you are referring to with malformed packet? Can you >> > eloborate a bit on in which way the packet has to be malformed to >> > reach this? >> >> Thank you very much for your prompt feedback. >> >> Actually, I found this problem by running syzkaller. Syzkaller >> reported the following uninit-value issue (I think the root cause of >> this issue is the same as the one Eric mentioned): > > Yes, I also have a similar syzbot report (but no repro yet) I am > releasing it right now. > >> >> ===================================================== >> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline] >> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662 >> __gre_xmit net/ipv4/ip_gre.c:469 [inline] >> > > > >> The simplified version of the repro is shown below: >> >> #include <linux/if_ether.h> >> #include <sys/ioctl.h> >> #include <netinet/ether.h> >> #include <net/if.h> >> #include <sys/socket.h> >> #include <netinet/in.h> >> #include <string.h> >> #include <unistd.h> >> #include <stdio.h> >> #include <stdlib.h> >> #include <linux/if_packet.h> >> >> int main(void) >> { >> int s, s1, s2, data = 0; >> struct ifreq ifr; >> struct sockaddr_ll addr = { 0 }; >> unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6}; >> >> s = socket(AF_PACKET, SOCK_DGRAM, 0x300); >> s1 = socket(AF_PACKET, SOCK_RAW, 0x300); >> s2 = socket(AF_NETLINK, SOCK_RAW, 0); >> >> strcpy(ifr.ifr_name, "gre0"); >> ioctl(s2, SIOCGIFINDEX, &ifr); >> >> addr.sll_family = AF_PACKET; >> addr.sll_ifindex = ifr.ifr_ifindex; >> addr.sll_protocol = htons(0); >> addr.sll_hatype = ARPHRD_ETHER; >> addr.sll_pkttype = PACKET_HOST; >> addr.sll_halen = ETH_ALEN; >> memcpy(addr.sll_addr, mac_addr, ETH_ALEN); >> >> sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr)); >> >> return 0; >> } >> >> The repro sends a 1-byte packet that doesn't have the correct IP >> header. I meant this as "malformed pachet", but that might be a bit >> confusing, sorry. >> >> I think the cause of the uninit-value access is that ipgre_xmit() >> reallocates the skb with skb_cow_head() and copies only the 1-byte >> data, so any IP header access through `tnl_params` can cause the >> problem. >> >> At first I tried to modify pskb_inet_may_pull() to detect this type of >> packet, but I ended up doing this patch. > > Even after your patch, __skb_pull() could call BUG() and crash. > > I would suggest using this fix instead. Thank you for your comment. Your patch ensures that skb_pull() can pull the required size, so it looks good to me. Also, I have tested your suggested patch with the repro and confirmed that it fixes the issue. Thanks, Shigeru > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c > index 22a26d1d29a09d234f18ce3b0f329e5047c0c046..5169c3c72cffe49cef613e69889d139db867ff74 > 100644 > --- a/net/ipv4/ip_gre.c > +++ b/net/ipv4/ip_gre.c > @@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > } > > if (dev->header_ops) { > + int pull_len = tunnel->hlen + sizeof(struct iphdr); > + > if (skb_cow_head(skb, 0)) > goto free_skb; > > tnl_params = (const struct iphdr *)skb->data; > > - /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > - * to gre header. > - */ > - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > + if (!pskb_network_may_pull(skb, pull_len)) > + goto free_skb; > + > + /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ > + skb_pull(skb, pull_len); > skb_reset_mac_header(skb); > > if (skb->ip_summed == CHECKSUM_PARTIAL && >
On Thu, Nov 30, 2023 at 3:03 PM Shigeru Yoshida <syoshida@redhat.com> wrote: > > On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote: > > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote: > >> > >> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote: > >> > Shigeru Yoshida wrote: > >> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > >> >> true. For example, applications can create a malformed packet that causes > >> >> this problem with PF_PACKET. > >> > > >> > It may fail because because pskb_inet_may_pull does not account for > >> > tunnel->hlen. > >> > > >> > Is that what you are referring to with malformed packet? Can you > >> > eloborate a bit on in which way the packet has to be malformed to > >> > reach this? > >> > >> Thank you very much for your prompt feedback. > >> > >> Actually, I found this problem by running syzkaller. Syzkaller > >> reported the following uninit-value issue (I think the root cause of > >> this issue is the same as the one Eric mentioned): > > > > Yes, I also have a similar syzbot report (but no repro yet) I am > > releasing it right now. > > > >> > >> ===================================================== > >> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline] > >> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662 > >> __gre_xmit net/ipv4/ip_gre.c:469 [inline] > >> > > > > > > > >> The simplified version of the repro is shown below: > >> > >> #include <linux/if_ether.h> > >> #include <sys/ioctl.h> > >> #include <netinet/ether.h> > >> #include <net/if.h> > >> #include <sys/socket.h> > >> #include <netinet/in.h> > >> #include <string.h> > >> #include <unistd.h> > >> #include <stdio.h> > >> #include <stdlib.h> > >> #include <linux/if_packet.h> > >> > >> int main(void) > >> { > >> int s, s1, s2, data = 0; > >> struct ifreq ifr; > >> struct sockaddr_ll addr = { 0 }; > >> unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6}; > >> > >> s = socket(AF_PACKET, SOCK_DGRAM, 0x300); > >> s1 = socket(AF_PACKET, SOCK_RAW, 0x300); > >> s2 = socket(AF_NETLINK, SOCK_RAW, 0); > >> > >> strcpy(ifr.ifr_name, "gre0"); > >> ioctl(s2, SIOCGIFINDEX, &ifr); > >> > >> addr.sll_family = AF_PACKET; > >> addr.sll_ifindex = ifr.ifr_ifindex; > >> addr.sll_protocol = htons(0); > >> addr.sll_hatype = ARPHRD_ETHER; > >> addr.sll_pkttype = PACKET_HOST; > >> addr.sll_halen = ETH_ALEN; > >> memcpy(addr.sll_addr, mac_addr, ETH_ALEN); > >> > >> sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr)); > >> > >> return 0; > >> } > >> > >> The repro sends a 1-byte packet that doesn't have the correct IP > >> header. I meant this as "malformed pachet", but that might be a bit > >> confusing, sorry. > >> > >> I think the cause of the uninit-value access is that ipgre_xmit() > >> reallocates the skb with skb_cow_head() and copies only the 1-byte > >> data, so any IP header access through `tnl_params` can cause the > >> problem. > >> > >> At first I tried to modify pskb_inet_may_pull() to detect this type of > >> packet, but I ended up doing this patch. > > > > Even after your patch, __skb_pull() could call BUG() and crash. > > > > I would suggest using this fix instead. > > Thank you for your comment. > > Your patch ensures that skb_pull() can pull the required size, so it > looks good to me. Also, I have tested your suggested patch with the > repro and confirmed that it fixes the issue. > This is great, please cook/send a V2 with this updated patch. I will add a 'Reviewed-by: Eric Dumazet <edumazet@google.com>' then. Thanks.
On Thu, 30 Nov 2023 15:05:38 +0100, Eric Dumazet wrote: > On Thu, Nov 30, 2023 at 3:03 PM Shigeru Yoshida <syoshida@redhat.com> wrote: >> >> On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote: >> > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote: >> >> >> >> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote: >> >> > Shigeru Yoshida wrote: >> >> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns >> >> >> true. For example, applications can create a malformed packet that causes >> >> >> this problem with PF_PACKET. >> >> > >> >> > It may fail because because pskb_inet_may_pull does not account for >> >> > tunnel->hlen. >> >> > >> >> > Is that what you are referring to with malformed packet? Can you >> >> > eloborate a bit on in which way the packet has to be malformed to >> >> > reach this? >> >> >> >> Thank you very much for your prompt feedback. >> >> >> >> Actually, I found this problem by running syzkaller. Syzkaller >> >> reported the following uninit-value issue (I think the root cause of >> >> this issue is the same as the one Eric mentioned): >> > >> > Yes, I also have a similar syzbot report (but no repro yet) I am >> > releasing it right now. >> > >> >> >> >> ===================================================== >> >> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline] >> >> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662 >> >> __gre_xmit net/ipv4/ip_gre.c:469 [inline] >> >> >> > >> > >> > >> >> The simplified version of the repro is shown below: >> >> >> >> #include <linux/if_ether.h> >> >> #include <sys/ioctl.h> >> >> #include <netinet/ether.h> >> >> #include <net/if.h> >> >> #include <sys/socket.h> >> >> #include <netinet/in.h> >> >> #include <string.h> >> >> #include <unistd.h> >> >> #include <stdio.h> >> >> #include <stdlib.h> >> >> #include <linux/if_packet.h> >> >> >> >> int main(void) >> >> { >> >> int s, s1, s2, data = 0; >> >> struct ifreq ifr; >> >> struct sockaddr_ll addr = { 0 }; >> >> unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6}; >> >> >> >> s = socket(AF_PACKET, SOCK_DGRAM, 0x300); >> >> s1 = socket(AF_PACKET, SOCK_RAW, 0x300); >> >> s2 = socket(AF_NETLINK, SOCK_RAW, 0); >> >> >> >> strcpy(ifr.ifr_name, "gre0"); >> >> ioctl(s2, SIOCGIFINDEX, &ifr); >> >> >> >> addr.sll_family = AF_PACKET; >> >> addr.sll_ifindex = ifr.ifr_ifindex; >> >> addr.sll_protocol = htons(0); >> >> addr.sll_hatype = ARPHRD_ETHER; >> >> addr.sll_pkttype = PACKET_HOST; >> >> addr.sll_halen = ETH_ALEN; >> >> memcpy(addr.sll_addr, mac_addr, ETH_ALEN); >> >> >> >> sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr)); >> >> >> >> return 0; >> >> } >> >> >> >> The repro sends a 1-byte packet that doesn't have the correct IP >> >> header. I meant this as "malformed pachet", but that might be a bit >> >> confusing, sorry. >> >> >> >> I think the cause of the uninit-value access is that ipgre_xmit() >> >> reallocates the skb with skb_cow_head() and copies only the 1-byte >> >> data, so any IP header access through `tnl_params` can cause the >> >> problem. >> >> >> >> At first I tried to modify pskb_inet_may_pull() to detect this type of >> >> packet, but I ended up doing this patch. >> > >> > Even after your patch, __skb_pull() could call BUG() and crash. >> > >> > I would suggest using this fix instead. >> >> Thank you for your comment. >> >> Your patch ensures that skb_pull() can pull the required size, so it >> looks good to me. Also, I have tested your suggested patch with the >> repro and confirmed that it fixes the issue. >> > > This is great, please cook/send a V2 with this updated patch. > > I will add a 'Reviewed-by: Eric Dumazet <edumazet@google.com>' then. Thanks, Eric! I'll send a v2 patch soon. Shigeru > > Thanks. >
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 22a26d1d29a0..95efa97cb84b 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, /* Pull skb since ip_tunnel_xmit() needs skb->data pointing * to gre header. */ - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); + if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr))) + goto free_skb; skb_reset_mac_header(skb); if (skb->ip_summed == CHECKSUM_PARTIAL &&
In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns true. For example, applications can create a malformed packet that causes this problem with PF_PACKET. This patch fixes the problem by dropping skb and returning from the function if skb_pull() fails. Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> --- net/ipv4/ip_gre.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)