Re: UBSAN: object-size-mismatch in wg_xmit

[Dmitry Vyukov <dvyukov@google.com>]

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.