On 20.09.2019 16:22, Dmitry Vyukov wrote: > On Fri, Sep 20, 2019 at 2:58 PM Kamil Rytarowski <n54%gmx.com@localhost> wrote: >> >> On 20.09.2019 08:14, Dmitry Vyukov wrote: >>> On Fri, Sep 20, 2019 at 3:39 AM Kamil Rytarowski <n54%gmx.com@localhost> wrote: >>>> >>>> On 20.09.2019 00:24, syzbot wrote: >>>>> Hello, >>>>> >>>>> syzbot found the following crash on: >>>>> >>>>> HEAD commit: 338a6d82 Use an explicit run-time assertion where >>>>> compile-.. >>>>> git tree: netbsd >>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=1176d8e9600000 >>>>> kernel config: https://syzkaller.appspot.com/x/.config?x=824b23e1f4b6c76b >>>>> dashboard link: >>>>> https://syzkaller.appspot.com/bug?extid=b53a9bcf030288081e65 >>>>> >>>>> Unfortunately, I don't have any reproducer for this crash yet. >>>>> >>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit: >>>>> Reported-by: syzbot+b53a9bcf030288081e65%syzkaller.appspotmail.com@localhost >>>>> >>>>> [ 47.4625386] panic: UBSan: Undefined Behavior in >>>>> /syzkaller/managers/netbsd-kubsan/kernel/sys/netinet6/scope6.c:480:6, >>>>> member access within misaligned address 0xffff9457bc441286 for type >>>>> 'struct in6_addr' which requires 4 byte alignment >>>>> >>>> >>>> How to address these reports? There are numbers of them. >>>> >>>> The problem is that struct in6_addr is stored in __packed structs such >>>> as (ip6_hdr) in the kernel. I don't know any other way to fix it than >>>> marking in6_addr as __packed, but that changes ABI and breaks userland >>>> qemu for some reason. >>>> >>>> FreeBSD/OpenBSD have the same structs and attributes. >>>> >>>> Illumos and Linux don't use __packed for ip6_hdr, but it is probably too >>>> late to change our struct layout? >>>> >>>> Also I don't have enough confidence of the ipv6 algorithms. >>>> >>>> One variation (valid ?) is to put back __packed under ifdef _KERNEL: >>>> >>>> http://netbsd.org/~kamil/patch-00151-ip6addr.txt >>>> >>>> Alternatively mark [probably too] many ipv6 related functions with >>>> __noubsan. >>>> >>>> This is probably the last UBSan issue before getting syzbot to fuzz the >>>> kernel. >>> >>> FWIW one of official way to fix this in C/C++ is to declare a properly >>> aligned type (int) and copyout the data into it with memcpy. For x86 >>> compiler should understand that and eliminate the copy. >>> >> >> memcpy/memcmp is too invasive here as we need to change macros shared >> with userland. >> >> Is the ifdefed __packed patch correct? >> >> http://netbsd.org/~kamil/patch-00151-ip6addr.txt > > > This makes struct alignment 1, right? > Can't it break some ABIs? What if the struct is included in some other > struct and alignment 1 changes overall layout. Then usespace will pass > padded, but kernel will assume unpadded layout. Is it possible? > > If we are out of good ideas, it can make sense to add some __noubsan's > for very frequent and/or hard to address otherwise warnings. This will > unstack finding the long tail of potentially more interesting > warnings. And then these warnings can be addressed, and in parallel > the __noubsan's can be fixed in better ways (if we come up with better > ways, but now this won't be urgent because we put down the fires). > For the time being, I have marked it with __noubsan. I think I know how to fix it. There is need to disable __NO_STRICT_ALIGNMENT under UBSan. http://cdn.netbsd.org/pub/NetBSD/misc/joerg/GENERIC/src/src/sys/arch/amd64/include/types.h.html#_M/__NO_STRICT_ALIGNMENT This will allow to use alternative code-paths, like here: http://cdn.netbsd.org/pub/NetBSD/misc/joerg/GENERIC/src/src/sys/netinet6/ip6_input.c.html#315 My patch: http://netbsd.org/~kamil/patch-00152-__NO_STRICT_ALIGNMENT-disable-under-ubsan.txt This needs proper testing whether it really helps (hopefully yes). My GCC patch (still building): http://netbsd.org/~kamil/patch-00153-gcc-lsan-ubsan-ifdef.txt The LSAN change is related to Clang/LLVM: https://reviews.llvm.org/D67719 We need LSAN for our userland applications for suppressing uninteresting leaks. I will try to upstream them both in one go. If the GCC patch will be merged, I will commit patch 0152 and 0153 to src/.
Attachment:
signature.asc
Description: OpenPGP digital signature