On Mon, Jan 11, 2021 at 7:15 PM Jeffrey Walton <noloa...@gmail.com> wrote: > > > > ... > > > > 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 > > > > > > I think the commit cut too deep. > > > > > > The overflows are important if folks are building with compilers other > > > than GCC. > > > > > > The aligned data accesses are important on platforms like MIPS64 and > > > Sparc64. > > > > > > Object size is important because it catches destination buffer overflows. > > > > > > I don't know what's in miscellaneous. There may be something useful in > > > there. > > > > Hi Jeff, > > > > See the commit for reasons why each of these is disabled. > > E.g. object size, somebody first needs to fix bugs like this one. > > While things like skbuff have these UBs on trivial workloads, there is > > no point in involving fuzzing and making it crash on this trivial bug > > all the time and stopping doing any other kernel testing as the > > result. > > Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE? > > It seems to me object size checking is being conflated with object > type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for > actual object sizes, and UBSAN_OBJECT_TYPE for the casts. > > I still have a bitter taste in my mouth from > https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html. > I hate to see buffer checks go away. (And I realize the kernel folks > are more skilled than the guy who wrote libupnp). > > Jeff
I've filed https://bugs.llvm.org/show_bug.cgi?id=48726 for this. Does it capture what you are asking? Let's move the discussion re ubsan there. However, in the first place I am suggesting fixing the code. E.g. for sk_buff I would assume it's relatively easily fixable. A formally legal fix I think should put sk_buff_head into sk_buff and use it, no downsides and will eliminate the confusing "should go first" comments. Or as an workaround maybe we could make __skb_queue_before accept sk_buff_head and cast the other way around.
