Re: UBSAN: object-size-mismatch in wg_xmit

[Dmitry Vyukov <dvyukov@google.com>]

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