Message-ID: <CACT4Y+b1nmsQBx=dD=Q9_y_GZx1PpqTbzR6j=u5UecQ0JLyMFg@mail.gmail.com>
Date:   Sat, 9 Jan 2021 10:46:02 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Netdev <netdev@...r.kernel.org>,
        syzkaller-bugs <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

On Fri, Jan 8, 2021 at 9:26 PM Kees Cook <keescook@...omium.org> wrote:
>> On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
>> > > >
>> > > > Hi Dmitry,
>> > > >
>> > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <dvyukov@...gle.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.

Powered by blists - more mailing lists