* UBSAN: array-index-out-of-bounds in alg_bind @ 2020-10-16 8:12 syzbot 2020-10-17 3:49 ` Kees Cook 2020-11-02 2:17 ` UBSAN: array-index-out-of-bounds in alg_bind syzbot 0 siblings, 2 replies; 15+ messages in thread From: syzbot @ 2020-10-16 8:12 UTC (permalink / raw) To: davem, herbert, linux-crypto, linux-kernel, syzkaller-bugs Hello, syzbot found the following issue on: HEAD commit: 726eb70e Merge tag 'char-misc-5.10-rc1' of git://git.kerne.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=1011b678500000 kernel config: https://syzkaller.appspot.com/x/.config?x=89a0a83d1be17a89 dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81) Unfortunately, I don't have any reproducer for this issue yet. IMPORTANT: if you fix the issue, please add the following tag to the commit: 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]' CPU: 1 PID: 8236 Comm: syz-executor.0 Not tainted 5.9.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1d6/0x29e lib/dump_stack.c:118 ubsan_epilogue lib/ubsan.c:148 [inline] __ubsan_handle_out_of_bounds+0xdb/0x130 lib/ubsan.c:356 alg_bind+0x738/0x740 crypto/af_alg.c:166 __sys_bind+0x283/0x360 net/socket.c:1656 __do_sys_bind net/socket.c:1667 [inline] __se_sys_bind net/socket.c:1665 [inline] __x64_sys_bind+0x76/0x80 net/socket.c:1665 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x45de59 Code: 0d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 db b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f547948ec78 EFLAGS: 00000246 ORIG_RAX: 0000000000000031 RAX: ffffffffffffffda RBX: 0000000000000ac0 RCX: 000000000045de59 RDX: 0000000000000074 RSI: 0000000020000940 RDI: 0000000000000003 RBP: 000000000118bf60 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 000000000118bf2c R13: 00007ffd6121d5bf R14: 00007f547948f9c0 R15: 000000000118bf2c ================================================================================ Kernel panic - not syncing: panic_on_warn set ... CPU: 0 PID: 8236 Comm: syz-executor.0 Not tainted 5.9.0-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1d6/0x29e lib/dump_stack.c:118 panic+0x316/0x910 kernel/panic.c:231 ubsan_epilogue lib/ubsan.c:162 [inline] __ubsan_handle_out_of_bounds+0x12b/0x130 lib/ubsan.c:356 alg_bind+0x738/0x740 crypto/af_alg.c:166 __sys_bind+0x283/0x360 net/socket.c:1656 __do_sys_bind net/socket.c:1667 [inline] __se_sys_bind net/socket.c:1665 [inline] __x64_sys_bind+0x76/0x80 net/socket.c:1665 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x45de59 Code: 0d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 db b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f547948ec78 EFLAGS: 00000246 ORIG_RAX: 0000000000000031 RAX: ffffffffffffffda RBX: 0000000000000ac0 RCX: 000000000045de59 RDX: 0000000000000074 RSI: 0000000020000940 RDI: 0000000000000003 RBP: 000000000118bf60 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 000000000118bf2c R13: 00007ffd6121d5bf R14: 00007f547948f9c0 R15: 000000000118bf2c Kernel Offset: disabled Rebooting in 86400 seconds.. --- This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkaller@googlegroups.com. syzbot will keep track of this issue. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in alg_bind 2020-10-16 8:12 UBSAN: array-index-out-of-bounds in alg_bind syzbot @ 2020-10-17 3:49 ` Kees Cook 2020-10-17 6:20 ` Jann Horn 2020-10-17 10:50 ` Dmitry Vyukov 2020-11-02 2:17 ` UBSAN: array-index-out-of-bounds in alg_bind syzbot 1 sibling, 2 replies; 15+ messages in thread From: Kees Cook @ 2020-10-17 3:49 UTC (permalink / raw) To: herbert Cc: syzbot, linux-crypto, linux-kernel, syzkaller-bugs, linux-hardening, Elena Petrova 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@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. 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in alg_bind 2020-10-17 3:49 ` Kees Cook @ 2020-10-17 6:20 ` Jann Horn 2020-10-17 10:50 ` Dmitry Vyukov 1 sibling, 0 replies; 15+ messages in thread From: Jann Horn @ 2020-10-17 6:20 UTC (permalink / raw) To: Kees Cook Cc: herbert, syzbot, linux-crypto, kernel list, syzkaller-bugs, linux-hardening, Elena Petrova, Linux API +linux-api because this is about fixing UAPI without breaking userspace On Sat, Oct 17, 2020 at 8:02 AM Kees Cook <keescook@chromium.org> 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@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. 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. If you e.g. recompiled the wrong parts of the "btcheck" project with such changed UAPI headers, I think you'd get OOB writes, because they have this in a header (https://sources.debian.org/src/btcheck/2.1-4/src/kernelcryptoapi.h/?hl=29#L29): typedef struct { struct sockaddr_alg sa; int safd; int fd; } lkca_hash_ctx; so if you rebuilt e.g. kernelcryptoapi.o (which uses the struct) without also rebuilding hash.o (which allocates the struct), code in kernelcryptoapi.o would write beyond the end of lkca_hash_ctx, I think. Sure, there aren't many places that do this kind of thing for this struct. But at least in theory, you can't change the size of UAPI structs because someone might be passing instances of that struct around between object files. > 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). Yeah, the kernel should probably use a struct that looks different from the userspace one. :/ I guess we'll probably end up with some ugly hack with "#ifdef __KERNEL__", where the same struct has different sizes between kernel and userspace? Or am I being too puritan about UAPI consistency? > 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. I think you mean 88<x<=128 ? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in alg_bind 2020-10-17 3:49 ` Kees Cook 2020-10-17 6:20 ` Jann Horn @ 2020-10-17 10:50 ` Dmitry Vyukov 2020-10-17 11:02 ` Jann Horn 1 sibling, 1 reply; 15+ messages in thread From: Dmitry Vyukov @ 2020-10-17 10:50 UTC (permalink / raw) To: Kees Cook Cc: Herbert Xu, syzbot, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML, syzkaller-bugs, linux-hardening, Elena Petrova, Vegard Nossum On Sat, Oct 17, 2020 at 5:49 AM Kees Cook <keescook@chromium.org> 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@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!). 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@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/202010162042.7C51549A16%40keescook. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in alg_bind 2020-10-17 10:50 ` Dmitry Vyukov @ 2020-10-17 11:02 ` Jann Horn 2020-10-17 14:41 ` Dmitry Vyukov 0 siblings, 1 reply; 15+ messages in thread From: Jann Horn @ 2020-10-17 11:02 UTC (permalink / raw) To: Dmitry Vyukov Cc: Kees Cook, Herbert Xu, syzbot, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML, syzkaller-bugs, linux-hardening, Elena Petrova, Vegard Nossum, Gustavo A. R. Silva, Linux API On Sat, Oct 17, 2020 at 12:50 PM Dmitry Vyukov <dvyukov@google.com> wrote: > On Sat, Oct 17, 2020 at 5:49 AM Kees Cook <keescook@chromium.org> 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@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. > 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)? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in alg_bind 2020-10-17 11:02 ` Jann Horn @ 2020-10-17 14:41 ` Dmitry Vyukov 2020-10-26 20:07 ` [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name Eric Biggers 0 siblings, 1 reply; 15+ messages in thread From: Dmitry Vyukov @ 2020-10-17 14:41 UTC (permalink / raw) To: Jann Horn Cc: Kees Cook, Herbert Xu, syzbot, open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML, syzkaller-bugs, linux-hardening, Elena Petrova, Vegard Nossum, Gustavo A. R. Silva, Linux API On Sat, Oct 17, 2020 at 1:02 PM Jann Horn <jannh@google.com> wrote: > > > > 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. > > > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-17 14:41 ` Dmitry Vyukov @ 2020-10-26 20:07 ` Eric Biggers 2020-10-26 21:21 ` Gustavo A. R. Silva ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Eric Biggers @ 2020-10-26 20:07 UTC (permalink / raw) To: linux-crypto, Herbert Xu Cc: syzkaller-bugs, linux-hardening, linux-api, linux-kernel, Jann Horn, Kees Cook, Elena Petrova, Vegard Nossum, Gustavo A . R . Silva, stable, syzbot+92ead4eb8e26a26d465e From: Eric Biggers <ebiggers@google.com> Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names") made the kernel start accepting arbitrarily long algorithm names in sockaddr_alg. However, the actual length of the salg_name field stayed at the original 64 bytes. This is broken because the kernel can access indices >= 64 in salg_name, which is undefined behavior -- even though the memory that is accessed is still located within the sockaddr structure. It would only be defined behavior if the array were properly marked as arbitrary-length (either by making it a flexible array, which is the recommended way these days, or by making it an array of length 0 or 1). We can't simply change salg_name into a flexible array, since that would break source compatibility with userspace programs that embed sockaddr_alg into another struct, or (more commonly) declare a sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'. One solution would be to change salg_name into a flexible array only when '#ifdef __KERNEL__'. However, that would keep userspace without an easy way to actually use the longer algorithm names. Instead, add a new structure 'sockaddr_alg_new' that has the flexible array field, and expose it to both userspace and the kernel. Make the kernel use it correctly in alg_bind(). This addresses the syzbot report "UBSAN: array-index-out-of-bounds in alg_bind" (https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e). Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names") Cc: <stable@vger.kernel.org> # v4.12+ Signed-off-by: Eric Biggers <ebiggers@google.com> --- crypto/af_alg.c | 10 +++++++--- include/uapi/linux/if_alg.h | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/crypto/af_alg.c b/crypto/af_alg.c index d11db80d24cd1..9acb9d2c4bcf9 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY; struct sock *sk = sock->sk; struct alg_sock *ask = alg_sk(sk); - struct sockaddr_alg *sa = (void *)uaddr; + struct sockaddr_alg_new *sa = (void *)uaddr; const struct af_alg_type *type; void *private; int err; @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) if (sock->state == SS_CONNECTED) return -EINVAL; - if (addr_len < sizeof(*sa)) + BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) != + offsetof(struct sockaddr_alg, salg_name)); + BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa)); + + if (addr_len < sizeof(*sa) + 1) return -EINVAL; /* If caller uses non-allowed flag, return error. */ @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) return -EINVAL; sa->salg_type[sizeof(sa->salg_type) - 1] = 0; - sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0; + sa->salg_name[addr_len - sizeof(*sa) - 1] = 0; type = alg_get_type(sa->salg_type); if (PTR_ERR(type) == -ENOENT) { diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h index 60b7c2efd921c..dc52a11ba6d15 100644 --- a/include/uapi/linux/if_alg.h +++ b/include/uapi/linux/if_alg.h @@ -24,6 +24,22 @@ struct sockaddr_alg { __u8 salg_name[64]; }; +/* + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an + * arbitrary-length field. We had to keep the original struct above for source + * compatibility with existing userspace programs, though. Use the new struct + * below if support for very long algorithm names is needed. To do this, + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and + * copy algname (including the null terminator) into salg_name. + */ +struct sockaddr_alg_new { + __u16 salg_family; + __u8 salg_type[14]; + __u32 salg_feat; + __u32 salg_mask; + __u8 salg_name[]; +}; + struct af_alg_iv { __u32 ivlen; __u8 iv[0]; base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec -- 2.29.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 20:07 ` [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name Eric Biggers @ 2020-10-26 21:21 ` Gustavo A. R. Silva 2020-10-26 23:10 ` Gustavo A. R. Silva 2020-10-26 21:23 ` Jann Horn 2020-11-06 7:01 ` Herbert Xu 2 siblings, 1 reply; 15+ messages in thread From: Gustavo A. R. Silva @ 2020-10-26 21:21 UTC (permalink / raw) To: Eric Biggers Cc: linux-crypto, Herbert Xu, syzkaller-bugs, linux-hardening, linux-api, linux-kernel, Jann Horn, Kees Cook, Elena Petrova, Vegard Nossum, stable, syzbot+92ead4eb8e26a26d465e Hi, On Mon, Oct 26, 2020 at 01:07:15PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > names") made the kernel start accepting arbitrarily long algorithm names > in sockaddr_alg. However, the actual length of the salg_name field > stayed at the original 64 bytes. > > This is broken because the kernel can access indices >= 64 in salg_name, > which is undefined behavior -- even though the memory that is accessed > is still located within the sockaddr structure. It would only be > defined behavior if the array were properly marked as arbitrary-length > (either by making it a flexible array, which is the recommended way > these days, or by making it an array of length 0 or 1). > > We can't simply change salg_name into a flexible array, since that would > break source compatibility with userspace programs that embed > sockaddr_alg into another struct, or (more commonly) declare a > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'. > > One solution would be to change salg_name into a flexible array only > when '#ifdef __KERNEL__'. However, that would keep userspace without an > easy way to actually use the longer algorithm names. > > Instead, add a new structure 'sockaddr_alg_new' that has the flexible > array field, and expose it to both userspace and the kernel. > Make the kernel use it correctly in alg_bind(). > > This addresses the syzbot report > "UBSAN: array-index-out-of-bounds in alg_bind" > (https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e). > > Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com > Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names") > Cc: <stable@vger.kernel.org> # v4.12+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > crypto/af_alg.c | 10 +++++++--- > include/uapi/linux/if_alg.h | 16 ++++++++++++++++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/crypto/af_alg.c b/crypto/af_alg.c > index d11db80d24cd1..9acb9d2c4bcf9 100644 > --- a/crypto/af_alg.c > +++ b/crypto/af_alg.c > @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY; > struct sock *sk = sock->sk; > struct alg_sock *ask = alg_sk(sk); > - struct sockaddr_alg *sa = (void *)uaddr; > + struct sockaddr_alg_new *sa = (void *)uaddr; > const struct af_alg_type *type; > void *private; > int err; > @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > if (sock->state == SS_CONNECTED) > return -EINVAL; > > - if (addr_len < sizeof(*sa)) > + BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) != > + offsetof(struct sockaddr_alg, salg_name)); > + BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa)); > + > + if (addr_len < sizeof(*sa) + 1) > return -EINVAL; > > /* If caller uses non-allowed flag, return error. */ > @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > return -EINVAL; > > sa->salg_type[sizeof(sa->salg_type) - 1] = 0; > - sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0; > + sa->salg_name[addr_len - sizeof(*sa) - 1] = 0; > > type = alg_get_type(sa->salg_type); > if (PTR_ERR(type) == -ENOENT) { > diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h > index 60b7c2efd921c..dc52a11ba6d15 100644 > --- a/include/uapi/linux/if_alg.h > +++ b/include/uapi/linux/if_alg.h > @@ -24,6 +24,22 @@ struct sockaddr_alg { > __u8 salg_name[64]; > }; > > +/* > + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an > + * arbitrary-length field. We had to keep the original struct above for source > + * compatibility with existing userspace programs, though. Use the new struct > + * below if support for very long algorithm names is needed. To do this, > + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and > + * copy algname (including the null terminator) into salg_name. > + */ > +struct sockaddr_alg_new { > + __u16 salg_family; > + __u8 salg_type[14]; > + __u32 salg_feat; > + __u32 salg_mask; > + __u8 salg_name[]; > +}; > + How something like this, instead: struct sockaddr_alg { - __u16 salg_family; - __u8 salg_type[14]; - __u32 salg_feat; - __u32 salg_mask; - __u8 salg_name[64]; + union { + struct { + __u16 salg_v1_family; + __u8 salg_v1_type[14]; + __u32 salg_v1_feat; + __u32 salg_v1_mask; + __u8 salg_name[64]; + }; + struct { + __u16 salg_family; + __u8 salg_type[14]; + __u32 salg_feat; + __u32 salg_mask; + __u8 salg_name_new[]; + }; + }; }; -- Gustavo ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 21:21 ` Gustavo A. R. Silva @ 2020-10-26 23:10 ` Gustavo A. R. Silva 2020-10-26 23:40 ` Eric Biggers 0 siblings, 1 reply; 15+ messages in thread From: Gustavo A. R. Silva @ 2020-10-26 23:10 UTC (permalink / raw) To: Eric Biggers Cc: linux-crypto, Herbert Xu, syzkaller-bugs, linux-hardening, linux-api, linux-kernel, Jann Horn, Kees Cook, Elena Petrova, Vegard Nossum, stable, syzbot+92ead4eb8e26a26d465e On Mon, Oct 26, 2020 at 04:21:48PM -0500, Gustavo A. R. Silva wrote: > > +/* > > + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an > > + * arbitrary-length field. We had to keep the original struct above for source > > + * compatibility with existing userspace programs, though. Use the new struct > > + * below if support for very long algorithm names is needed. To do this, > > + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and > > + * copy algname (including the null terminator) into salg_name. > > + */ > > +struct sockaddr_alg_new { > > + __u16 salg_family; > > + __u8 salg_type[14]; > > + __u32 salg_feat; > > + __u32 salg_mask; > > + __u8 salg_name[]; > > +}; > > + > > How something like this, instead: > > struct sockaddr_alg { > - __u16 salg_family; > - __u8 salg_type[14]; > - __u32 salg_feat; > - __u32 salg_mask; > - __u8 salg_name[64]; > + union { > + struct { > + __u16 salg_v1_family; > + __u8 salg_v1_type[14]; > + __u32 salg_v1_feat; > + __u32 salg_v1_mask; > + __u8 salg_name[64]; > + }; > + struct { > + __u16 salg_family; > + __u8 salg_type[14]; > + __u32 salg_feat; > + __u32 salg_mask; > + __u8 salg_name_new[]; > + }; > + }; > }; > Something similar to the following approach might work: https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/uapi/gntalloc&id=db46c8aba41c436edb0b4ef2941bd7390b0e5d61 -- Gustavo ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 23:10 ` Gustavo A. R. Silva @ 2020-10-26 23:40 ` Eric Biggers 0 siblings, 0 replies; 15+ messages in thread From: Eric Biggers @ 2020-10-26 23:40 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: linux-crypto, Herbert Xu, syzkaller-bugs, linux-hardening, linux-api, linux-kernel, Jann Horn, Kees Cook, Elena Petrova, Vegard Nossum, stable, syzbot+92ead4eb8e26a26d465e On Mon, Oct 26, 2020 at 06:10:59PM -0500, Gustavo A. R. Silva wrote: > On Mon, Oct 26, 2020 at 04:21:48PM -0500, Gustavo A. R. Silva wrote: > > > +/* > > > + * Linux v4.12 and later removed the 64-byte limit on salg_name[]; it's now an > > > + * arbitrary-length field. We had to keep the original struct above for source > > > + * compatibility with existing userspace programs, though. Use the new struct > > > + * below if support for very long algorithm names is needed. To do this, > > > + * allocate 'sizeof(struct sockaddr_alg_new) + strlen(algname) + 1' bytes, and > > > + * copy algname (including the null terminator) into salg_name. > > > + */ > > > +struct sockaddr_alg_new { > > > + __u16 salg_family; > > > + __u8 salg_type[14]; > > > + __u32 salg_feat; > > > + __u32 salg_mask; > > > + __u8 salg_name[]; > > > +}; > > > + > > > > How something like this, instead: > > > > struct sockaddr_alg { > > - __u16 salg_family; > > - __u8 salg_type[14]; > > - __u32 salg_feat; > > - __u32 salg_mask; > > - __u8 salg_name[64]; > > + union { > > + struct { > > + __u16 salg_v1_family; > > + __u8 salg_v1_type[14]; > > + __u32 salg_v1_feat; > > + __u32 salg_v1_mask; > > + __u8 salg_name[64]; > > + }; > > + struct { > > + __u16 salg_family; > > + __u8 salg_type[14]; > > + __u32 salg_feat; > > + __u32 salg_mask; > > + __u8 salg_name_new[]; > > + }; > > + }; > > }; > > > > Something similar to the following approach might work: > > https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=testing/uapi/gntalloc&id=db46c8aba41c436edb0b4ef2941bd7390b0e5d61 > I suppose so. It's very confusing to see a union like that at first glance, though. It definitely needs an explanatory comment... - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 20:07 ` [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name Eric Biggers 2020-10-26 21:21 ` Gustavo A. R. Silva @ 2020-10-26 21:23 ` Jann Horn 2020-10-26 21:56 ` Eric Biggers 2020-11-06 7:01 ` Herbert Xu 2 siblings, 1 reply; 15+ messages in thread From: Jann Horn @ 2020-10-26 21:23 UTC (permalink / raw) To: Eric Biggers Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu, syzkaller-bugs, linux-hardening, Linux API, kernel list, Kees Cook, Elena Petrova, Vegard Nossum, Gustavo A . R . Silva, stable, syzbot On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > names") made the kernel start accepting arbitrarily long algorithm names > in sockaddr_alg. That's not true; it's still limited by the size of struct sockaddr_storage (128 bytes total for the entire address). If you make it longer, __copy_msghdr_from_user() will silently truncate the size. > This is broken because the kernel can access indices >= 64 in salg_name, > which is undefined behavior -- even though the memory that is accessed > is still located within the sockaddr structure. It would only be > defined behavior if the array were properly marked as arbitrary-length > (either by making it a flexible array, which is the recommended way > these days, or by making it an array of length 0 or 1). > > We can't simply change salg_name into a flexible array, since that would > break source compatibility with userspace programs that embed > sockaddr_alg into another struct, or (more commonly) declare a > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'. > > One solution would be to change salg_name into a flexible array only > when '#ifdef __KERNEL__'. However, that would keep userspace without an > easy way to actually use the longer algorithm names. > > Instead, add a new structure 'sockaddr_alg_new' that has the flexible > array field, and expose it to both userspace and the kernel. > Make the kernel use it correctly in alg_bind(). [...] > @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY; > struct sock *sk = sock->sk; > struct alg_sock *ask = alg_sk(sk); > - struct sockaddr_alg *sa = (void *)uaddr; > + struct sockaddr_alg_new *sa = (void *)uaddr; > const struct af_alg_type *type; > void *private; > int err; > @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > if (sock->state == SS_CONNECTED) > return -EINVAL; > > - if (addr_len < sizeof(*sa)) > + BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) != > + offsetof(struct sockaddr_alg, salg_name)); > + BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa)); > + > + if (addr_len < sizeof(*sa) + 1) > return -EINVAL; > > /* If caller uses non-allowed flag, return error. */ > @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > return -EINVAL; > > sa->salg_type[sizeof(sa->salg_type) - 1] = 0; > - sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0; > + sa->salg_name[addr_len - sizeof(*sa) - 1] = 0; This looks like an out-of-bounds write in the case `addr_len == sizeof(struct sockaddr_storage)`. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 21:23 ` Jann Horn @ 2020-10-26 21:56 ` Eric Biggers 2020-10-26 22:40 ` Jann Horn 0 siblings, 1 reply; 15+ messages in thread From: Eric Biggers @ 2020-10-26 21:56 UTC (permalink / raw) To: Jann Horn Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu, syzkaller-bugs, linux-hardening, Linux API, kernel list, Kees Cook, Elena Petrova, Vegard Nossum, Gustavo A . R . Silva, stable, syzbot On Mon, Oct 26, 2020 at 10:23:35PM +0100, 'Jann Horn' via syzkaller-bugs wrote: > On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > > names") made the kernel start accepting arbitrarily long algorithm names > > in sockaddr_alg. > > That's not true; it's still limited by the size of struct > sockaddr_storage (128 bytes total for the entire address). Interesting, so the actual limit is 104 bytes. It seems like the intent of that commit was to make it unlimited, though... > If you make it longer, __copy_msghdr_from_user() will silently truncate the > size. That's used for sys_sendmsg(), which AFAICT isn't relevant here. sockaddr_alg is used with sys_bind(), which fails with EINVAL if the address is longer than sizeof(struct sockaddr_storage). However, since sys_sendmsg() is truncating overly-long addresses, it's probably the case that sizeof(struct sockaddr_storage) can never be increased in the future... > > > This is broken because the kernel can access indices >= 64 in salg_name, > > which is undefined behavior -- even though the memory that is accessed > > is still located within the sockaddr structure. It would only be > > defined behavior if the array were properly marked as arbitrary-length > > (either by making it a flexible array, which is the recommended way > > these days, or by making it an array of length 0 or 1). > > > > We can't simply change salg_name into a flexible array, since that would > > break source compatibility with userspace programs that embed > > sockaddr_alg into another struct, or (more commonly) declare a > > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'. > > > > One solution would be to change salg_name into a flexible array only > > when '#ifdef __KERNEL__'. However, that would keep userspace without an > > easy way to actually use the longer algorithm names. > > > > Instead, add a new structure 'sockaddr_alg_new' that has the flexible > > array field, and expose it to both userspace and the kernel. > > Make the kernel use it correctly in alg_bind(). > [...] > > @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > > const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY; > > struct sock *sk = sock->sk; > > struct alg_sock *ask = alg_sk(sk); > > - struct sockaddr_alg *sa = (void *)uaddr; > > + struct sockaddr_alg_new *sa = (void *)uaddr; > > const struct af_alg_type *type; > > void *private; > > int err; > > @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > > if (sock->state == SS_CONNECTED) > > return -EINVAL; > > > > - if (addr_len < sizeof(*sa)) > > + BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) != > > + offsetof(struct sockaddr_alg, salg_name)); > > + BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa)); > > + > > + if (addr_len < sizeof(*sa) + 1) > > return -EINVAL; > > > > /* If caller uses non-allowed flag, return error. */ > > @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > > return -EINVAL; > > > > sa->salg_type[sizeof(sa->salg_type) - 1] = 0; > > - sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0; > > + sa->salg_name[addr_len - sizeof(*sa) - 1] = 0; > > This looks like an out-of-bounds write in the case `addr_len == > sizeof(struct sockaddr_storage)`. I think you mean addr_len == sizeof(*sa)? That's what the 'if (addr_len < sizeof(*sa) + 1) return -EINVAL' above is for. - Eric ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 21:56 ` Eric Biggers @ 2020-10-26 22:40 ` Jann Horn 0 siblings, 0 replies; 15+ messages in thread From: Jann Horn @ 2020-10-26 22:40 UTC (permalink / raw) To: Eric Biggers Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu, syzkaller-bugs, linux-hardening, Linux API, kernel list, Kees Cook, Elena Petrova, Vegard Nossum, Gustavo A . R . Silva, stable, syzbot On Mon, Oct 26, 2020 at 10:57 PM Eric Biggers <ebiggers@kernel.org> wrote: > On Mon, Oct 26, 2020 at 10:23:35PM +0100, 'Jann Horn' via syzkaller-bugs wrote: > > On Mon, Oct 26, 2020 at 9:08 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > > > names") made the kernel start accepting arbitrarily long algorithm names > > > in sockaddr_alg. > > > > That's not true; it's still limited by the size of struct > > sockaddr_storage (128 bytes total for the entire address). > > Interesting, so the actual limit is 104 bytes. It seems like the intent of that > commit was to make it unlimited, though... > > > If you make it longer, __copy_msghdr_from_user() will silently truncate the > > size. > > That's used for sys_sendmsg(), which AFAICT isn't relevant here. sockaddr_alg > is used with sys_bind(), which fails with EINVAL if the address is longer than > sizeof(struct sockaddr_storage). Ugh, of course you're right, sorry. > However, since sys_sendmsg() is truncating overly-long addresses, it's probably > the case that sizeof(struct sockaddr_storage) can never be increased in the > future... Eh, I think there'd probably be bigger issues with that elsewhere. > > > This is broken because the kernel can access indices >= 64 in salg_name, > > > which is undefined behavior -- even though the memory that is accessed > > > is still located within the sockaddr structure. It would only be > > > defined behavior if the array were properly marked as arbitrary-length > > > (either by making it a flexible array, which is the recommended way > > > these days, or by making it an array of length 0 or 1). > > > > > > We can't simply change salg_name into a flexible array, since that would > > > break source compatibility with userspace programs that embed > > > sockaddr_alg into another struct, or (more commonly) declare a > > > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'. > > > > > > One solution would be to change salg_name into a flexible array only > > > when '#ifdef __KERNEL__'. However, that would keep userspace without an > > > easy way to actually use the longer algorithm names. > > > > > > Instead, add a new structure 'sockaddr_alg_new' that has the flexible > > > array field, and expose it to both userspace and the kernel. > > > Make the kernel use it correctly in alg_bind(). > > [...] > > > @@ -147,7 +147,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > > > const u32 allowed = CRYPTO_ALG_KERN_DRIVER_ONLY; > > > struct sock *sk = sock->sk; > > > struct alg_sock *ask = alg_sk(sk); > > > - struct sockaddr_alg *sa = (void *)uaddr; > > > + struct sockaddr_alg_new *sa = (void *)uaddr; > > > const struct af_alg_type *type; > > > void *private; > > > int err; > > > @@ -155,7 +155,11 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > > > if (sock->state == SS_CONNECTED) > > > return -EINVAL; > > > > > > - if (addr_len < sizeof(*sa)) > > > + BUILD_BUG_ON(offsetof(struct sockaddr_alg_new, salg_name) != > > > + offsetof(struct sockaddr_alg, salg_name)); > > > + BUILD_BUG_ON(offsetof(struct sockaddr_alg, salg_name) != sizeof(*sa)); > > > + > > > + if (addr_len < sizeof(*sa) + 1) > > > return -EINVAL; > > > > > > /* If caller uses non-allowed flag, return error. */ > > > @@ -163,7 +167,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > > > return -EINVAL; > > > > > > sa->salg_type[sizeof(sa->salg_type) - 1] = 0; > > > - sa->salg_name[sizeof(sa->salg_name) + addr_len - sizeof(*sa) - 1] = 0; > > > + sa->salg_name[addr_len - sizeof(*sa) - 1] = 0; > > > > This looks like an out-of-bounds write in the case `addr_len == > > sizeof(struct sockaddr_storage)`. Sorry, I've been unusually unconcentrated today. Sorry about the noise, ignore what I said. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name 2020-10-26 20:07 ` [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name Eric Biggers 2020-10-26 21:21 ` Gustavo A. R. Silva 2020-10-26 21:23 ` Jann Horn @ 2020-11-06 7:01 ` Herbert Xu 2 siblings, 0 replies; 15+ messages in thread From: Herbert Xu @ 2020-11-06 7:01 UTC (permalink / raw) To: Eric Biggers Cc: linux-crypto, syzkaller-bugs, linux-hardening, linux-api, linux-kernel, Jann Horn, Kees Cook, Elena Petrova, Vegard Nossum, Gustavo A . R . Silva, stable, syzbot+92ead4eb8e26a26d465e On Mon, Oct 26, 2020 at 01:07:15PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Commit 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm > names") made the kernel start accepting arbitrarily long algorithm names > in sockaddr_alg. However, the actual length of the salg_name field > stayed at the original 64 bytes. > > This is broken because the kernel can access indices >= 64 in salg_name, > which is undefined behavior -- even though the memory that is accessed > is still located within the sockaddr structure. It would only be > defined behavior if the array were properly marked as arbitrary-length > (either by making it a flexible array, which is the recommended way > these days, or by making it an array of length 0 or 1). > > We can't simply change salg_name into a flexible array, since that would > break source compatibility with userspace programs that embed > sockaddr_alg into another struct, or (more commonly) declare a > sockaddr_alg like 'struct sockaddr_alg sa = { .salg_name = "foo" };'. > > One solution would be to change salg_name into a flexible array only > when '#ifdef __KERNEL__'. However, that would keep userspace without an > easy way to actually use the longer algorithm names. > > Instead, add a new structure 'sockaddr_alg_new' that has the flexible > array field, and expose it to both userspace and the kernel. > Make the kernel use it correctly in alg_bind(). > > This addresses the syzbot report > "UBSAN: array-index-out-of-bounds in alg_bind" > (https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e). > > Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com > Fixes: 3f69cc60768b ("crypto: af_alg - Allow arbitrarily long algorithm names") > Cc: <stable@vger.kernel.org> # v4.12+ > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > crypto/af_alg.c | 10 +++++++--- > include/uapi/linux/if_alg.h | 16 ++++++++++++++++ > 2 files changed, 23 insertions(+), 3 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: UBSAN: array-index-out-of-bounds in alg_bind 2020-10-16 8:12 UBSAN: array-index-out-of-bounds in alg_bind syzbot 2020-10-17 3:49 ` Kees Cook @ 2020-11-02 2:17 ` syzbot 1 sibling, 0 replies; 15+ messages in thread From: syzbot @ 2020-11-02 2:17 UTC (permalink / raw) To: davem, dvyukov, ebiggers, gustavoars, herbert, jannh, keescook, lenaptr, linux-api, linux-crypto, linux-hardening, linux-kernel, stable, syzkaller-bugs, vegard.nossum syzbot has found a reproducer for the following issue on: HEAD commit: 3cea11cd Linux 5.10-rc2 git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=1443bf92500000 kernel config: https://syzkaller.appspot.com/x/.config?x=e791ddf0875adf65 dashboard link: https://syzkaller.appspot.com/bug?extid=92ead4eb8e26a26d465e compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project.git ca2dcbd030eadbf0aa9b660efe864ff08af6e18b) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=145afc2c500000 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=141ad11a500000 IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+92ead4eb8e26a26d465e@syzkaller.appspotmail.com ================================================================================ UBSAN: array-index-out-of-bounds in crypto/af_alg.c:166:2 index 98 is out of range for type '__u8 [64]' CPU: 1 PID: 8468 Comm: syz-executor983 Not tainted 5.10.0-rc2-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x137/0x1be lib/dump_stack.c:118 ubsan_epilogue lib/ubsan.c:148 [inline] __ubsan_handle_out_of_bounds+0xdb/0x130 lib/ubsan.c:356 alg_bind+0x738/0x740 crypto/af_alg.c:166 __sys_bind+0x283/0x360 net/socket.c:1656 __do_sys_bind net/socket.c:1667 [inline] __se_sys_bind net/socket.c:1665 [inline] __x64_sys_bind+0x76/0x80 net/socket.c:1665 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x4402c9 Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007ffe05301528 EFLAGS: 00000246 ORIG_RAX: 0000000000000031 RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004402c9 RDX: 000000000000007b RSI: 00000000200000c0 RDI: 0000000000000003 RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401ad0 R13: 0000000000401b60 R14: 0000000000000000 R15: 0000000000000000 ================================================================================ Kernel panic - not syncing: panic_on_warn set ... CPU: 1 PID: 8468 Comm: syz-executor983 Not tainted 5.10.0-rc2-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x137/0x1be lib/dump_stack.c:118 panic+0x291/0x800 kernel/panic.c:231 ubsan_epilogue lib/ubsan.c:162 [inline] __ubsan_handle_out_of_bounds+0x12b/0x130 lib/ubsan.c:356 alg_bind+0x738/0x740 crypto/af_alg.c:166 __sys_bind+0x283/0x360 net/socket.c:1656 __do_sys_bind net/socket.c:1667 [inline] __se_sys_bind net/socket.c:1665 [inline] __x64_sys_bind+0x76/0x80 net/socket.c:1665 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x4402c9 Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 7b 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007ffe05301528 EFLAGS: 00000246 ORIG_RAX: 0000000000000031 RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 00000000004402c9 RDX: 000000000000007b RSI: 00000000200000c0 RDI: 0000000000000003 RBP: 00000000006ca018 R08: 0000000000000000 R09: 00000000004002c8 R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000401ad0 R13: 0000000000401b60 R14: 0000000000000000 R15: 0000000000000000 Kernel Offset: disabled Rebooting in 86400 seconds.. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-11-06 7:01 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-16 8:12 UBSAN: array-index-out-of-bounds in alg_bind syzbot 2020-10-17 3:49 ` Kees Cook 2020-10-17 6:20 ` Jann Horn 2020-10-17 10:50 ` Dmitry Vyukov 2020-10-17 11:02 ` Jann Horn 2020-10-17 14:41 ` Dmitry Vyukov 2020-10-26 20:07 ` [PATCH] crypto: af_alg - avoid undefined behavior accessing salg_name Eric Biggers 2020-10-26 21:21 ` Gustavo A. R. Silva 2020-10-26 23:10 ` Gustavo A. R. Silva 2020-10-26 23:40 ` Eric Biggers 2020-10-26 21:23 ` Jann Horn 2020-10-26 21:56 ` Eric Biggers 2020-10-26 22:40 ` Jann Horn 2020-11-06 7:01 ` Herbert Xu 2020-11-02 2:17 ` UBSAN: array-index-out-of-bounds in alg_bind syzbot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).