Re: UBSAN: object-size-mismatch in wg_xmit

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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