Re: UBSAN: object-size-mismatch in wg_xmit

On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld [off-list ref] wrote:

quoted

quoted

Hi Dmitry,

On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov [off-list ref] wrote:

quoted

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?
Kees, do you have any suggestions on how to proceed? This seems to
stop testing of the whole kernel at the moment.