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

[Dmitry Vyukov]

On Sat, Oct 17, 2020 at 5:49 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Oct 16, 2020 at 01:12:24AM -0700, syzbot 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!).

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.


> The origin appears to be that 3f69cc60768b
> ("crypto: af_alg - Allow arbitrarily long algorithm names") intentionally
> didn't extend the kernel structure (which is actually just using the UAPI
> structure). I don't see a reason the UAPI couldn't have been extended:
> it's a sockaddr implementation, so the size is always passed in as part
> of the existing API.
>
> At the very least the kernel needs to switch to using a correctly-sized
> structure: I expected UBSAN_BOUNDS to be enabled globally by default at
> some point in the future (with the minimal runtime -- the
> instrumentation is tiny and catches real issues).
>
> Reproduction:
>
> struct sockaddr_alg sa = {
>     .salg_family = AF_ALG,
>     .salg_type = "skcipher",
>     .salg_name = "cbc(aes)"
> };
> fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
> bind(fd, (void *)&sa, sizeof(sa));
>
> Replace "sizeof(sa)" with x where 64<x<=128.
>
> --
> Kees Cook
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/202010162042.7C51549A16%40keescook.