netdev - Re: UBSAN: object-size-mismatch in wg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 20 Dec 2020 22:11:00 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Netdev <netdev@...r.kernel.org>
Cc:     syzkaller-bugs@...glegroups.com,
        WireGuard mailing list <wireguard@...ts.zx2c4.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: UBSAN: object-size-mismatch in wg_xmit

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@...kaller.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