On Sat, Jan 9, 2021 at 10:46 AM Dmitry Vyukov <[email protected]> wrote: > > On Fri, Jan 8, 2021 at 9:26 PM Kees Cook <[email protected]> wrote: > >> On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld <[email protected]> wrote: > >> > > > > >> > > > Hi Dmitry, > >> > > > > >> > > > On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov <[email protected]> > >> > > > 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
