* UBSAN: object-size-mismatch in wg_xmit @ 2020-12-20 16:54 ` syzbot 0 siblings, 0 replies; 39+ messages in thread From: syzbot @ 2020-12-20 16:54 UTC (permalink / raw) To: Jason, davem, kuba, linux-kernel, netdev, syzkaller-bugs, wireguard Hello, syzbot found the following issue on: HEAD commit: 5e60366d Merge tag 'fallthrough-fixes-clang-5.11-rc1' of g.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=12b12c13500000 kernel config: https://syzkaller.appspot.com/x/.config?x=267a60b188ded8ed dashboard link: https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project.git ca2dcbd030eadbf0aa9b660efe864ff08af6e18b) Unfortunately, I don't have any reproducer for this issue yet. IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+8f90d005ab2d22342b6d@syzkaller.appspotmail.com ================================================================================ UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2021:28 member access within address 0000000085889cc2 with insufficient space for an object of type 'struct sk_buff' CPU: 1 PID: 2998 Comm: kworker/1:2 Not tainted 5.10.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: ipv6_addrconf addrconf_dad_work Call Trace: __dump_stack lib/dump_stack.c:79 [inline] dump_stack+0x137/0x1be lib/dump_stack.c:120 ubsan_epilogue lib/ubsan.c:148 [inline] handle_object_size_mismatch lib/ubsan.c:297 [inline] ubsan_type_mismatch_common+0x1e2/0x390 lib/ubsan.c:310 __ubsan_handle_type_mismatch_v1+0x41/0x50 lib/ubsan.c:339 __skb_queue_before include/linux/skbuff.h:2021 [inline] __skb_queue_tail include/linux/skbuff.h:2054 [inline] wg_xmit+0x45d/0xdf0 drivers/net/wireguard/device.c:182 __netdev_start_xmit include/linux/netdevice.h:4775 [inline] netdev_start_xmit+0x7b/0x140 include/linux/netdevice.h:4789 xmit_one net/core/dev.c:3556 [inline] dev_hard_start_xmit+0x182/0x2e0 net/core/dev.c:3572 __dev_queue_xmit+0x1229/0x1e60 net/core/dev.c:4133 neigh_output include/net/neighbour.h:510 [inline] ip6_finish_output2+0xe8d/0x11e0 net/ipv6/ip6_output.c:117 dst_output include/net/dst.h:441 [inline] NF_HOOK include/linux/netfilter.h:301 [inline] ndisc_send_skb+0x85b/0xc70 net/ipv6/ndisc.c:508 addrconf_dad_completed+0x5ef/0x990 net/ipv6/addrconf.c:4192 addrconf_dad_work+0xb92/0x1480 net/ipv6/addrconf.c:3959 process_one_work+0x471/0x830 kernel/workqueue.c:2275 worker_thread+0x757/0xb10 kernel/workqueue.c:2421 kthread+0x39a/0x3c0 kernel/kthread.c:292 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296 ================================================================================ --- 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. ^ permalink raw reply [flat|nested] 39+ messages in thread
* UBSAN: object-size-mismatch in wg_xmit @ 2020-12-20 16:54 ` syzbot 0 siblings, 0 replies; 39+ messages in thread From: syzbot @ 2020-12-20 16:54 UTC (permalink / raw) To: Jason, davem, kuba, linux-kernel, netdev, syzkaller-bugs, wireguard Hello, syzbot found the following issue on: HEAD commit: 5e60366d Merge tag 'fallthrough-fixes-clang-5.11-rc1' of g.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=12b12c13500000 kernel config: https://syzkaller.appspot.com/x/.config?x=267a60b188ded8ed dashboard link: https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project.git ca2dcbd030eadbf0aa9b660efe864ff08af6e18b) Unfortunately, I don't have any reproducer for this issue yet. IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+8f90d005ab2d22342b6d@syzkaller.appspotmail.com ================================================================================ UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2021:28 member access within address 0000000085889cc2 with insufficient space for an object of type 'struct sk_buff' CPU: 1 PID: 2998 Comm: kworker/1:2 Not tainted 5.10.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: ipv6_addrconf addrconf_dad_work Call Trace: __dump_stack lib/dump_stack.c:79 [inline] dump_stack+0x137/0x1be lib/dump_stack.c:120 ubsan_epilogue lib/ubsan.c:148 [inline] handle_object_size_mismatch lib/ubsan.c:297 [inline] ubsan_type_mismatch_common+0x1e2/0x390 lib/ubsan.c:310 __ubsan_handle_type_mismatch_v1+0x41/0x50 lib/ubsan.c:339 __skb_queue_before include/linux/skbuff.h:2021 [inline] __skb_queue_tail include/linux/skbuff.h:2054 [inline] wg_xmit+0x45d/0xdf0 drivers/net/wireguard/device.c:182 __netdev_start_xmit include/linux/netdevice.h:4775 [inline] netdev_start_xmit+0x7b/0x140 include/linux/netdevice.h:4789 xmit_one net/core/dev.c:3556 [inline] dev_hard_start_xmit+0x182/0x2e0 net/core/dev.c:3572 __dev_queue_xmit+0x1229/0x1e60 net/core/dev.c:4133 neigh_output include/net/neighbour.h:510 [inline] ip6_finish_output2+0xe8d/0x11e0 net/ipv6/ip6_output.c:117 dst_output include/net/dst.h:441 [inline] NF_HOOK include/linux/netfilter.h:301 [inline] ndisc_send_skb+0x85b/0xc70 net/ipv6/ndisc.c:508 addrconf_dad_completed+0x5ef/0x990 net/ipv6/addrconf.c:4192 addrconf_dad_work+0xb92/0x1480 net/ipv6/addrconf.c:3959 process_one_work+0x471/0x830 kernel/workqueue.c:2275 worker_thread+0x757/0xb10 kernel/workqueue.c:2421 kthread+0x39a/0x3c0 kernel/kthread.c:292 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:296 ================================================================================ --- 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. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2020-12-20 16:54 ` syzbot @ 2020-12-20 21:11 ` Jason A. Donenfeld -1 siblings, 0 replies; 39+ messages in thread From: Jason A. Donenfeld @ 2020-12-20 21:11 UTC (permalink / raw) To: Netdev; +Cc: syzkaller-bugs, WireGuard mailing list, Eric Dumazet Hmm, on first glance, I'm not sure I'm seeing the bug: On Sun, Dec 20, 2020 at 5:54 PM syzbot <syzbot+8f90d005ab2d22342b6d@syzkaller.appspotmail.com> wrote: > UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2021:28 > member access within address 0000000085889cc2 with insufficient space > for an object of type 'struct sk_buff' > __skb_queue_before include/linux/skbuff.h:2021 [inline] > __skb_queue_tail include/linux/skbuff.h:2054 [inline] > wg_xmit+0x45d/0xdf0 drivers/net/wireguard/device.c:182 The code in question is: struct sk_buff_head packets; __skb_queue_head_init(&packets); ... skb_list_walk_safe(skb, skb, next) { skb_mark_not_on_list(skb); skb = skb_share_check(skb, GFP_ATOMIC); if (unlikely(!skb)) continue; ... __skb_queue_tail(&packets, skb); } We're in a netdev's xmit function, so nothing else should have skb at that point. Given the warning is about "member access", I assume it's the next->prev dereference here: static inline void __skb_queue_before(struct sk_buff_head *list, struct sk_buff *next, struct sk_buff *newsk) { __skb_insert(newsk, next->prev, next, list); } So where is "next" coming from that UBSAN would complain about object-size-mismatch? static inline void __skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk) { __skb_queue_before(list, (struct sk_buff *)list, newsk); } It comes from casting "list" into an sk_buff. While this might be some CFI-violating polymorphism, I can't see why this cast would actually be a problem in practice. The top of sk_buff is intentionally the same as sk_buff_head: struct sk_buff_head { struct sk_buff *next; struct sk_buff *prev; ... struct sk_buff { union { struct { struct sk_buff *next; struct sk_buff *prev; ... I'd suspect, "oh maybe it's just a clang 11 bug", but syzbot says it can't reproduce. So that makes me a little more nervous. Does anybody see something I've missed? Jason ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2020-12-20 21:11 ` Jason A. Donenfeld 0 siblings, 0 replies; 39+ messages in thread From: Jason A. Donenfeld @ 2020-12-20 21:11 UTC (permalink / raw) To: Netdev; +Cc: syzkaller-bugs, WireGuard mailing list, Eric Dumazet Hmm, on first glance, I'm not sure I'm seeing the bug: On Sun, Dec 20, 2020 at 5:54 PM syzbot <syzbot+8f90d005ab2d22342b6d@syzkaller.appspotmail.com> wrote: > UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2021:28 > member access within address 0000000085889cc2 with insufficient space > for an object of type 'struct sk_buff' > __skb_queue_before include/linux/skbuff.h:2021 [inline] > __skb_queue_tail include/linux/skbuff.h:2054 [inline] > wg_xmit+0x45d/0xdf0 drivers/net/wireguard/device.c:182 The code in question is: struct sk_buff_head packets; __skb_queue_head_init(&packets); ... skb_list_walk_safe(skb, skb, next) { skb_mark_not_on_list(skb); skb = skb_share_check(skb, GFP_ATOMIC); if (unlikely(!skb)) continue; ... __skb_queue_tail(&packets, skb); } We're in a netdev's xmit function, so nothing else should have skb at that point. Given the warning is about "member access", I assume it's the next->prev dereference here: static inline void __skb_queue_before(struct sk_buff_head *list, struct sk_buff *next, struct sk_buff *newsk) { __skb_insert(newsk, next->prev, next, list); } So where is "next" coming from that UBSAN would complain about object-size-mismatch? static inline void __skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk) { __skb_queue_before(list, (struct sk_buff *)list, newsk); } It comes from casting "list" into an sk_buff. While this might be some CFI-violating polymorphism, I can't see why this cast would actually be a problem in practice. The top of sk_buff is intentionally the same as sk_buff_head: struct sk_buff_head { struct sk_buff *next; struct sk_buff *prev; ... struct sk_buff { union { struct { struct sk_buff *next; struct sk_buff *prev; ... I'd suspect, "oh maybe it's just a clang 11 bug", but syzbot says it can't reproduce. So that makes me a little more nervous. Does anybody see something I've missed? Jason ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2020-12-20 21:11 ` Jason A. Donenfeld @ 2020-12-21 9:14 ` Dmitry Vyukov -1 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2020-12-21 9:14 UTC (permalink / raw) To: Jason A. Donenfeld, Kees Cook Cc: Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Sun, Dec 20, 2020 at 10:11 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Hmm, on first glance, I'm not sure I'm seeing the bug: > > On Sun, Dec 20, 2020 at 5:54 PM syzbot > <syzbot+8f90d005ab2d22342b6d@syzkaller.appspotmail.com> wrote: > > UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2021:28 > > member access within address 0000000085889cc2 with insufficient space > > for an object of type 'struct sk_buff' > > __skb_queue_before include/linux/skbuff.h:2021 [inline] > > __skb_queue_tail include/linux/skbuff.h:2054 [inline] > > wg_xmit+0x45d/0xdf0 drivers/net/wireguard/device.c:182 > > The code in question is: > > struct sk_buff_head packets; > __skb_queue_head_init(&packets); > ... > skb_list_walk_safe(skb, skb, next) { > skb_mark_not_on_list(skb); > > skb = skb_share_check(skb, GFP_ATOMIC); > if (unlikely(!skb)) > continue; > ... > __skb_queue_tail(&packets, skb); > } > > We're in a netdev's xmit function, so nothing else should have skb at > that point. Given the warning is about "member access", I assume it's > the next->prev dereference here: > > static inline void __skb_queue_before(struct sk_buff_head *list, > struct sk_buff *next, > struct sk_buff *newsk) > { > __skb_insert(newsk, next->prev, next, list); > } > > So where is "next" coming from that UBSAN would complain about > object-size-mismatch? > > static inline void __skb_queue_tail(struct sk_buff_head *list, > struct sk_buff *newsk) > { > __skb_queue_before(list, (struct sk_buff *)list, newsk); > } > > It comes from casting "list" into an sk_buff. While this might be some > CFI-violating polymorphism, I can't see why this cast would actually > be a problem in practice. The top of sk_buff is intentionally the same > as sk_buff_head: > > struct sk_buff_head { > struct sk_buff *next; > struct sk_buff *prev; > ... > struct sk_buff { > union { > struct { > struct sk_buff *next; > struct sk_buff *prev; > ... > > I'd suspect, "oh maybe it's just a clang 11 bug", but syzbot says it > can't reproduce. So that makes me a little more nervous. > > Does anybody see something I've missed? +Kees for UBSAN report questions Hi Jason, Thanks for looking into this. Reading clang docs for ubsan: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html -fsanitize=object-size: An attempt to potentially use bytes which the optimizer can determine are not part of the object being accessed. This will also detect some types of undefined behavior that may not directly access memory, but are provably incorrect given the size of the objects involved, such as invalid downcasts and calling methods on invalid pointers. These checks are made in terms of __builtin_object_size, and consequently may be able to detect more problems at higher optimization levels. From skimming though your description this seems to fall into "provably incorrect given the size of the objects involved". I guess it's one of these cases which trigger undefined behavior and compiler can e.g. remove all of this code assuming it will be never called at runtime and any branches leading to it will always branch in other directions, or something. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2020-12-21 9:14 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2020-12-21 9:14 UTC (permalink / raw) To: Jason A. Donenfeld, Kees Cook Cc: Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Sun, Dec 20, 2020 at 10:11 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > Hmm, on first glance, I'm not sure I'm seeing the bug: > > On Sun, Dec 20, 2020 at 5:54 PM syzbot > <syzbot+8f90d005ab2d22342b6d@syzkaller.appspotmail.com> wrote: > > UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2021:28 > > member access within address 0000000085889cc2 with insufficient space > > for an object of type 'struct sk_buff' > > __skb_queue_before include/linux/skbuff.h:2021 [inline] > > __skb_queue_tail include/linux/skbuff.h:2054 [inline] > > wg_xmit+0x45d/0xdf0 drivers/net/wireguard/device.c:182 > > The code in question is: > > struct sk_buff_head packets; > __skb_queue_head_init(&packets); > ... > skb_list_walk_safe(skb, skb, next) { > skb_mark_not_on_list(skb); > > skb = skb_share_check(skb, GFP_ATOMIC); > if (unlikely(!skb)) > continue; > ... > __skb_queue_tail(&packets, skb); > } > > We're in a netdev's xmit function, so nothing else should have skb at > that point. Given the warning is about "member access", I assume it's > the next->prev dereference here: > > static inline void __skb_queue_before(struct sk_buff_head *list, > struct sk_buff *next, > struct sk_buff *newsk) > { > __skb_insert(newsk, next->prev, next, list); > } > > So where is "next" coming from that UBSAN would complain about > object-size-mismatch? > > static inline void __skb_queue_tail(struct sk_buff_head *list, > struct sk_buff *newsk) > { > __skb_queue_before(list, (struct sk_buff *)list, newsk); > } > > It comes from casting "list" into an sk_buff. While this might be some > CFI-violating polymorphism, I can't see why this cast would actually > be a problem in practice. The top of sk_buff is intentionally the same > as sk_buff_head: > > struct sk_buff_head { > struct sk_buff *next; > struct sk_buff *prev; > ... > struct sk_buff { > union { > struct { > struct sk_buff *next; > struct sk_buff *prev; > ... > > I'd suspect, "oh maybe it's just a clang 11 bug", but syzbot says it > can't reproduce. So that makes me a little more nervous. > > Does anybody see something I've missed? +Kees for UBSAN report questions Hi Jason, Thanks for looking into this. Reading clang docs for ubsan: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html -fsanitize=object-size: An attempt to potentially use bytes which the optimizer can determine are not part of the object being accessed. This will also detect some types of undefined behavior that may not directly access memory, but are provably incorrect given the size of the objects involved, such as invalid downcasts and calling methods on invalid pointers. These checks are made in terms of __builtin_object_size, and consequently may be able to detect more problems at higher optimization levels. From skimming though your description this seems to fall into "provably incorrect given the size of the objects involved". I guess it's one of these cases which trigger undefined behavior and compiler can e.g. remove all of this code assuming it will be never called at runtime and any branches leading to it will always branch in other directions, or something. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2020-12-21 9:14 ` Dmitry Vyukov @ 2020-12-21 11:23 ` Jason A. Donenfeld -1 siblings, 0 replies; 39+ messages in thread From: Jason A. Donenfeld @ 2020-12-21 11:23 UTC (permalink / raw) To: Dmitry Vyukov Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet Hi Dmitry, On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > Hi Jason, > > Thanks for looking into this. > > Reading clang docs for ubsan: > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > -fsanitize=object-size: An attempt to potentially use bytes which the > optimizer can determine are not part of the object being accessed. > This will also detect some types of undefined behavior that may not > directly access memory, but are provably incorrect given the size of > the objects involved, such as invalid downcasts and calling methods on > invalid pointers. These checks are made in terms of > __builtin_object_size, and consequently may be able to detect more > problems at higher optimization levels. > > From skimming though your description this seems to fall into > "provably incorrect given the size of the objects involved". > I guess it's one of these cases which trigger undefined behavior and > compiler can e.g. remove all of this code assuming it will be never > called at runtime and any branches leading to it will always branch in > other directions, or something. Right that sort of makes sense, and I can imagine that in more general cases the struct casting could lead to UB. But what has me scratching my head is that syzbot couldn't reproduce. The cast happens every time. What about that one time was special? Did the address happen to fall on the border of a mapping? Is UBSAN non-deterministic as an optimization? Or is there actually some mysterious UaF happening with my usage of skbs that I shouldn't overlook? Jason ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2020-12-21 11:23 ` Jason A. Donenfeld 0 siblings, 0 replies; 39+ messages in thread From: Jason A. Donenfeld @ 2020-12-21 11:23 UTC (permalink / raw) To: Dmitry Vyukov Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet Hi Dmitry, On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > Hi Jason, > > Thanks for looking into this. > > Reading clang docs for ubsan: > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > -fsanitize=object-size: An attempt to potentially use bytes which the > optimizer can determine are not part of the object being accessed. > This will also detect some types of undefined behavior that may not > directly access memory, but are provably incorrect given the size of > the objects involved, such as invalid downcasts and calling methods on > invalid pointers. These checks are made in terms of > __builtin_object_size, and consequently may be able to detect more > problems at higher optimization levels. > > From skimming though your description this seems to fall into > "provably incorrect given the size of the objects involved". > I guess it's one of these cases which trigger undefined behavior and > compiler can e.g. remove all of this code assuming it will be never > called at runtime and any branches leading to it will always branch in > other directions, or something. Right that sort of makes sense, and I can imagine that in more general cases the struct casting could lead to UB. But what has me scratching my head is that syzbot couldn't reproduce. The cast happens every time. What about that one time was special? Did the address happen to fall on the border of a mapping? Is UBSAN non-deterministic as an optimization? Or is there actually some mysterious UaF happening with my usage of skbs that I shouldn't overlook? Jason ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2020-12-21 11:23 ` Jason A. Donenfeld @ 2021-01-07 12:22 ` Dmitry Vyukov -1 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-07 12:22 UTC (permalink / raw) To: Jason A. Donenfeld Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Dmitry, > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > Hi Jason, > > > > Thanks for looking into this. > > > > Reading clang docs for ubsan: > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > > -fsanitize=object-size: An attempt to potentially use bytes which the > > optimizer can determine are not part of the object being accessed. > > This will also detect some types of undefined behavior that may not > > directly access memory, but are provably incorrect given the size of > > the objects involved, such as invalid downcasts and calling methods on > > invalid pointers. These checks are made in terms of > > __builtin_object_size, and consequently may be able to detect more > > problems at higher optimization levels. > > > > From skimming though your description this seems to fall into > > "provably incorrect given the size of the objects involved". > > I guess it's one of these cases which trigger undefined behavior and > > compiler can e.g. remove all of this code assuming it will be never > > called at runtime and any branches leading to it will always branch in > > other directions, or something. > > Right that sort of makes sense, and I can imagine that in more general > cases the struct casting could lead to UB. But what has me scratching > my head is that syzbot couldn't reproduce. The cast happens every > time. What about that one time was special? Did the address happen to > fall on the border of a mapping? Is UBSAN non-deterministic as an > optimization? Or is there actually some mysterious UaF happening with > my usage of skbs that I shouldn't overlook? These UBSAN checks were just enabled recently. It's indeed super easy to trigger: 133083 VMs were crashed on this already: https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d So it's one of the top crashers by now. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-07 12:22 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-07 12:22 UTC (permalink / raw) To: Jason A. Donenfeld Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Dmitry, > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > Hi Jason, > > > > Thanks for looking into this. > > > > Reading clang docs for ubsan: > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > > -fsanitize=object-size: An attempt to potentially use bytes which the > > optimizer can determine are not part of the object being accessed. > > This will also detect some types of undefined behavior that may not > > directly access memory, but are provably incorrect given the size of > > the objects involved, such as invalid downcasts and calling methods on > > invalid pointers. These checks are made in terms of > > __builtin_object_size, and consequently may be able to detect more > > problems at higher optimization levels. > > > > From skimming though your description this seems to fall into > > "provably incorrect given the size of the objects involved". > > I guess it's one of these cases which trigger undefined behavior and > > compiler can e.g. remove all of this code assuming it will be never > > called at runtime and any branches leading to it will always branch in > > other directions, or something. > > Right that sort of makes sense, and I can imagine that in more general > cases the struct casting could lead to UB. But what has me scratching > my head is that syzbot couldn't reproduce. The cast happens every > time. What about that one time was special? Did the address happen to > fall on the border of a mapping? Is UBSAN non-deterministic as an > optimization? Or is there actually some mysterious UaF happening with > my usage of skbs that I shouldn't overlook? These UBSAN checks were just enabled recently. It's indeed super easy to trigger: 133083 VMs were crashed on this already: https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d So it's one of the top crashers by now. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-07 12:22 ` Dmitry Vyukov (?) @ 2021-01-07 19:00 ` Jason A. Donenfeld 2021-01-07 19:06 ` Jeffrey Walton 2021-01-08 9:30 ` Dmitry Vyukov -1 siblings, 2 replies; 39+ messages in thread From: Jason A. Donenfeld @ 2021-01-07 19:00 UTC (permalink / raw) To: Dmitry Vyukov Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > Hi Dmitry, > > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > Hi Jason, > > > > > > Thanks for looking into this. > > > > > > Reading clang docs for ubsan: > > > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > > > -fsanitize=object-size: An attempt to potentially use bytes which the > > > optimizer can determine are not part of the object being accessed. > > > This will also detect some types of undefined behavior that may not > > > directly access memory, but are provably incorrect given the size of > > > the objects involved, such as invalid downcasts and calling methods on > > > invalid pointers. These checks are made in terms of > > > __builtin_object_size, and consequently may be able to detect more > > > problems at higher optimization levels. > > > > > > From skimming though your description this seems to fall into > > > "provably incorrect given the size of the objects involved". > > > I guess it's one of these cases which trigger undefined behavior and > > > compiler can e.g. remove all of this code assuming it will be never > > > called at runtime and any branches leading to it will always branch in > > > other directions, or something. > > > > Right that sort of makes sense, and I can imagine that in more general > > cases the struct casting could lead to UB. But what has me scratching > > my head is that syzbot couldn't reproduce. The cast happens every > > time. What about that one time was special? Did the address happen to > > fall on the border of a mapping? Is UBSAN non-deterministic as an > > optimization? Or is there actually some mysterious UaF happening with > > my usage of skbs that I shouldn't overlook? > > These UBSAN checks were just enabled recently. > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > So it's one of the top crashers by now. Ahh, makes sense. So it is easily reproducible after all. You're still of the opinion that it's a false positive, right? I shouldn't spend more cycles on this? Jason ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-07 19:00 ` Jason A. Donenfeld @ 2021-01-07 19:06 ` Jeffrey Walton 2021-01-08 9:30 ` Dmitry Vyukov 1 sibling, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-07 19:06 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > ... > > > > These UBSAN checks were just enabled recently. > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > So it's one of the top crashers by now. > > Ahh, makes sense. So it is easily reproducible after all. > > You're still of the opinion that it's a false positive, right? I > shouldn't spend more cycles on this? You might consider making a test build with -fno-lto in case LTO is mucking things up. Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By Clang, https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Clang-LTO-Patches Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-07 19:06 ` Jeffrey Walton 0 siblings, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-07 19:06 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > ... > > > > These UBSAN checks were just enabled recently. > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > So it's one of the top crashers by now. > > Ahh, makes sense. So it is easily reproducible after all. > > You're still of the opinion that it's a false positive, right? I > shouldn't spend more cycles on this? You might consider making a test build with -fno-lto in case LTO is mucking things up. Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By Clang, https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Clang-LTO-Patches Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-07 19:06 ` Jeffrey Walton (?) @ 2021-01-08 0:34 ` Corey Costello 2021-01-08 0:42 ` Eric Light -1 siblings, 1 reply; 39+ messages in thread From: Corey Costello @ 2021-01-08 0:34 UTC (permalink / raw) To: noloader Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list Get me off this fucking list ffs. > On Jan 7, 2021, at 2:06 PM, Jeffrey Walton <noloader@gmail.com> wrote: > > On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> >> On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: >>> >>> On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >>>> >>>> ... >>> >>> These UBSAN checks were just enabled recently. >>> It's indeed super easy to trigger: 133083 VMs were crashed on this already: >>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fsyzkaller.appspot.com%2fbug%3fextid%3d8f90d005ab2d22342b6d&c=E,1,RVpgZsRUCGs2jKlumiMAMnpeOF4QdiW5h8GDIsBJPz-orFNwvwCXnceC9n5Bhr1h-G2EsU0tlC7N4QUpHuF6tIMI7tTnBoRjAo5tT-Bk9-Fhe8CppuOL4mqdkA,,&typo=1 >>> So it's one of the top crashers by now. >> >> Ahh, makes sense. So it is easily reproducible after all. >> >> You're still of the opinion that it's a false positive, right? I >> shouldn't spend more cycles on this? > > You might consider making a test build with -fno-lto in case LTO is > mucking things up. > > Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > Clang, https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fwww.phoronix.com%2fscan.php%3fpage%3dnews_item%26px%3dLinux-Kernel-Clang-LTO-Patches&c=E,1,7u3-jWadklYo8ai_XrPNvjnu46LLAyg0hqsGIaMPaoQ5UxtcNM84jrHUgSg4VciXKk9XVpwgyBwD85LbbW5_j195jSH6RrAej45I1kr_XfQ,&typo=1 > > Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-08 0:34 ` Corey Costello @ 2021-01-08 0:42 ` Eric Light 2021-01-08 0:44 ` Corey Costello 2021-01-08 1:02 ` Phillip McMahon 0 siblings, 2 replies; 39+ messages in thread From: Eric Light @ 2021-01-08 0:42 UTC (permalink / raw) To: Corey Costello; +Cc: WireGuard mailing list Corey - have you tried unsubscribing at the unsubscribe page? https://lists.zx2c4.com/mailman/options/wireguard Hope this helps, E -------------------------------------------- Q: Why is this email five sentences or less? A: http://five.sentenc.es On Fri, 8 Jan 2021, at 13:34, Corey Costello wrote: > Get me off this fucking list ffs. > > > > > On Jan 7, 2021, at 2:06 PM, Jeffrey Walton <noloader@gmail.com> wrote: > > > > On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >> > >> On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > >>> > >>> On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >>>> > >>>> ... > >>> > >>> These UBSAN checks were just enabled recently. > >>> It's indeed super easy to trigger: 133083 VMs were crashed on this already: > >>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fsyzkaller.appspot.com%2fbug%3fextid%3d8f90d005ab2d22342b6d&c=E,1,RVpgZsRUCGs2jKlumiMAMnpeOF4QdiW5h8GDIsBJPz-orFNwvwCXnceC9n5Bhr1h-G2EsU0tlC7N4QUpHuF6tIMI7tTnBoRjAo5tT-Bk9-Fhe8CppuOL4mqdkA,,&typo=1 > >>> So it's one of the top crashers by now. > >> > >> Ahh, makes sense. So it is easily reproducible after all. > >> > >> You're still of the opinion that it's a false positive, right? I > >> shouldn't spend more cycles on this? > > > > You might consider making a test build with -fno-lto in case LTO is > > mucking things up. > > > > Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > > Clang, https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fwww.phoronix.com%2fscan.php%3fpage%3dnews_item%26px%3dLinux-Kernel-Clang-LTO-Patches&c=E,1,7u3-jWadklYo8ai_XrPNvjnu46LLAyg0hqsGIaMPaoQ5UxtcNM84jrHUgSg4VciXKk9XVpwgyBwD85LbbW5_j195jSH6RrAej45I1kr_XfQ,&typo=1 > > > > Jeff > > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-08 0:42 ` Eric Light @ 2021-01-08 0:44 ` Corey Costello 2021-01-08 0:50 ` Eric Light 2021-01-08 1:02 ` Phillip McMahon 1 sibling, 1 reply; 39+ messages in thread From: Corey Costello @ 2021-01-08 0:44 UTC (permalink / raw) To: Eric Light; +Cc: WireGuard mailing list Yeah, it doesn’t work for me :( > On Jan 7, 2021, at 7:42 PM, Eric Light <eric@ericlight.com> wrote: > > Corey - have you tried unsubscribing at the unsubscribe page? https://linkprotect.cudasvc.com/url?a=https%3a%2f%2flists.zx2c4.com%2fmailman%2foptions%2fwireguard&c=E,1,Axa-ElbX0arA0gS27knqMcMnUXikmpGux0uV7Gv66JniMpXRVVtWNeYE8mu17nQawpfQEodteBfH9tq43ronGkmm07T_Tq9urTmtYLTTK8YwFg,,&typo=1 > > Hope this helps, > > E > > -------------------------------------------- > Q: Why is this email five sentences or less? > A: https://linkprotect.cudasvc.com/url?a=http%3a%2f%2ffive.sentenc.es&c=E,1,L28_oBFr0CKAXDsG0BiDkDDhOYaG4FoH7vdeaKoqnsVwn14kiz9J_PpjMZAoItr_wdFvmWS4wAI7qDgBPljT8qxgdAwCA9Vy54M3bcq8nJBKnA11GFTTG6BEv04,&typo=1 > > On Fri, 8 Jan 2021, at 13:34, Corey Costello wrote: >> Get me off this fucking list ffs. >> >> >> >>> On Jan 7, 2021, at 2:06 PM, Jeffrey Walton <noloader@gmail.com> wrote: >>> >>> On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >>>> >>>> On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: >>>>> >>>>> On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >>>>>> >>>>>> ... >>>>> >>>>> These UBSAN checks were just enabled recently. >>>>> It's indeed super easy to trigger: 133083 VMs were crashed on this already: >>>>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fsyzkaller.appspot.com%2fbug%3fextid%3d8f90d005ab2d22342b6d&c=E,1,RVpgZsRUCGs2jKlumiMAMnpeOF4QdiW5h8GDIsBJPz-orFNwvwCXnceC9n5Bhr1h-G2EsU0tlC7N4QUpHuF6tIMI7tTnBoRjAo5tT-Bk9-Fhe8CppuOL4mqdkA,,&typo=1 >>>>> So it's one of the top crashers by now. >>>> >>>> Ahh, makes sense. So it is easily reproducible after all. >>>> >>>> You're still of the opinion that it's a false positive, right? I >>>> shouldn't spend more cycles on this? >>> >>> You might consider making a test build with -fno-lto in case LTO is >>> mucking things up. >>> >>> Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By >>> Clang, https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fwww.phoronix.com%2fscan.php%3fpage%3dnews_item%26px%3dLinux-Kernel-Clang-LTO-Patches&c=E,1,7u3-jWadklYo8ai_XrPNvjnu46LLAyg0hqsGIaMPaoQ5UxtcNM84jrHUgSg4VciXKk9XVpwgyBwD85LbbW5_j195jSH6RrAej45I1kr_XfQ,&typo=1 >>> >>> Jeff >> >> ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-08 0:44 ` Corey Costello @ 2021-01-08 0:50 ` Eric Light 0 siblings, 0 replies; 39+ messages in thread From: Eric Light @ 2021-01-08 0:50 UTC (permalink / raw) To: WireGuard mailing list @List - I'll reach out to Corey directly and try to help. Sorry for the noise. E -------------------------------------------- Q: Why is this email five sentences or less? A: http://five.sentenc.es On Fri, 8 Jan 2021, at 13:44, Corey Costello wrote: > Yeah, it doesn’t work for me :( > > > On Jan 7, 2021, at 7:42 PM, Eric Light <eric@ericlight.com> wrote: > > > > Corey - have you tried unsubscribing at the unsubscribe page? https://linkprotect.cudasvc.com/url?a=https%3a%2f%2flists.zx2c4.com%2fmailman%2foptions%2fwireguard&c=E,1,Axa-ElbX0arA0gS27knqMcMnUXikmpGux0uV7Gv66JniMpXRVVtWNeYE8mu17nQawpfQEodteBfH9tq43ronGkmm07T_Tq9urTmtYLTTK8YwFg,,&typo=1 > > > > Hope this helps, > > > > E > > > > -------------------------------------------- > > Q: Why is this email five sentences or less? > > A: https://linkprotect.cudasvc.com/url?a=http%3a%2f%2ffive.sentenc.es&c=E,1,L28_oBFr0CKAXDsG0BiDkDDhOYaG4FoH7vdeaKoqnsVwn14kiz9J_PpjMZAoItr_wdFvmWS4wAI7qDgBPljT8qxgdAwCA9Vy54M3bcq8nJBKnA11GFTTG6BEv04,&typo=1 > > > > On Fri, 8 Jan 2021, at 13:34, Corey Costello wrote: > >> Get me off this fucking list ffs. > >> > >> > >> > >>> On Jan 7, 2021, at 2:06 PM, Jeffrey Walton <noloader@gmail.com> wrote: > >>> > >>> On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >>>> > >>>> On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > >>>>> > >>>>> On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >>>>>> > >>>>>> ... > >>>>> > >>>>> These UBSAN checks were just enabled recently. > >>>>> It's indeed super easy to trigger: 133083 VMs were crashed on this already: > >>>>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fsyzkaller.appspot.com%2fbug%3fextid%3d8f90d005ab2d22342b6d&c=E,1,RVpgZsRUCGs2jKlumiMAMnpeOF4QdiW5h8GDIsBJPz-orFNwvwCXnceC9n5Bhr1h-G2EsU0tlC7N4QUpHuF6tIMI7tTnBoRjAo5tT-Bk9-Fhe8CppuOL4mqdkA,,&typo=1 > >>>>> So it's one of the top crashers by now. > >>>> > >>>> Ahh, makes sense. So it is easily reproducible after all. > >>>> > >>>> You're still of the opinion that it's a false positive, right? I > >>>> shouldn't spend more cycles on this? > >>> > >>> You might consider making a test build with -fno-lto in case LTO is > >>> mucking things up. > >>> > >>> Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > >>> Clang, https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fwww.phoronix.com%2fscan.php%3fpage%3dnews_item%26px%3dLinux-Kernel-Clang-LTO-Patches&c=E,1,7u3-jWadklYo8ai_XrPNvjnu46LLAyg0hqsGIaMPaoQ5UxtcNM84jrHUgSg4VciXKk9XVpwgyBwD85LbbW5_j195jSH6RrAej45I1kr_XfQ,&typo=1 > >>> > >>> Jeff > >> > >> > > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-08 0:42 ` Eric Light 2021-01-08 0:44 ` Corey Costello @ 2021-01-08 1:02 ` Phillip McMahon 1 sibling, 0 replies; 39+ messages in thread From: Phillip McMahon @ 2021-01-08 1:02 UTC (permalink / raw) To: Eric Light; +Cc: Corey Costello, WireGuard mailing list I went and did it for you. You should have a confirmation email to respond to. Check your spam. On Fri, 8 Jan 2021 at 01:49, Eric Light <eric@ericlight.com> wrote: > > Corey - have you tried unsubscribing at the unsubscribe page? https://lists.zx2c4.com/mailman/options/wireguard > > Hope this helps, > > E > > -------------------------------------------- > Q: Why is this email five sentences or less? > A: http://five.sentenc.es > > On Fri, 8 Jan 2021, at 13:34, Corey Costello wrote: > > Get me off this fucking list ffs. > > > > > > > > > On Jan 7, 2021, at 2:06 PM, Jeffrey Walton <noloader@gmail.com> wrote: > > > > > > On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > >> > > >> On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > >>> > > >>> On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > >>>> > > >>>> ... > > >>> > > >>> These UBSAN checks were just enabled recently. > > >>> It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > >>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fsyzkaller.appspot.com%2fbug%3fextid%3d8f90d005ab2d22342b6d&c=E,1,RVpgZsRUCGs2jKlumiMAMnpeOF4QdiW5h8GDIsBJPz-orFNwvwCXnceC9n5Bhr1h-G2EsU0tlC7N4QUpHuF6tIMI7tTnBoRjAo5tT-Bk9-Fhe8CppuOL4mqdkA,,&typo=1 > > >>> So it's one of the top crashers by now. > > >> > > >> Ahh, makes sense. So it is easily reproducible after all. > > >> > > >> You're still of the opinion that it's a false positive, right? I > > >> shouldn't spend more cycles on this? > > > > > > You might consider making a test build with -fno-lto in case LTO is > > > mucking things up. > > > > > > Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > > > Clang, https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fwww.phoronix.com%2fscan.php%3fpage%3dnews_item%26px%3dLinux-Kernel-Clang-LTO-Patches&c=E,1,7u3-jWadklYo8ai_XrPNvjnu46LLAyg0hqsGIaMPaoQ5UxtcNM84jrHUgSg4VciXKk9XVpwgyBwD85LbbW5_j195jSH6RrAej45I1kr_XfQ,&typo=1 > > > > > > Jeff > > > > -- Use this contact page to send me encrypted messages and files https://flowcrypt.com/me/phillipmcmahon P.S. Drowning in email? Try SaneBox and take back control: http://sanebox.com/t/old3m. I love it. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-07 19:06 ` Jeffrey Walton @ 2021-01-08 9:33 ` Dmitry Vyukov -1 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-08 9:33 UTC (permalink / raw) To: noloader Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list On Thu, Jan 7, 2021 at 8:06 PM Jeffrey Walton <noloader@gmail.com> wrote: > > On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > > > ... > > > > > > These UBSAN checks were just enabled recently. > > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > > So it's one of the top crashers by now. > > > > Ahh, makes sense. So it is easily reproducible after all. > > > > You're still of the opinion that it's a false positive, right? I > > shouldn't spend more cycles on this? > > You might consider making a test build with -fno-lto in case LTO is > mucking things up. > > Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > Clang, https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Clang-LTO-Patches Hi Jeff, Are these patches upstream now? syzbot doesn't enable LTO intentionally, nor I see CONFIG_LTO in the provided config. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-08 9:33 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-08 9:33 UTC (permalink / raw) To: noloader Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list On Thu, Jan 7, 2021 at 8:06 PM Jeffrey Walton <noloader@gmail.com> wrote: > > On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > > > ... > > > > > > These UBSAN checks were just enabled recently. > > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > > So it's one of the top crashers by now. > > > > Ahh, makes sense. So it is easily reproducible after all. > > > > You're still of the opinion that it's a false positive, right? I > > shouldn't spend more cycles on this? > > You might consider making a test build with -fno-lto in case LTO is > mucking things up. > > Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > Clang, https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Clang-LTO-Patches Hi Jeff, Are these patches upstream now? syzbot doesn't enable LTO intentionally, nor I see CONFIG_LTO in the provided config. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-08 9:33 ` Dmitry Vyukov (?) @ 2021-01-08 20:54 ` Nathan Chancellor -1 siblings, 0 replies; 39+ messages in thread From: Nathan Chancellor @ 2021-01-08 20:54 UTC (permalink / raw) To: Dmitry Vyukov Cc: noloader, Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list On Fri, Jan 08, 2021 at 10:33:19AM +0100, Dmitry Vyukov wrote: > On Thu, Jan 7, 2021 at 8:06 PM Jeffrey Walton <noloader@gmail.com> wrote: > > > > On Thu, Jan 7, 2021 at 2:03 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > On Thu, Jan 7, 2021 at 1:22 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > > > > > On Mon, Dec 21, 2020 at 12:23 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > > > > > ... > > > > > > > > These UBSAN checks were just enabled recently. > > > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > > > So it's one of the top crashers by now. > > > > > > Ahh, makes sense. So it is easily reproducible after all. > > > > > > You're still of the opinion that it's a false positive, right? I > > > shouldn't spend more cycles on this? > > > > You might consider making a test build with -fno-lto in case LTO is > > mucking things up. > > > > Google Posts Patches So The Linux Kernel Can Be LTO-Optimized By > > Clang, https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Clang-LTO-Patches > > Hi Jeff, > > Are these patches upstream now? > syzbot doesn't enable LTO intentionally, nor I see CONFIG_LTO in the > provided config. Those patches are not upstream yet and LTO will have to be explicitly enabled via config. Cheers, Nathan ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-07 19:00 ` Jason A. Donenfeld @ 2021-01-08 9:30 ` Dmitry Vyukov 2021-01-08 9:30 ` Dmitry Vyukov 1 sibling, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-08 9:30 UTC (permalink / raw) To: Jason A. Donenfeld Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > Hi Dmitry, > > > > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > Hi Jason, > > > > > > > > Thanks for looking into this. > > > > > > > > Reading clang docs for ubsan: > > > > > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > > > > -fsanitize=object-size: An attempt to potentially use bytes which the > > > > optimizer can determine are not part of the object being accessed. > > > > This will also detect some types of undefined behavior that may not > > > > directly access memory, but are provably incorrect given the size of > > > > the objects involved, such as invalid downcasts and calling methods on > > > > invalid pointers. These checks are made in terms of > > > > __builtin_object_size, and consequently may be able to detect more > > > > problems at higher optimization levels. > > > > > > > > From skimming though your description this seems to fall into > > > > "provably incorrect given the size of the objects involved". > > > > I guess it's one of these cases which trigger undefined behavior and > > > > compiler can e.g. remove all of this code assuming it will be never > > > > called at runtime and any branches leading to it will always branch in > > > > other directions, or something. > > > > > > Right that sort of makes sense, and I can imagine that in more general > > > cases the struct casting could lead to UB. But what has me scratching > > > my head is that syzbot couldn't reproduce. The cast happens every > > > time. What about that one time was special? Did the address happen to > > > fall on the border of a mapping? Is UBSAN non-deterministic as an > > > optimization? Or is there actually some mysterious UaF happening with > > > my usage of skbs that I shouldn't overlook? > > > > These UBSAN checks were just enabled recently. > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > So it's one of the top crashers by now. > > Ahh, makes sense. So it is easily reproducible after all. > > You're still of the opinion that it's a false positive, right? I > shouldn't spend more cycles on this? No, I am not saying this is a false positive. I think it's an undefined behavior. Either way, we need to resolve this one way or another to unblock testing the rest of the kernel, if not with a fix to wg, then with a fix to ubsan, or disable this check for kernel if kernel community decides we want to use and keep this type of C undefined behavior in the code base intentionally. So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard: https://syzkaller.appspot.com/upstream So cleaning them up looks doable. Is there a way to restructure the code to not invoke undefined behavior? Kees, do you have any suggestions on how to proceed? This seems to stop testing of the whole kernel at the moment. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-08 9:30 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-08 9:30 UTC (permalink / raw) To: Jason A. Donenfeld Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > Hi Dmitry, > > > > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > > > Hi Jason, > > > > > > > > Thanks for looking into this. > > > > > > > > Reading clang docs for ubsan: > > > > > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > > > > -fsanitize=object-size: An attempt to potentially use bytes which the > > > > optimizer can determine are not part of the object being accessed. > > > > This will also detect some types of undefined behavior that may not > > > > directly access memory, but are provably incorrect given the size of > > > > the objects involved, such as invalid downcasts and calling methods on > > > > invalid pointers. These checks are made in terms of > > > > __builtin_object_size, and consequently may be able to detect more > > > > problems at higher optimization levels. > > > > > > > > From skimming though your description this seems to fall into > > > > "provably incorrect given the size of the objects involved". > > > > I guess it's one of these cases which trigger undefined behavior and > > > > compiler can e.g. remove all of this code assuming it will be never > > > > called at runtime and any branches leading to it will always branch in > > > > other directions, or something. > > > > > > Right that sort of makes sense, and I can imagine that in more general > > > cases the struct casting could lead to UB. But what has me scratching > > > my head is that syzbot couldn't reproduce. The cast happens every > > > time. What about that one time was special? Did the address happen to > > > fall on the border of a mapping? Is UBSAN non-deterministic as an > > > optimization? Or is there actually some mysterious UaF happening with > > > my usage of skbs that I shouldn't overlook? > > > > These UBSAN checks were just enabled recently. > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > > So it's one of the top crashers by now. > > Ahh, makes sense. So it is easily reproducible after all. > > You're still of the opinion that it's a false positive, right? I > shouldn't spend more cycles on this? No, I am not saying this is a false positive. I think it's an undefined behavior. Either way, we need to resolve this one way or another to unblock testing the rest of the kernel, if not with a fix to wg, then with a fix to ubsan, or disable this check for kernel if kernel community decides we want to use and keep this type of C undefined behavior in the code base intentionally. So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard: https://syzkaller.appspot.com/upstream So cleaning them up looks doable. Is there a way to restructure the code to not invoke undefined behavior? Kees, do you have any suggestions on how to proceed? This seems to stop testing of the whole kernel at the moment. ^ permalink raw reply [flat|nested] 39+ messages in thread
[parent not found: <CAGXu5j+jzmkiU_AWoTVF6e263iYSSJYUHB=Kdqh-MCfEO-aNSg@mail.gmail.com>]
* Re: UBSAN: object-size-mismatch in wg_xmit [not found] ` <CAGXu5j+jzmkiU_AWoTVF6e263iYSSJYUHB=Kdqh-MCfEO-aNSg@mail.gmail.com> @ 2021-01-09 9:46 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-09 9:46 UTC (permalink / raw) To: Kees Cook Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Fri, Jan 8, 2021 at 9:26 PM Kees Cook <keescook@chromium.org> wrote: >> On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> > > > >> > > > Hi Dmitry, >> > > > >> > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: >> > > > > Hi Jason, >> > > > > >> > > > > Thanks for looking into this. >> > > > > >> > > > > Reading clang docs for ubsan: >> > > > > >> > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html >> > > > > -fsanitize=object-size: An attempt to potentially use bytes which the >> > > > > optimizer can determine are not part of the object being accessed. >> > > > > This will also detect some types of undefined behavior that may not >> > > > > directly access memory, but are provably incorrect given the size of >> > > > > the objects involved, such as invalid downcasts and calling methods on >> > > > > invalid pointers. These checks are made in terms of >> > > > > __builtin_object_size, and consequently may be able to detect more >> > > > > problems at higher optimization levels. >> > > > > >> > > > > From skimming though your description this seems to fall into >> > > > > "provably incorrect given the size of the objects involved". >> > > > > I guess it's one of these cases which trigger undefined behavior and >> > > > > compiler can e.g. remove all of this code assuming it will be never >> > > > > called at runtime and any branches leading to it will always branch in >> > > > > other directions, or something. >> > > > >> > > > Right that sort of makes sense, and I can imagine that in more general >> > > > cases the struct casting could lead to UB. But what has me scratching >> > > > my head is that syzbot couldn't reproduce. The cast happens every >> > > > time. What about that one time was special? Did the address happen to >> > > > fall on the border of a mapping? Is UBSAN non-deterministic as an >> > > > optimization? Or is there actually some mysterious UaF happening with >> > > > my usage of skbs that I shouldn't overlook? >> > > >> > > These UBSAN checks were just enabled recently. >> > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: >> > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d >> > > So it's one of the top crashers by now. >> > >> > Ahh, makes sense. So it is easily reproducible after all. >> > >> > You're still of the opinion that it's a false positive, right? I >> > shouldn't spend more cycles on this? >> >> No, I am not saying this is a false positive. I think it's an >> undefined behavior. >> >> Either way, we need to resolve this one way or another to unblock >> testing the rest of the kernel, if not with a fix to wg, then with a >> fix to ubsan, or disable this check for kernel if kernel community >> decides we want to use and keep this type of C undefined behavior in >> the code base intentionally. >> So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard: >> https://syzkaller.appspot.com/upstream >> So cleaning them up looks doable. Is there a way to restructure the >> code to not invoke undefined behavior? > > > Right; that's my question as well. > >> >> Kees, do you have any suggestions on how to proceed? This seems to >> stop testing of the whole kernel at the moment. > > > If it's blocking other stuff and there isn't a path to fixing it soon, then I think we'll need to disable this check (and open an issue to track it). Oh, I see, the code is actually in skbuff.h: static inline void __skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk) { __skb_queue_before(list, (struct sk_buff *)list, newsk); } It casts sk_buff_head to sk_buff relying on equal layout of some prefix of these structs. Is it really UB in C? UBSAN docs say: "An attempt to potentially use bytes which the optimizer can determine are not part of the object being accessed". But C has POD layout for structs, right? These next/prev fields are within sk_buff_head (otherwise things would explode). I can imagine this may be not valid in C++, can this UBSAN check be C++-specific? Or at least some subset of this check, I can imagine it can detect bad bugs in C as well where things go really wrong. If there is no quick solution proposed, I tend to disable this check in syzbot for now. We need to clean at least common things like sk_buff first. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-09 9:46 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-09 9:46 UTC (permalink / raw) To: Kees Cook Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Fri, Jan 8, 2021 at 9:26 PM Kees Cook <keescook@chromium.org> wrote: >> On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> > > > >> > > > Hi Dmitry, >> > > > >> > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: >> > > > > Hi Jason, >> > > > > >> > > > > Thanks for looking into this. >> > > > > >> > > > > Reading clang docs for ubsan: >> > > > > >> > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html >> > > > > -fsanitize=object-size: An attempt to potentially use bytes which the >> > > > > optimizer can determine are not part of the object being accessed. >> > > > > This will also detect some types of undefined behavior that may not >> > > > > directly access memory, but are provably incorrect given the size of >> > > > > the objects involved, such as invalid downcasts and calling methods on >> > > > > invalid pointers. These checks are made in terms of >> > > > > __builtin_object_size, and consequently may be able to detect more >> > > > > problems at higher optimization levels. >> > > > > >> > > > > From skimming though your description this seems to fall into >> > > > > "provably incorrect given the size of the objects involved". >> > > > > I guess it's one of these cases which trigger undefined behavior and >> > > > > compiler can e.g. remove all of this code assuming it will be never >> > > > > called at runtime and any branches leading to it will always branch in >> > > > > other directions, or something. >> > > > >> > > > Right that sort of makes sense, and I can imagine that in more general >> > > > cases the struct casting could lead to UB. But what has me scratching >> > > > my head is that syzbot couldn't reproduce. The cast happens every >> > > > time. What about that one time was special? Did the address happen to >> > > > fall on the border of a mapping? Is UBSAN non-deterministic as an >> > > > optimization? Or is there actually some mysterious UaF happening with >> > > > my usage of skbs that I shouldn't overlook? >> > > >> > > These UBSAN checks were just enabled recently. >> > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: >> > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d >> > > So it's one of the top crashers by now. >> > >> > Ahh, makes sense. So it is easily reproducible after all. >> > >> > You're still of the opinion that it's a false positive, right? I >> > shouldn't spend more cycles on this? >> >> No, I am not saying this is a false positive. I think it's an >> undefined behavior. >> >> Either way, we need to resolve this one way or another to unblock >> testing the rest of the kernel, if not with a fix to wg, then with a >> fix to ubsan, or disable this check for kernel if kernel community >> decides we want to use and keep this type of C undefined behavior in >> the code base intentionally. >> So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard: >> https://syzkaller.appspot.com/upstream >> So cleaning them up looks doable. Is there a way to restructure the >> code to not invoke undefined behavior? > > > Right; that's my question as well. > >> >> Kees, do you have any suggestions on how to proceed? This seems to >> stop testing of the whole kernel at the moment. > > > If it's blocking other stuff and there isn't a path to fixing it soon, then I think we'll need to disable this check (and open an issue to track it). Oh, I see, the code is actually in skbuff.h: static inline void __skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk) { __skb_queue_before(list, (struct sk_buff *)list, newsk); } It casts sk_buff_head to sk_buff relying on equal layout of some prefix of these structs. Is it really UB in C? UBSAN docs say: "An attempt to potentially use bytes which the optimizer can determine are not part of the object being accessed". But C has POD layout for structs, right? These next/prev fields are within sk_buff_head (otherwise things would explode). I can imagine this may be not valid in C++, can this UBSAN check be C++-specific? Or at least some subset of this check, I can imagine it can detect bad bugs in C as well where things go really wrong. If there is no quick solution proposed, I tend to disable this check in syzbot for now. We need to clean at least common things like sk_buff first. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-09 9:46 ` Dmitry Vyukov (?) @ 2021-01-09 10:49 ` Matthias Urlichs -1 siblings, 0 replies; 39+ messages in thread From: Matthias Urlichs @ 2021-01-09 10:49 UTC (permalink / raw) To: wireguard [-- Attachment #1.1: Type: text/plain, Size: 244 bytes --] On 09.01.21 10:46, Dmitry Vyukov wrote: > It casts sk_buff_head to sk_buff relying on equal layout of some > prefix of these structs. This kind of construct is used with about all kernel lists ever, so … -- -- Matthias Urlichs [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-09 9:46 ` Dmitry Vyukov @ 2021-01-11 17:17 ` Dmitry Vyukov -1 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-11 17:17 UTC (permalink / raw) To: Kees Cook Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Sat, Jan 9, 2021 at 10:46 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Fri, Jan 8, 2021 at 9:26 PM Kees Cook <keescook@chromium.org> wrote: > >> On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >> > > > > >> > > > Hi Dmitry, > >> > > > > >> > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > >> > > > > Hi Jason, > >> > > > > > >> > > > > Thanks for looking into this. > >> > > > > > >> > > > > Reading clang docs for ubsan: > >> > > > > > >> > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > >> > > > > -fsanitize=object-size: An attempt to potentially use bytes which the > >> > > > > optimizer can determine are not part of the object being accessed. > >> > > > > This will also detect some types of undefined behavior that may not > >> > > > > directly access memory, but are provably incorrect given the size of > >> > > > > the objects involved, such as invalid downcasts and calling methods on > >> > > > > invalid pointers. These checks are made in terms of > >> > > > > __builtin_object_size, and consequently may be able to detect more > >> > > > > problems at higher optimization levels. > >> > > > > > >> > > > > From skimming though your description this seems to fall into > >> > > > > "provably incorrect given the size of the objects involved". > >> > > > > I guess it's one of these cases which trigger undefined behavior and > >> > > > > compiler can e.g. remove all of this code assuming it will be never > >> > > > > called at runtime and any branches leading to it will always branch in > >> > > > > other directions, or something. > >> > > > > >> > > > Right that sort of makes sense, and I can imagine that in more general > >> > > > cases the struct casting could lead to UB. But what has me scratching > >> > > > my head is that syzbot couldn't reproduce. The cast happens every > >> > > > time. What about that one time was special? Did the address happen to > >> > > > fall on the border of a mapping? Is UBSAN non-deterministic as an > >> > > > optimization? Or is there actually some mysterious UaF happening with > >> > > > my usage of skbs that I shouldn't overlook? > >> > > > >> > > These UBSAN checks were just enabled recently. > >> > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > >> > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > >> > > So it's one of the top crashers by now. > >> > > >> > Ahh, makes sense. So it is easily reproducible after all. > >> > > >> > You're still of the opinion that it's a false positive, right? I > >> > shouldn't spend more cycles on this? > >> > >> No, I am not saying this is a false positive. I think it's an > >> undefined behavior. > >> > >> Either way, we need to resolve this one way or another to unblock > >> testing the rest of the kernel, if not with a fix to wg, then with a > >> fix to ubsan, or disable this check for kernel if kernel community > >> decides we want to use and keep this type of C undefined behavior in > >> the code base intentionally. > >> So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard: > >> https://syzkaller.appspot.com/upstream > >> So cleaning them up looks doable. Is there a way to restructure the > >> code to not invoke undefined behavior? > > > > > > Right; that's my question as well. > > > >> > >> Kees, do you have any suggestions on how to proceed? This seems to > >> stop testing of the whole kernel at the moment. > > > > > > If it's blocking other stuff and there isn't a path to fixing it soon, then I think we'll need to disable this check (and open an issue to track it). > > Oh, I see, the code is actually in skbuff.h: > > static inline void __skb_queue_tail(struct sk_buff_head *list, struct > sk_buff *newsk) > { > __skb_queue_before(list, (struct sk_buff *)list, newsk); > } > > It casts sk_buff_head to sk_buff relying on equal layout of some > prefix of these structs. > Is it really UB in C? UBSAN docs say: > "An attempt to potentially use bytes which the optimizer can determine > are not part of the object being accessed". > But C has POD layout for structs, right? These next/prev fields are > within sk_buff_head (otherwise things would explode). > I can imagine this may be not valid in C++, can this UBSAN check be > C++-specific? Or at least some subset of this check, I can imagine it > can detect bad bugs in C as well where things go really wrong. > > If there is no quick solution proposed, I tend to disable this check > in syzbot for now. We need to clean at least common things like > sk_buff first. FTR, I've disabled the following UBSAN configs: UBSAN_MISC UBSAN_DIV_ZERO UBSAN_BOOL UBSAN_OBJECT_SIZE UBSAN_SIGNED_OVERFLOW UBSAN_UNSIGNED_OVERFLOW UBSAN_ENUM UBSAN_ALIGNMENT UBSAN_UNREACHABLE Only these are enabled now: UBSAN_BOUNDS UBSAN_SHIFT This is commit: https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-11 17:17 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-11 17:17 UTC (permalink / raw) To: Kees Cook Cc: Jason A. Donenfeld, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Sat, Jan 9, 2021 at 10:46 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Fri, Jan 8, 2021 at 9:26 PM Kees Cook <keescook@chromium.org> wrote: > >> On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >> > > > > >> > > > Hi Dmitry, > >> > > > > >> > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > >> > > > > Hi Jason, > >> > > > > > >> > > > > Thanks for looking into this. > >> > > > > > >> > > > > Reading clang docs for ubsan: > >> > > > > > >> > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > >> > > > > -fsanitize=object-size: An attempt to potentially use bytes which the > >> > > > > optimizer can determine are not part of the object being accessed. > >> > > > > This will also detect some types of undefined behavior that may not > >> > > > > directly access memory, but are provably incorrect given the size of > >> > > > > the objects involved, such as invalid downcasts and calling methods on > >> > > > > invalid pointers. These checks are made in terms of > >> > > > > __builtin_object_size, and consequently may be able to detect more > >> > > > > problems at higher optimization levels. > >> > > > > > >> > > > > From skimming though your description this seems to fall into > >> > > > > "provably incorrect given the size of the objects involved". > >> > > > > I guess it's one of these cases which trigger undefined behavior and > >> > > > > compiler can e.g. remove all of this code assuming it will be never > >> > > > > called at runtime and any branches leading to it will always branch in > >> > > > > other directions, or something. > >> > > > > >> > > > Right that sort of makes sense, and I can imagine that in more general > >> > > > cases the struct casting could lead to UB. But what has me scratching > >> > > > my head is that syzbot couldn't reproduce. The cast happens every > >> > > > time. What about that one time was special? Did the address happen to > >> > > > fall on the border of a mapping? Is UBSAN non-deterministic as an > >> > > > optimization? Or is there actually some mysterious UaF happening with > >> > > > my usage of skbs that I shouldn't overlook? > >> > > > >> > > These UBSAN checks were just enabled recently. > >> > > It's indeed super easy to trigger: 133083 VMs were crashed on this already: > >> > > https://syzkaller.appspot.com/bug?extid=8f90d005ab2d22342b6d > >> > > So it's one of the top crashers by now. > >> > > >> > Ahh, makes sense. So it is easily reproducible after all. > >> > > >> > You're still of the opinion that it's a false positive, right? I > >> > shouldn't spend more cycles on this? > >> > >> No, I am not saying this is a false positive. I think it's an > >> undefined behavior. > >> > >> Either way, we need to resolve this one way or another to unblock > >> testing the rest of the kernel, if not with a fix to wg, then with a > >> fix to ubsan, or disable this check for kernel if kernel community > >> decides we want to use and keep this type of C undefined behavior in > >> the code base intentionally. > >> So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard: > >> https://syzkaller.appspot.com/upstream > >> So cleaning them up looks doable. Is there a way to restructure the > >> code to not invoke undefined behavior? > > > > > > Right; that's my question as well. > > > >> > >> Kees, do you have any suggestions on how to proceed? This seems to > >> stop testing of the whole kernel at the moment. > > > > > > If it's blocking other stuff and there isn't a path to fixing it soon, then I think we'll need to disable this check (and open an issue to track it). > > Oh, I see, the code is actually in skbuff.h: > > static inline void __skb_queue_tail(struct sk_buff_head *list, struct > sk_buff *newsk) > { > __skb_queue_before(list, (struct sk_buff *)list, newsk); > } > > It casts sk_buff_head to sk_buff relying on equal layout of some > prefix of these structs. > Is it really UB in C? UBSAN docs say: > "An attempt to potentially use bytes which the optimizer can determine > are not part of the object being accessed". > But C has POD layout for structs, right? These next/prev fields are > within sk_buff_head (otherwise things would explode). > I can imagine this may be not valid in C++, can this UBSAN check be > C++-specific? Or at least some subset of this check, I can imagine it > can detect bad bugs in C as well where things go really wrong. > > If there is no quick solution proposed, I tend to disable this check > in syzbot for now. We need to clean at least common things like > sk_buff first. FTR, I've disabled the following UBSAN configs: UBSAN_MISC UBSAN_DIV_ZERO UBSAN_BOOL UBSAN_OBJECT_SIZE UBSAN_SIGNED_OVERFLOW UBSAN_UNSIGNED_OVERFLOW UBSAN_ENUM UBSAN_ALIGNMENT UBSAN_UNREACHABLE Only these are enabled now: UBSAN_BOUNDS UBSAN_SHIFT This is commit: https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-11 17:17 ` Dmitry Vyukov @ 2021-01-11 17:35 ` Jeffrey Walton -1 siblings, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-11 17:35 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov <dvyukov@google.com> wrote: > ... > FTR, I've disabled the following UBSAN configs: > UBSAN_MISC > UBSAN_DIV_ZERO > UBSAN_BOOL > UBSAN_OBJECT_SIZE > UBSAN_SIGNED_OVERFLOW > UBSAN_UNSIGNED_OVERFLOW > UBSAN_ENUM > UBSAN_ALIGNMENT > UBSAN_UNREACHABLE > > Only these are enabled now: > UBSAN_BOUNDS > UBSAN_SHIFT > > This is commit: > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e I think the commit cut too deep. The overflows are important if folks are building with compilers other than GCC. The aligned data accesses are important on platforms like MIPS64 and Sparc64. Object size is important because it catches destination buffer overflows. I don't know what's in miscellaneous. There may be something useful in there. Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-11 17:35 ` Jeffrey Walton 0 siblings, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-11 17:35 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov <dvyukov@google.com> wrote: > ... > FTR, I've disabled the following UBSAN configs: > UBSAN_MISC > UBSAN_DIV_ZERO > UBSAN_BOOL > UBSAN_OBJECT_SIZE > UBSAN_SIGNED_OVERFLOW > UBSAN_UNSIGNED_OVERFLOW > UBSAN_ENUM > UBSAN_ALIGNMENT > UBSAN_UNREACHABLE > > Only these are enabled now: > UBSAN_BOUNDS > UBSAN_SHIFT > > This is commit: > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e I think the commit cut too deep. The overflows are important if folks are building with compilers other than GCC. The aligned data accesses are important on platforms like MIPS64 and Sparc64. Object size is important because it catches destination buffer overflows. I don't know what's in miscellaneous. There may be something useful in there. Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-11 17:35 ` Jeffrey Walton @ 2021-01-11 17:58 ` Dmitry Vyukov -1 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-11 17:58 UTC (permalink / raw) To: noloader; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 6:35 PM Jeffrey Walton <noloader@gmail.com> wrote: > > On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > ... > > FTR, I've disabled the following UBSAN configs: > > UBSAN_MISC > > UBSAN_DIV_ZERO > > UBSAN_BOOL > > UBSAN_OBJECT_SIZE > > UBSAN_SIGNED_OVERFLOW > > UBSAN_UNSIGNED_OVERFLOW > > UBSAN_ENUM > > UBSAN_ALIGNMENT > > UBSAN_UNREACHABLE > > > > Only these are enabled now: > > UBSAN_BOUNDS > > UBSAN_SHIFT > > > > This is commit: > > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e > > I think the commit cut too deep. > > The overflows are important if folks are building with compilers other than GCC. > > The aligned data accesses are important on platforms like MIPS64 and Sparc64. > > Object size is important because it catches destination buffer overflows. > > I don't know what's in miscellaneous. There may be something useful in there. Hi Jeff, See the commit for reasons why each of these is disabled. E.g. object size, somebody first needs to fix bugs like this one. While things like skbuff have these UBs on trivial workloads, there is no point in involving fuzzing and making it crash on this trivial bug all the time and stopping doing any other kernel testing as the result. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-11 17:58 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-11 17:58 UTC (permalink / raw) To: noloader; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 6:35 PM Jeffrey Walton <noloader@gmail.com> wrote: > > On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > ... > > FTR, I've disabled the following UBSAN configs: > > UBSAN_MISC > > UBSAN_DIV_ZERO > > UBSAN_BOOL > > UBSAN_OBJECT_SIZE > > UBSAN_SIGNED_OVERFLOW > > UBSAN_UNSIGNED_OVERFLOW > > UBSAN_ENUM > > UBSAN_ALIGNMENT > > UBSAN_UNREACHABLE > > > > Only these are enabled now: > > UBSAN_BOUNDS > > UBSAN_SHIFT > > > > This is commit: > > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e > > I think the commit cut too deep. > > The overflows are important if folks are building with compilers other than GCC. > > The aligned data accesses are important on platforms like MIPS64 and Sparc64. > > Object size is important because it catches destination buffer overflows. > > I don't know what's in miscellaneous. There may be something useful in there. Hi Jeff, See the commit for reasons why each of these is disabled. E.g. object size, somebody first needs to fix bugs like this one. While things like skbuff have these UBs on trivial workloads, there is no point in involving fuzzing and making it crash on this trivial bug all the time and stopping doing any other kernel testing as the result. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-11 17:58 ` Dmitry Vyukov @ 2021-01-11 18:14 ` Jeffrey Walton -1 siblings, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-11 18:14 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 12:58 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Mon, Jan 11, 2021 at 6:35 PM Jeffrey Walton <noloader@gmail.com> wrote: > > > > On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > ... > > > FTR, I've disabled the following UBSAN configs: > > > UBSAN_MISC > > > UBSAN_DIV_ZERO > > > UBSAN_BOOL > > > UBSAN_OBJECT_SIZE > > > UBSAN_SIGNED_OVERFLOW > > > UBSAN_UNSIGNED_OVERFLOW > > > UBSAN_ENUM > > > UBSAN_ALIGNMENT > > > UBSAN_UNREACHABLE > > > > > > Only these are enabled now: > > > UBSAN_BOUNDS > > > UBSAN_SHIFT > > > > > > This is commit: > > > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e > > > > I think the commit cut too deep. > > > > The overflows are important if folks are building with compilers other than GCC. > > > > The aligned data accesses are important on platforms like MIPS64 and Sparc64. > > > > Object size is important because it catches destination buffer overflows. > > > > I don't know what's in miscellaneous. There may be something useful in there. > > Hi Jeff, > > See the commit for reasons why each of these is disabled. > E.g. object size, somebody first needs to fix bugs like this one. > While things like skbuff have these UBs on trivial workloads, there is > no point in involving fuzzing and making it crash on this trivial bug > all the time and stopping doing any other kernel testing as the > result. Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE? It seems to me object size checking is being conflated with object type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for actual object sizes, and UBSAN_OBJECT_TYPE for the casts. I still have a bitter taste in my mouth from https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html. I hate to see buffer checks go away. (And I realize the kernel folks are more skilled than the guy who wrote libupnp). Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-11 18:14 ` Jeffrey Walton 0 siblings, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-11 18:14 UTC (permalink / raw) To: Dmitry Vyukov; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 12:58 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > On Mon, Jan 11, 2021 at 6:35 PM Jeffrey Walton <noloader@gmail.com> wrote: > > > > On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov <dvyukov@google.com> wrote: > > > ... > > > FTR, I've disabled the following UBSAN configs: > > > UBSAN_MISC > > > UBSAN_DIV_ZERO > > > UBSAN_BOOL > > > UBSAN_OBJECT_SIZE > > > UBSAN_SIGNED_OVERFLOW > > > UBSAN_UNSIGNED_OVERFLOW > > > UBSAN_ENUM > > > UBSAN_ALIGNMENT > > > UBSAN_UNREACHABLE > > > > > > Only these are enabled now: > > > UBSAN_BOUNDS > > > UBSAN_SHIFT > > > > > > This is commit: > > > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e > > > > I think the commit cut too deep. > > > > The overflows are important if folks are building with compilers other than GCC. > > > > The aligned data accesses are important on platforms like MIPS64 and Sparc64. > > > > Object size is important because it catches destination buffer overflows. > > > > I don't know what's in miscellaneous. There may be something useful in there. > > Hi Jeff, > > See the commit for reasons why each of these is disabled. > E.g. object size, somebody first needs to fix bugs like this one. > While things like skbuff have these UBs on trivial workloads, there is > no point in involving fuzzing and making it crash on this trivial bug > all the time and stopping doing any other kernel testing as the > result. Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE? It seems to me object size checking is being conflated with object type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for actual object sizes, and UBSAN_OBJECT_TYPE for the casts. I still have a bitter taste in my mouth from https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html. I hate to see buffer checks go away. (And I realize the kernel folks are more skilled than the guy who wrote libupnp). Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-11 18:14 ` Jeffrey Walton @ 2021-01-12 9:54 ` Dmitry Vyukov -1 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-12 9:54 UTC (permalink / raw) To: noloader; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 7:15 PM Jeffrey Walton <noloader@gmail.com> wrote: > > > > ... > > > > FTR, I've disabled the following UBSAN configs: > > > > UBSAN_MISC > > > > UBSAN_DIV_ZERO > > > > UBSAN_BOOL > > > > UBSAN_OBJECT_SIZE > > > > UBSAN_SIGNED_OVERFLOW > > > > UBSAN_UNSIGNED_OVERFLOW > > > > UBSAN_ENUM > > > > UBSAN_ALIGNMENT > > > > UBSAN_UNREACHABLE > > > > > > > > Only these are enabled now: > > > > UBSAN_BOUNDS > > > > UBSAN_SHIFT > > > > > > > > This is commit: > > > > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e > > > > > > I think the commit cut too deep. > > > > > > The overflows are important if folks are building with compilers other than GCC. > > > > > > The aligned data accesses are important on platforms like MIPS64 and Sparc64. > > > > > > Object size is important because it catches destination buffer overflows. > > > > > > I don't know what's in miscellaneous. There may be something useful in there. > > > > Hi Jeff, > > > > See the commit for reasons why each of these is disabled. > > E.g. object size, somebody first needs to fix bugs like this one. > > While things like skbuff have these UBs on trivial workloads, there is > > no point in involving fuzzing and making it crash on this trivial bug > > all the time and stopping doing any other kernel testing as the > > result. > > Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE? > > It seems to me object size checking is being conflated with object > type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for > actual object sizes, and UBSAN_OBJECT_TYPE for the casts. > > I still have a bitter taste in my mouth from > https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html. > I hate to see buffer checks go away. (And I realize the kernel folks > are more skilled than the guy who wrote libupnp). > > Jeff I've filed https://bugs.llvm.org/show_bug.cgi?id=48726 for this. Does it capture what you are asking? Let's move the discussion re ubsan there. However, in the first place I am suggesting fixing the code. E.g. for sk_buff I would assume it's relatively easily fixable. A formally legal fix I think should put sk_buff_head into sk_buff and use it, no downsides and will eliminate the confusing "should go first" comments. Or as an workaround maybe we could make __skb_queue_before accept sk_buff_head and cast the other way around. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit @ 2021-01-12 9:54 ` Dmitry Vyukov 0 siblings, 0 replies; 39+ messages in thread From: Dmitry Vyukov @ 2021-01-12 9:54 UTC (permalink / raw) To: noloader; +Cc: Netdev, syzkaller-bugs, WireGuard mailing list On Mon, Jan 11, 2021 at 7:15 PM Jeffrey Walton <noloader@gmail.com> wrote: > > > > ... > > > > FTR, I've disabled the following UBSAN configs: > > > > UBSAN_MISC > > > > UBSAN_DIV_ZERO > > > > UBSAN_BOOL > > > > UBSAN_OBJECT_SIZE > > > > UBSAN_SIGNED_OVERFLOW > > > > UBSAN_UNSIGNED_OVERFLOW > > > > UBSAN_ENUM > > > > UBSAN_ALIGNMENT > > > > UBSAN_UNREACHABLE > > > > > > > > Only these are enabled now: > > > > UBSAN_BOUNDS > > > > UBSAN_SHIFT > > > > > > > > This is commit: > > > > https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e > > > > > > I think the commit cut too deep. > > > > > > The overflows are important if folks are building with compilers other than GCC. > > > > > > The aligned data accesses are important on platforms like MIPS64 and Sparc64. > > > > > > Object size is important because it catches destination buffer overflows. > > > > > > I don't know what's in miscellaneous. There may be something useful in there. > > > > Hi Jeff, > > > > See the commit for reasons why each of these is disabled. > > E.g. object size, somebody first needs to fix bugs like this one. > > While things like skbuff have these UBs on trivial workloads, there is > > no point in involving fuzzing and making it crash on this trivial bug > > all the time and stopping doing any other kernel testing as the > > result. > > Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE? > > It seems to me object size checking is being conflated with object > type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for > actual object sizes, and UBSAN_OBJECT_TYPE for the casts. > > I still have a bitter taste in my mouth from > https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html. > I hate to see buffer checks go away. (And I realize the kernel folks > are more skilled than the guy who wrote libupnp). > > Jeff I've filed https://bugs.llvm.org/show_bug.cgi?id=48726 for this. Does it capture what you are asking? Let's move the discussion re ubsan there. However, in the first place I am suggesting fixing the code. E.g. for sk_buff I would assume it's relatively easily fixable. A formally legal fix I think should put sk_buff_head into sk_buff and use it, no downsides and will eliminate the confusing "should go first" comments. Or as an workaround maybe we could make __skb_queue_before accept sk_buff_head and cast the other way around. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2020-12-21 11:23 ` Jason A. Donenfeld (?) (?) @ 2021-01-07 12:53 ` Jeffrey Walton -1 siblings, 0 replies; 39+ messages in thread From: Jeffrey Walton @ 2021-01-07 12:53 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Mon, Dec 21, 2020 at 6:24 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Dmitry, > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@google.com> wrote: > > Hi Jason, > > > > Thanks for looking into this. > > > > Reading clang docs for ubsan: > > > > https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html > > -fsanitize=object-size: An attempt to potentially use bytes which the > > optimizer can determine are not part of the object being accessed. > > This will also detect some types of undefined behavior that may not > > directly access memory, but are provably incorrect given the size of > > the objects involved, such as invalid downcasts and calling methods on > > invalid pointers. These checks are made in terms of > > __builtin_object_size, and consequently may be able to detect more > > problems at higher optimization levels. > > > > From skimming though your description this seems to fall into > > "provably incorrect given the size of the objects involved". > > I guess it's one of these cases which trigger undefined behavior and > > compiler can e.g. remove all of this code assuming it will be never > > called at runtime and any branches leading to it will always branch in > > other directions, or something. > > Right that sort of makes sense, and I can imagine that in more general > cases the struct casting could lead to UB. But what has me scratching > my head is that syzbot couldn't reproduce. The cast happens every > time. What about that one time was special? Did the address happen to > fall on the border of a mapping? Is UBSAN non-deterministic as an > optimization? Or is there actually some mysterious UaF happening with > my usage of skbs that I shouldn't overlook? The object size checker depends upon compiler analysis. If the compiler can determine the destination buffer size, then the compiler can insert a call to a safer function, like a safer memcpy. If the compiler cannot determine the destination buffer size, then the compiler will not insert a call to a safer function. (And Wireguard won't see the crash). Note... The object size checker and use of safer functions when the compiler can determine destination buffer sizes is quasi-automatic use of the safer memory functions from TR 24731-1. They are the ones the Glibc folks refuse to provide to developers. Jeff ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2020-12-21 11:23 ` Jason A. Donenfeld ` (2 preceding siblings ...) (?) @ 2021-01-07 17:01 ` Julian Wiedmann 2021-01-07 18:58 ` Jason A. Donenfeld -1 siblings, 1 reply; 39+ messages in thread From: Julian Wiedmann @ 2021-01-07 17:01 UTC (permalink / raw) To: Jason A. Donenfeld, Dmitry Vyukov Cc: Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On 21.12.20 12:23, Jason A. Donenfeld wrote: > Hi Dmitry, > ... > fall on the border of a mapping? Is UBSAN non-deterministic as an > optimization? Or is there actually some mysterious UaF happening with > my usage of skbs that I shouldn't overlook? > One oddity is that wg_xmit() returns negative errnos, rather than a netdev_tx_t (ie. NETDEV_TX_OK or NETDEV_TX_BUSY). Any chance that the stack mis-interprets one of those custom errnos as NETDEV_TX_BUSY, and thus believes that it still owns the skb? > Jason > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: UBSAN: object-size-mismatch in wg_xmit 2021-01-07 17:01 ` Julian Wiedmann @ 2021-01-07 18:58 ` Jason A. Donenfeld 0 siblings, 0 replies; 39+ messages in thread From: Jason A. Donenfeld @ 2021-01-07 18:58 UTC (permalink / raw) To: Julian Wiedmann Cc: Dmitry Vyukov, Kees Cook, Netdev, syzkaller-bugs, WireGuard mailing list, Eric Dumazet On Thu, Jan 7, 2021 at 6:02 PM Julian Wiedmann <jwi@linux.ibm.com> wrote: > > On 21.12.20 12:23, Jason A. Donenfeld wrote: > > Hi Dmitry, > > > > ... > > > fall on the border of a mapping? Is UBSAN non-deterministic as an > > optimization? Or is there actually some mysterious UaF happening with > > my usage of skbs that I shouldn't overlook? > > > > One oddity is that wg_xmit() returns negative errnos, rather than a > netdev_tx_t (ie. NETDEV_TX_OK or NETDEV_TX_BUSY). > > Any chance that the stack mis-interprets one of those custom errnos > as NETDEV_TX_BUSY, and thus believes that it still owns the skb? The stack trace shows the splat happening as a result of __skb_queue_tail, called from wg_xmit, not something that happens after wg_xmit returns. ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2021-01-12 9:55 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-20 16:54 UBSAN: object-size-mismatch in wg_xmit syzbot 2020-12-20 16:54 ` syzbot 2020-12-20 21:11 ` Jason A. Donenfeld 2020-12-20 21:11 ` Jason A. Donenfeld 2020-12-21 9:14 ` Dmitry Vyukov 2020-12-21 9:14 ` Dmitry Vyukov 2020-12-21 11:23 ` Jason A. Donenfeld 2020-12-21 11:23 ` Jason A. Donenfeld 2021-01-07 12:22 ` Dmitry Vyukov 2021-01-07 12:22 ` Dmitry Vyukov 2021-01-07 19:00 ` Jason A. Donenfeld 2021-01-07 19:06 ` Jeffrey Walton 2021-01-07 19:06 ` Jeffrey Walton 2021-01-08 0:34 ` Corey Costello 2021-01-08 0:42 ` Eric Light 2021-01-08 0:44 ` Corey Costello 2021-01-08 0:50 ` Eric Light 2021-01-08 1:02 ` Phillip McMahon 2021-01-08 9:33 ` Dmitry Vyukov 2021-01-08 9:33 ` Dmitry Vyukov 2021-01-08 20:54 ` Nathan Chancellor 2021-01-08 9:30 ` Dmitry Vyukov 2021-01-08 9:30 ` Dmitry Vyukov [not found] ` <CAGXu5j+jzmkiU_AWoTVF6e263iYSSJYUHB=Kdqh-MCfEO-aNSg@mail.gmail.com> 2021-01-09 9:46 ` Dmitry Vyukov 2021-01-09 9:46 ` Dmitry Vyukov 2021-01-09 10:49 ` Matthias Urlichs 2021-01-11 17:17 ` Dmitry Vyukov 2021-01-11 17:17 ` Dmitry Vyukov 2021-01-11 17:35 ` Jeffrey Walton 2021-01-11 17:35 ` Jeffrey Walton 2021-01-11 17:58 ` Dmitry Vyukov 2021-01-11 17:58 ` Dmitry Vyukov 2021-01-11 18:14 ` Jeffrey Walton 2021-01-11 18:14 ` Jeffrey Walton 2021-01-12 9:54 ` Dmitry Vyukov 2021-01-12 9:54 ` Dmitry Vyukov 2021-01-07 12:53 ` Jeffrey Walton 2021-01-07 17:01 ` Julian Wiedmann 2021-01-07 18:58 ` Jason A. Donenfeld
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.