* [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) @ 2024-09-16 22:47 syzbot 2024-09-24 21:51 ` syzbot 0 siblings, 1 reply; 6+ messages in thread From: syzbot @ 2024-09-16 22:47 UTC (permalink / raw) To: davem, edumazet, herbert, kuba, linux-kernel, netdev, pabeni, steffen.klassert, syzkaller-bugs Hello, syzbot found the following issue on: HEAD commit: 3561373114c8 Merge git://git.kernel.org/pub/scm/linux/kern.. git tree: net-next console output: https://syzkaller.appspot.com/x/log.txt?x=14a36a8b980000 kernel config: https://syzkaller.appspot.com/x/.config?x=be4832509d93a86b dashboard link: https://syzkaller.appspot.com/bug?extid=cc39f136925517aed571 compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 Unfortunately, I don't have any reproducer for this issue yet. Downloadable assets: disk image: https://storage.googleapis.com/syzbot-assets/494b5ef0e99e/disk-35613731.raw.xz vmlinux: https://storage.googleapis.com/syzbot-assets/2ec90c91c7b4/vmlinux-35613731.xz kernel image: https://storage.googleapis.com/syzbot-assets/59a0684dc747/bzImage-35613731.xz IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+cc39f136925517aed571@syzkaller.appspotmail.com ------------[ cut here ]------------ UBSAN: shift-out-of-bounds in ./include/net/xfrm.h:900:23 shift exponent -96 is negative CPU: 1 UID: 0 PID: 12120 Comm: syz.1.1258 Not tainted 6.11.0-rc7-syzkaller-01543-g3561373114c8 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024 Call Trace: <TASK> __dump_stack lib/dump_stack.c:93 [inline] dump_stack_lvl+0x241/0x360 lib/dump_stack.c:119 ubsan_epilogue lib/ubsan.c:231 [inline] __ubsan_handle_shift_out_of_bounds+0x3c8/0x420 lib/ubsan.c:468 addr4_match include/net/xfrm.h:900 [inline] __xfrm4_selector_match net/xfrm/xfrm_policy.c:222 [inline] xfrm_selector_match+0xe9b/0x1030 net/xfrm/xfrm_policy.c:247 xfrm_state_look_at+0xe8/0x480 net/xfrm/xfrm_state.c:1172 xfrm_state_find+0x199f/0x4d70 net/xfrm/xfrm_state.c:1280 xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:2481 [inline] xfrm_tmpl_resolve net/xfrm/xfrm_policy.c:2532 [inline] xfrm_resolve_and_create_bundle+0x6d2/0x2c90 net/xfrm/xfrm_policy.c:2826 xfrm_lookup_with_ifid+0x334/0x1ee0 net/xfrm/xfrm_policy.c:3160 xfrm_lookup net/xfrm/xfrm_policy.c:3289 [inline] xfrm_lookup_route+0x3c/0x1c0 net/xfrm/xfrm_policy.c:3300 ip_route_connect include/net/route.h:333 [inline] __ip4_datagram_connect+0x96c/0x1260 net/ipv4/datagram.c:49 __ip6_datagram_connect+0x194/0x1230 ip6_datagram_connect net/ipv6/datagram.c:279 [inline] ip6_datagram_connect_v6_only+0x63/0xa0 net/ipv6/datagram.c:291 __sys_connect_file net/socket.c:2067 [inline] __sys_connect+0x2df/0x310 net/socket.c:2084 __do_sys_connect net/socket.c:2094 [inline] __se_sys_connect net/socket.c:2091 [inline] __x64_sys_connect+0x7a/0x90 net/socket.c:2091 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7fe6d677def9 Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007fe6d751f038 EFLAGS: 00000246 ORIG_RAX: 000000000000002a RAX: ffffffffffffffda RBX: 00007fe6d6936130 RCX: 00007fe6d677def9 RDX: 000000000000001c RSI: 0000000020000000 RDI: 0000000000000007 RBP: 00007fe6d67f0b76 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 R13: 0000000000000001 R14: 00007fe6d6936130 R15: 00007ffcce438838 </TASK> ---[ end trace ]--- --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. If the report is already addressed, let syzbot know by replying with: #syz fix: exact-commit-title If you want to overwrite report's subsystems, reply with: #syz set subsystems: new-subsystem (See the list of subsystem names on the web dashboard) If the report is a duplicate of another one, reply with: #syz dup: exact-subject-of-another-report If you want to undo deduplication, reply with: #syz undup ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) 2024-09-16 22:47 [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) syzbot @ 2024-09-24 21:51 ` syzbot 2024-09-25 11:08 ` Sabrina Dubroca 0 siblings, 1 reply; 6+ messages in thread From: syzbot @ 2024-09-24 21:51 UTC (permalink / raw) To: davem, edumazet, herbert, kuba, linux-kernel, netdev, pabeni, steffen.klassert, syzkaller-bugs syzbot has found a reproducer for the following issue on: HEAD commit: 151ac45348af net: sparx5: Fix invalid timestamps git tree: net-next console+strace: https://syzkaller.appspot.com/x/log.txt?x=15808a80580000 kernel config: https://syzkaller.appspot.com/x/.config?x=37c006d80708398d dashboard link: https://syzkaller.appspot.com/bug?extid=cc39f136925517aed571 compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 syz repro: https://syzkaller.appspot.com/x/repro.syz?x=122ad2a9980000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1387b107980000 Downloadable assets: disk image: https://storage.googleapis.com/syzbot-assets/81152b131cff/disk-151ac453.raw.xz vmlinux: https://storage.googleapis.com/syzbot-assets/013d9758c594/vmlinux-151ac453.xz kernel image: https://storage.googleapis.com/syzbot-assets/9ff7505093fc/bzImage-151ac453.xz IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+cc39f136925517aed571@syzkaller.appspotmail.com ------------[ cut here ]------------ UBSAN: shift-out-of-bounds in ./include/net/xfrm.h:900:23 shift exponent -96 is negative CPU: 0 UID: 0 PID: 5231 Comm: syz-executor893 Not tainted 6.11.0-syzkaller-01459-g151ac45348af #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024 Call Trace: <TASK> __dump_stack lib/dump_stack.c:93 [inline] dump_stack_lvl+0x241/0x360 lib/dump_stack.c:119 ubsan_epilogue lib/ubsan.c:231 [inline] __ubsan_handle_shift_out_of_bounds+0x3c8/0x420 lib/ubsan.c:468 addr4_match include/net/xfrm.h:900 [inline] __xfrm4_selector_match net/xfrm/xfrm_policy.c:222 [inline] xfrm_selector_match+0xe9b/0x1030 net/xfrm/xfrm_policy.c:247 xfrm_state_look_at+0xe8/0x480 net/xfrm/xfrm_state.c:1172 xfrm_state_find+0x199f/0x4d70 net/xfrm/xfrm_state.c:1280 xfrm_tmpl_resolve_one net/xfrm/xfrm_policy.c:2481 [inline] xfrm_tmpl_resolve net/xfrm/xfrm_policy.c:2532 [inline] xfrm_resolve_and_create_bundle+0x6d2/0x2c90 net/xfrm/xfrm_policy.c:2826 xfrm_lookup_with_ifid+0x334/0x1ee0 net/xfrm/xfrm_policy.c:3160 xfrm_lookup net/xfrm/xfrm_policy.c:3289 [inline] xfrm_lookup_route+0x3c/0x1c0 net/xfrm/xfrm_policy.c:3300 ip_route_connect include/net/route.h:333 [inline] __ip4_datagram_connect+0x96c/0x1260 net/ipv4/datagram.c:49 __ip6_datagram_connect+0x194/0x1230 ip6_datagram_connect net/ipv6/datagram.c:279 [inline] ip6_datagram_connect_v6_only+0x63/0xa0 net/ipv6/datagram.c:291 __sys_connect_file net/socket.c:2067 [inline] __sys_connect+0x2df/0x310 net/socket.c:2084 __do_sys_connect net/socket.c:2094 [inline] __se_sys_connect net/socket.c:2091 [inline] __x64_sys_connect+0x7a/0x90 net/socket.c:2091 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f RIP: 0033:0x7fb0cdb8e8a9 Code: 48 83 c4 28 c3 e8 37 17 00 00 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:00007ffdce8cd648 EFLAGS: 00000246 ORIG_RAX: 000000000000002a RAX: ffffffffffffffda RBX: 00007ffdce8cd818 RCX: 00007fb0cdb8e8a9 RDX: 000000000000001c RSI: 0000000020000000 RDI: 0000000000000004 RBP: 00007fb0cdc01610 R08: 000000000000000a R09: 00007ffdce8cd818 R10: 00000000000000e8 R11: 0000000000000246 R12: 0000000000000001 R13: 00007ffdce8cd808 R14: 0000000000000001 R15: 0000000000000001 </TASK> ---[ end trace ]--- --- If you want syzbot to run the reproducer, reply with: #syz test: git://repo/address.git branch-or-commit-hash If you attach or paste a git patch, syzbot will apply it before testing. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) 2024-09-24 21:51 ` syzbot @ 2024-09-25 11:08 ` Sabrina Dubroca 2024-09-27 7:30 ` Steffen Klassert 0 siblings, 1 reply; 6+ messages in thread From: Sabrina Dubroca @ 2024-09-25 11:08 UTC (permalink / raw) To: steffen.klassert, pabeni, syzbot Cc: davem, edumazet, herbert, kuba, linux-kernel, netdev, syzkaller-bugs 2024-09-24, 14:51:20 -0700, syzbot wrote: > syzbot has found a reproducer for the following issue on: > > HEAD commit: 151ac45348af net: sparx5: Fix invalid timestamps > git tree: net-next > console+strace: https://syzkaller.appspot.com/x/log.txt?x=15808a80580000 > kernel config: https://syzkaller.appspot.com/x/.config?x=37c006d80708398d > dashboard link: https://syzkaller.appspot.com/bug?extid=cc39f136925517aed571 > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=122ad2a9980000 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1387b107980000 syzbot managed to create an SA with: usersa.sel.family = 0 usersa.sel.prefixlen_s = 128 usersa.family = AF_INET Because of the AF_UNSPEC selector, verify_newsa_info doesn't put limits on prefixlen_{s,d}. But then copy_from_user_state sets x->sel.family to usersa.family (AF_INET). So I think verify_newsa_info should do the same conversion before checking prefixlen: diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 55f039ec3d59..8d06a37adbd9 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -201,6 +201,7 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, { int err; u8 sa_dir = attrs[XFRMA_SA_DIR] ? nla_get_u8(attrs[XFRMA_SA_DIR]) : 0; + u16 family = p->sel.family; err = -EINVAL; switch (p->family) { @@ -221,7 +222,10 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, goto out; } - switch (p->sel.family) { + if (!family && !(p->flags & XFRM_STATE_AF_UNSPEC)) + family = p->family; + + switch (family) { case AF_UNSPEC: break; Steffen, does that make sense? Without this, we have prefixlen=128 when we get to addr4_match, which does a shift of (32 - prefixlen), so we get UBSAN: shift-out-of-bounds in ./include/net/xfrm.h:900:23 shift exponent -96 is negative Maybe a check for prefixlen < 128 would also be useful in the XFRM_STATE_AF_UNSPEC case, to avoid the same problems with syzbot passing prefixlen=200 for an ipv6 SA. I don't know how XFRM_STATE_AF_UNSPEC is used, so I'm not sure what restrictions we can put. If we end up with prefixlen = 100 used from ipv4 we'll still have the same issues. > __ip4_datagram_connect+0x96c/0x1260 net/ipv4/datagram.c:49 > __ip6_datagram_connect+0x194/0x1230 > ip6_datagram_connect net/ipv6/datagram.c:279 [inline] > ip6_datagram_connect_v6_only+0x63/0xa0 net/ipv6/datagram.c:291 This path also looks a bit dubious. From the reproducer, we have a rawv6 socket trying to connect to a v4mapped address, despite having ip6_datagram_connect_v6_only as its ->connect. pingv6 sockets also use ip6_datagram_connect_v6_only and set sk->sk_ipv6only=1 (in net/ipv4/ping.c ping_init_sock), but rawv6 don't have this, so __ip6_datagram_connect can end up in __ip4_datagram_connect. I guess it would make sense to set it in rawv6 too. rawv6_bind already rejected v4mapped addresses. And then we could add a DEBUG_NET_WARN_ON_ONCE(!ipv6_only_sock(sk)) in ip6_datagram_connect_v6_only, or maybe even call ipv6_addr_type to reject v4mapped addresses and reject them like the non-AF_INET6 case. -- Sabrina ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) 2024-09-25 11:08 ` Sabrina Dubroca @ 2024-09-27 7:30 ` Steffen Klassert 2024-09-27 8:38 ` Sabrina Dubroca 0 siblings, 1 reply; 6+ messages in thread From: Steffen Klassert @ 2024-09-27 7:30 UTC (permalink / raw) To: Sabrina Dubroca Cc: pabeni, syzbot, davem, edumazet, herbert, kuba, linux-kernel, netdev, syzkaller-bugs On Wed, Sep 25, 2024 at 01:08:48PM +0200, Sabrina Dubroca wrote: > 2024-09-24, 14:51:20 -0700, syzbot wrote: > > syzbot has found a reproducer for the following issue on: > > > > HEAD commit: 151ac45348af net: sparx5: Fix invalid timestamps > > git tree: net-next > > console+strace: https://syzkaller.appspot.com/x/log.txt?x=15808a80580000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=37c006d80708398d > > dashboard link: https://syzkaller.appspot.com/bug?extid=cc39f136925517aed571 > > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=122ad2a9980000 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1387b107980000 > > syzbot managed to create an SA with: > > usersa.sel.family = 0 > usersa.sel.prefixlen_s = 128 > usersa.family = AF_INET > > Because of the AF_UNSPEC selector, verify_newsa_info doesn't put > limits on prefixlen_{s,d}. But then copy_from_user_state sets > x->sel.family to usersa.family (AF_INET). > > So I think verify_newsa_info should do the same conversion before > checking prefixlen: > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > index 55f039ec3d59..8d06a37adbd9 100644 > --- a/net/xfrm/xfrm_user.c > +++ b/net/xfrm/xfrm_user.c > @@ -201,6 +201,7 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, > { > int err; > u8 sa_dir = attrs[XFRMA_SA_DIR] ? nla_get_u8(attrs[XFRMA_SA_DIR]) : 0; > + u16 family = p->sel.family; > > err = -EINVAL; > switch (p->family) { > @@ -221,7 +222,10 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, > goto out; > } > > - switch (p->sel.family) { > + if (!family && !(p->flags & XFRM_STATE_AF_UNSPEC)) > + family = p->family; > + > + switch (family) { > case AF_UNSPEC: > break; > > > > Steffen, does that make sense? Yes, it does. Later, in copy_from_user_state() we do if (!x->sel.family && !(p->flags & XFRM_STATE_AF_UNSPEC)) x->sel.family = p->family; anyway. > Without this, we have prefixlen=128 when we get to addr4_match, which > does a shift of (32 - prefixlen), so we get > > UBSAN: shift-out-of-bounds in ./include/net/xfrm.h:900:23 > shift exponent -96 is negative > > > Maybe a check for prefixlen < 128 would also be useful in the > XFRM_STATE_AF_UNSPEC case, to avoid the same problems with syzbot > passing prefixlen=200 for an ipv6 SA. I don't know how > XFRM_STATE_AF_UNSPEC is used, so I'm not sure what restrictions we can > put. If we end up with prefixlen = 100 used from ipv4 we'll still have > the same issues. I've introduced XFRM_STATE_AF_UNSPEC back in 2008 to make inter addressfamily tunnels working while maintaining backwards compatibility to openswan that did not set the selector family. At least that's what I found in an E-Mail conversation from back then. A check for prefixlen <= 128 would make sense in any case. But not sure if we can restrict that somehow further. > > > __ip4_datagram_connect+0x96c/0x1260 net/ipv4/datagram.c:49 > > __ip6_datagram_connect+0x194/0x1230 > > ip6_datagram_connect net/ipv6/datagram.c:279 [inline] > > ip6_datagram_connect_v6_only+0x63/0xa0 net/ipv6/datagram.c:291 > > This path also looks a bit dubious. From the reproducer, we have a > rawv6 socket trying to connect to a v4mapped address, despite having > ip6_datagram_connect_v6_only as its ->connect. > > pingv6 sockets also use ip6_datagram_connect_v6_only and set > sk->sk_ipv6only=1 (in net/ipv4/ping.c ping_init_sock), but rawv6 don't > have this, so __ip6_datagram_connect can end up in > __ip4_datagram_connect. I guess it would make sense to set it in rawv6 > too. rawv6_bind already rejected v4mapped addresses. > > And then we could add a DEBUG_NET_WARN_ON_ONCE(!ipv6_only_sock(sk)) in > ip6_datagram_connect_v6_only, or maybe even call ipv6_addr_type to > reject v4mapped addresses and reject them like the non-AF_INET6 case. I can't comment on that now, let me have a closer look into it. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) 2024-09-27 7:30 ` Steffen Klassert @ 2024-09-27 8:38 ` Sabrina Dubroca 2024-10-01 17:10 ` Sabrina Dubroca 0 siblings, 1 reply; 6+ messages in thread From: Sabrina Dubroca @ 2024-09-27 8:38 UTC (permalink / raw) To: Steffen Klassert Cc: pabeni, syzbot, davem, edumazet, herbert, kuba, linux-kernel, netdev, syzkaller-bugs 2024-09-27, 09:30:09 +0200, Steffen Klassert wrote: > On Wed, Sep 25, 2024 at 01:08:48PM +0200, Sabrina Dubroca wrote: > > 2024-09-24, 14:51:20 -0700, syzbot wrote: > > > syzbot has found a reproducer for the following issue on: > > > > > > HEAD commit: 151ac45348af net: sparx5: Fix invalid timestamps > > > git tree: net-next > > > console+strace: https://syzkaller.appspot.com/x/log.txt?x=15808a80580000 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=37c006d80708398d > > > dashboard link: https://syzkaller.appspot.com/bug?extid=cc39f136925517aed571 > > > compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40 > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=122ad2a9980000 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1387b107980000 > > > > syzbot managed to create an SA with: > > > > usersa.sel.family = 0 > > usersa.sel.prefixlen_s = 128 > > usersa.family = AF_INET > > > > Because of the AF_UNSPEC selector, verify_newsa_info doesn't put > > limits on prefixlen_{s,d}. But then copy_from_user_state sets > > x->sel.family to usersa.family (AF_INET). > > > > So I think verify_newsa_info should do the same conversion before > > checking prefixlen: > > > > > > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c > > index 55f039ec3d59..8d06a37adbd9 100644 > > --- a/net/xfrm/xfrm_user.c > > +++ b/net/xfrm/xfrm_user.c > > @@ -201,6 +201,7 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, > > { > > int err; > > u8 sa_dir = attrs[XFRMA_SA_DIR] ? nla_get_u8(attrs[XFRMA_SA_DIR]) : 0; > > + u16 family = p->sel.family; > > > > err = -EINVAL; > > switch (p->family) { > > @@ -221,7 +222,10 @@ static int verify_newsa_info(struct xfrm_usersa_info *p, > > goto out; > > } > > > > - switch (p->sel.family) { > > + if (!family && !(p->flags & XFRM_STATE_AF_UNSPEC)) > > + family = p->family; > > + > > + switch (family) { > > case AF_UNSPEC: > > break; > > > > > > > > Steffen, does that make sense? > > Yes, it does. Later, in copy_from_user_state() we do > > if (!x->sel.family && !(p->flags & XFRM_STATE_AF_UNSPEC)) > x->sel.family = p->family; > > anyway. Yes, that's what I based this on. Ok, so I'll make this a proper patch, thanks. > > Without this, we have prefixlen=128 when we get to addr4_match, which > > does a shift of (32 - prefixlen), so we get > > > > UBSAN: shift-out-of-bounds in ./include/net/xfrm.h:900:23 > > shift exponent -96 is negative > > > > > > Maybe a check for prefixlen < 128 would also be useful in the > > XFRM_STATE_AF_UNSPEC case, to avoid the same problems with syzbot > > passing prefixlen=200 for an ipv6 SA. I don't know how > > XFRM_STATE_AF_UNSPEC is used, so I'm not sure what restrictions we can > > put. If we end up with prefixlen = 100 used from ipv4 we'll still have > > the same issues. > > I've introduced XFRM_STATE_AF_UNSPEC back in 2008 to make > inter addressfamily tunnels working while maintaining > backwards compatibility to openswan that did not set > the selector family. At least that's what I found in > an E-Mail conversation from back then. > > A check for prefixlen <= 128 would make sense in any case. > But not sure if we can restrict that somehow further. I'll add this check too, and then I'll run some more experiments with that flag. > > > > > __ip4_datagram_connect+0x96c/0x1260 net/ipv4/datagram.c:49 > > > __ip6_datagram_connect+0x194/0x1230 > > > ip6_datagram_connect net/ipv6/datagram.c:279 [inline] > > > ip6_datagram_connect_v6_only+0x63/0xa0 net/ipv6/datagram.c:291 > > > > This path also looks a bit dubious. From the reproducer, we have a > > rawv6 socket trying to connect to a v4mapped address, despite having > > ip6_datagram_connect_v6_only as its ->connect. > > > > pingv6 sockets also use ip6_datagram_connect_v6_only and set > > sk->sk_ipv6only=1 (in net/ipv4/ping.c ping_init_sock), but rawv6 don't > > have this, so __ip6_datagram_connect can end up in > > __ip4_datagram_connect. I guess it would make sense to set it in rawv6 > > too. rawv6_bind already rejected v4mapped addresses. > > > > And then we could add a DEBUG_NET_WARN_ON_ONCE(!ipv6_only_sock(sk)) in > > ip6_datagram_connect_v6_only, or maybe even call ipv6_addr_type to > > reject v4mapped addresses and reject them like the non-AF_INET6 case. > > I can't comment on that now, let me have a closer look into it. This bit was more intended for Paolo/the netdev maintainers. I looked into it a bit more yesterday, and I don't think we should do anything for ping/raw sockets, because userspace can change sk_ipv6only via setsockopt(with SOL_IPV6,IPV6_V6ONLY). So we can only make sure that the kernel doesn't misbehave with v4mapped addresses, which I think is the case (pingv6 sockets will return EINVAL when ping_v6_sendmsg sees a v4mapped address, and rawv6 sockets will let the user send those packets but I didn't see any OOB accesses). -- Sabrina ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) 2024-09-27 8:38 ` Sabrina Dubroca @ 2024-10-01 17:10 ` Sabrina Dubroca 0 siblings, 0 replies; 6+ messages in thread From: Sabrina Dubroca @ 2024-10-01 17:10 UTC (permalink / raw) To: Steffen Klassert Cc: pabeni, syzbot, davem, edumazet, herbert, kuba, linux-kernel, netdev, syzkaller-bugs Hi Steffen, 2024-09-27, 10:38:13 +0200, Sabrina Dubroca wrote: > 2024-09-27, 09:30:09 +0200, Steffen Klassert wrote: > > On Wed, Sep 25, 2024 at 01:08:48PM +0200, Sabrina Dubroca wrote: > > > Maybe a check for prefixlen < 128 would also be useful in the > > > XFRM_STATE_AF_UNSPEC case, to avoid the same problems with syzbot > > > passing prefixlen=200 for an ipv6 SA. I don't know how > > > XFRM_STATE_AF_UNSPEC is used, so I'm not sure what restrictions we can > > > put. If we end up with prefixlen = 100 used from ipv4 we'll still have > > > the same issues. > > > > I've introduced XFRM_STATE_AF_UNSPEC back in 2008 to make > > inter addressfamily tunnels working while maintaining > > backwards compatibility to openswan that did not set > > the selector family. At least that's what I found in > > an E-Mail conversation from back then. > > > > A check for prefixlen <= 128 would make sense in any case. > > But not sure if we can restrict that somehow further. > > I'll add this check too, and then I'll run some more experiments with > that flag. I ended up not adding the check, since for x->sel.family == AF_UNSPEC, xfrm_state_look_at doesn't use the selector at all, so I don't think restricting prefixlen in that case would do anything. -- Sabrina ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-01 17:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-16 22:47 [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2) syzbot 2024-09-24 21:51 ` syzbot 2024-09-25 11:08 ` Sabrina Dubroca 2024-09-27 7:30 ` Steffen Klassert 2024-09-27 8:38 ` Sabrina Dubroca 2024-10-01 17:10 ` Sabrina Dubroca
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).