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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> wrote:
>
> On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <[email protected]> wrote:
> >
> > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]> wrote:
>
> On Tue, Nov 28, 2023 at 5:05 PM Eric Dumazet <[email protected]> wrote:
> >
> > On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <[email protected]> wrote:
> > >
> > > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> > > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>
> On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote:
> > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <[email protected]> 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 <[email protected]>' 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 <[email protected]> wrote:
>>
>> On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote:
>> > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <[email protected]> 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 <[email protected]>' then.
Thanks, Eric! I'll send a v2 patch soon.
Shigeru
>
> Thanks.
>