Re: UBSAN: array-index-out-of-bounds in alg_bind

On Sat, Oct 17, 2020 at 1:02 PM Jann Horn [off-list ref] wrote:

quoted

quoted

quoted

dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
[...]
Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com
[...]
UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2
index 91 is out of range for type '__u8 [64]'

This seems to be an "as intended", if very odd. false positive (the actual
memory area is backed by the on-stack _K_SS_MAXSIZE-sized sockaddr_storage
"address" variable in __sys_bind. But yes, af_alg's salg_name member
size here doesn't make sense.

As Vegard noted elsewhere, compilers can start making assumptions
based on absence of UB and compile code in surprising ways as the
result leading to very serious and very real bugs.

One example would be a compiler generating jump table for common sizes
during PGO and leaving size > 64 as wild jump.

Another example would be a compiler assuming that copy size <= 64.
Then if there is another copy into a 64-byte buffer with a proper size
check, the compiler can now drop that size check (since it now knows
size <= 64) and we get real stack smash (for a copy that does have a
proper size check before!).

FWIW, the kernel currently still has a bunch of places that use
C89-style length-1 arrays (which were in the past used to work around
C89's lack of proper flexible arrays). Gustavo A. R. Silva has a bunch
of patches pending to change those places now, but those are not
marked for stable backporting; so in all currently released kernels,
we'll probably keep having length-1 arrays at the ends of C structs
that are used as if they were flexible arrays. (Unless someone makes
the case that these patches are not just cleanups but actually fix
some sort of real bug, and therefore need to be backported.)

The code in this example looks just like one of those C89-style
length-1 arrays to me (except that the length isn't 1).

Of course I do agree that this should be cleaned up, and that having
bogus array lengths in the source code is a bad idea.

quoted

And we do want compilers to be that smart today. Because of all levels
of abstractions/macros/inlining we actually have lots of
redundant/nonsensical code in the end after all inlining and
expansions, and we do want compilers to infer things, remove redundant
checks, etc so that we can have both nice abstract source code and
efficient machine code at the same time.

I guess that kinda leads to the question: Do we just need to fix the
kernel code here (which is comparatively easy), or do you think that
this is a sufficiently big problem that we need to go and somehow
change the actual UAPI headers here (e.g. by deprecating the existing
UAPI struct and making a new one with a different name)?

Good question. What I wrote is not based on some concrete
miscompilation at hand. I just meant that there are more things
involved that may appear at first glance.

Re proactively fixing UAPI, I would say if somebody is up to doing it
now, I would say it's good and a right change. Otherwise delaying
fixing it is also a reasonable strategy because (1) there are probably
more such cases, (2) any work on enabling more optimizations, global
optimizations, etc is only feasible if there is a tool that helps to
identify all places that need to be fixed. So whoever/whenever will be
fixing this, one more or one less case probably does not matter much.
It's a different story if there is already a tool/compiler warning
that traps on some code and that code harms deployment of the tool.