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

[Dmitry Vyukov]

On Sat, Oct 17, 2020 at 1:02 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e
> > > > [...]
> > > > Reported-by: syzbot+92ead4eb8e26a26d465e@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > [...]
> > > > 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.
>
> > 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.