Re: panic: UBSan: Undefined Behavior in /syzkaller/managers/netbsd-kubsan/kernel/sys/netinet6/scope6.c:LINE, member access w

[Dmitry Vyukov <dvyukov%google.com@localhost>]

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).