LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* KASAN: slab-out-of-bounds Write in sha1_finup
@ 2018-06-07 13:07 syzbot
  2018-06-07 14:43 ` syzbot
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: syzbot @ 2018-06-07 13:07 UTC (permalink / raw)
  To: davem, herbert, hpa, linux-crypto, linux-kernel, mingo,
	syzkaller-bugs, tglx, x86

Hello,

syzbot found the following crash on:

HEAD commit:    1c8c5a9d38f6 Merge git://git.kernel.org/pub/scm/linux/kern..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=16289b9f800000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2e1a31e8576e013a
dashboard link: https://syzkaller.appspot.com/bug?extid=486f97f892efeb2075a3
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
userspace arch: i386
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=122bb5df800000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+486f97f892efeb2075a3@syzkaller.appspotmail.com

IPv6: ADDRCONF(NETDEV_UP): veth0: link is not ready
IPv6: ADDRCONF(NETDEV_CHANGE): veth0: link becomes ready
IPv6: ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
8021q: adding VLAN 0 to HW filter on device team0
==================================================================
BUG: KASAN: slab-out-of-bounds in put_unaligned_be32  
include/linux/unaligned/access_ok.h:60 [inline]
BUG: KASAN: slab-out-of-bounds in sha1_base_finish  
include/crypto/sha1_base.h:102 [inline]
BUG: KASAN: slab-out-of-bounds in sha1_finup+0x44e/0x4b0  
arch/x86/crypto/sha1_ssse3_glue.c:70
Write of size 4 at addr ffff8801d7130ed8 by task syz-executor0/4761

CPU: 0 PID: 4761 Comm: syz-executor0 Not tainted 4.17.0+ #113
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+0x1b9/0x294 lib/dump_stack.c:113
  print_address_description+0x6c/0x20b mm/kasan/report.c:256
  kasan_report_error mm/kasan/report.c:354 [inline]
  kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
  __asan_report_store4_noabort+0x17/0x20 mm/kasan/report.c:437
  put_unaligned_be32 include/linux/unaligned/access_ok.h:60 [inline]
  sha1_base_finish include/crypto/sha1_base.h:102 [inline]
  sha1_finup+0x44e/0x4b0 arch/x86/crypto/sha1_ssse3_glue.c:70
  sha1_avx2_finup arch/x86/crypto/sha1_ssse3_glue.c:232 [inline]
  sha1_avx2_final+0x28/0x30 arch/x86/crypto/sha1_ssse3_glue.c:238
  crypto_shash_final+0x104/0x260 crypto/shash.c:152
  kdf_ctr security/keys/dh.c:186 [inline]
  keyctl_dh_compute_kdf security/keys/dh.c:217 [inline]
  __keyctl_dh_compute+0x1184/0x1bc0 security/keys/dh.c:389
  compat_keyctl_dh_compute+0x2c8/0x3e0 security/keys/compat_dh.c:39
  __do_compat_sys_keyctl security/keys/compat.c:136 [inline]
  __se_compat_sys_keyctl security/keys/compat.c:59 [inline]
  __ia32_compat_sys_keyctl+0x137/0x3b0 security/keys/compat.c:59
  do_syscall_32_irqs_on arch/x86/entry/common.c:323 [inline]
  do_fast_syscall_32+0x345/0xf9b arch/x86/entry/common.c:394
  entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
RIP: 0023:0xf7fbdcb9
Code: 55 08 8b 88 64 cd ff ff 8b 98 68 cd ff ff 89 c8 85 d2 74 02 89 0a 5b  
5d c3 8b 04 24 c3 8b 1c 24 c3 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90  
90 90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90
RSP: 002b:00000000ffdb494c EFLAGS: 00000286 ORIG_RAX: 0000000000000120
RAX: ffffffffffffffda RBX: 0000000000000017 RCX: 0000000020000100
RDX: 0000000020a53ffb RSI: 0000000000000005 RDI: 0000000020c61fc8
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000292 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000

Allocated by task 4761:
  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
  set_track mm/kasan/kasan.c:460 [inline]
  kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
  __do_kmalloc mm/slab.c:3718 [inline]
  __kmalloc+0x14e/0x760 mm/slab.c:3727
  kmalloc include/linux/slab.h:518 [inline]
  keyctl_dh_compute_kdf security/keys/dh.c:211 [inline]
  __keyctl_dh_compute+0xfe9/0x1bc0 security/keys/dh.c:389
  compat_keyctl_dh_compute+0x2c8/0x3e0 security/keys/compat_dh.c:39
  __do_compat_sys_keyctl security/keys/compat.c:136 [inline]
  __se_compat_sys_keyctl security/keys/compat.c:59 [inline]
  __ia32_compat_sys_keyctl+0x137/0x3b0 security/keys/compat.c:59
  do_syscall_32_irqs_on arch/x86/entry/common.c:323 [inline]
  do_fast_syscall_32+0x345/0xf9b arch/x86/entry/common.c:394
  entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139

Freed by task 1:
  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
  set_track mm/kasan/kasan.c:460 [inline]
  __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
  kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
  __cache_free mm/slab.c:3498 [inline]
  kfree+0xd9/0x260 mm/slab.c:3813
  acpi_os_free include/acpi/platform/aclinuxex.h:62 [inline]
  acpi_ns_get_node_unlocked+0x2b1/0x2ee drivers/acpi/acpica/nsutils.c:698
  acpi_ns_get_node+0x4d/0x6b drivers/acpi/acpica/nsutils.c:738
  acpi_get_handle+0x153/0x24b drivers/acpi/acpica/nsxfname.c:98
  acpi_has_method+0x68/0xa0 drivers/acpi/utils.c:555
  acpi_device_setup_files+0x393/0x820 drivers/acpi/device_sysfs.c:565
  acpi_device_add+0x8af/0x1240 drivers/acpi/scan.c:700
  acpi_add_single_object+0xaa5/0x1e70 drivers/acpi/scan.c:1620
  acpi_bus_check_add+0x5fa/0xb40 drivers/acpi/scan.c:1860
  acpi_ns_walk_namespace+0x224/0x400 drivers/acpi/acpica/nswalk.c:237
  acpi_walk_namespace+0xf2/0x12c drivers/acpi/acpica/nsxfeval.c:606
  acpi_bus_scan+0x138/0x160 drivers/acpi/scan.c:2041
  acpi_scan_init+0x404/0x8df drivers/acpi/scan.c:2198
  acpi_init+0x936/0x9fa drivers/acpi/bus.c:1284
  do_one_initcall+0x127/0x913 init/main.c:884
  do_initcall_level init/main.c:952 [inline]
  do_initcalls init/main.c:960 [inline]
  do_basic_setup init/main.c:978 [inline]
  kernel_init_freeable+0x49b/0x58e init/main.c:1135
  kernel_init+0x11/0x1b3 init/main.c:1061
  ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412

The buggy address belongs to the object at ffff8801d7130ec0
  which belongs to the cache kmalloc-32 of size 32
The buggy address is located 24 bytes inside of
  32-byte region [ffff8801d7130ec0, ffff8801d7130ee0)
The buggy address belongs to the page:
page:ffffea00075c4c00 count:1 mapcount:0 mapping:ffff8801d7130000  
index:0xffff8801d7130fc1
flags: 0x2fffc0000000100(slab)
raw: 02fffc0000000100 ffff8801d7130000 ffff8801d7130fc1 0000000100000036
raw: ffffea00076061e0 ffffea0007633a60 ffff8801da8001c0 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
  ffff8801d7130d80: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
  ffff8801d7130e00: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
> ffff8801d7130e80: fb fb fb fb fc fc fc fc 00 00 00 fc fc fc fc fc
                                                     ^
  ffff8801d7130f00: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
  ffff8801d7130f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


---
This bug 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 bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with  
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KASAN: slab-out-of-bounds Write in sha1_finup
  2018-06-07 13:07 KASAN: slab-out-of-bounds Write in sha1_finup syzbot
@ 2018-06-07 14:43 ` syzbot
  2018-06-07 19:12 ` [PATCH] dh key: fix rounding up KDF output length Eric Biggers
  2018-06-08 15:37 ` David Howells
  2 siblings, 0 replies; 9+ messages in thread
From: syzbot @ 2018-06-07 14:43 UTC (permalink / raw)
  To: davem, herbert, hpa, linux-crypto, linux-kernel, mingo,
	syzkaller-bugs, tglx, x86

syzbot has found a reproducer for the following crash on:

HEAD commit:    1c8c5a9d38f6 Merge git://git.kernel.org/pub/scm/linux/kern..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=109fbb4f800000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4f1acdf888c9d4e9
dashboard link: https://syzkaller.appspot.com/bug?extid=486f97f892efeb2075a3
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=11095fef800000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=175de79f800000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+486f97f892efeb2075a3@syzkaller.appspotmail.com

random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
random: sshd: uninitialized urandom read (32 bytes read)
==================================================================
BUG: KASAN: slab-out-of-bounds in put_unaligned_be32  
include/linux/unaligned/access_ok.h:60 [inline]
BUG: KASAN: slab-out-of-bounds in sha1_base_finish  
include/crypto/sha1_base.h:102 [inline]
BUG: KASAN: slab-out-of-bounds in sha1_finup+0x44e/0x4b0  
arch/x86/crypto/sha1_ssse3_glue.c:70
Write of size 4 at addr ffff8801d97e1ad8 by task syz-executor437/4553

CPU: 0 PID: 4553 Comm: syz-executor437 Not tainted 4.17.0+ #89
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+0x1b9/0x294 lib/dump_stack.c:113
  print_address_description+0x6c/0x20b mm/kasan/report.c:256
  kasan_report_error mm/kasan/report.c:354 [inline]
  kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
  __asan_report_store4_noabort+0x17/0x20 mm/kasan/report.c:437
  put_unaligned_be32 include/linux/unaligned/access_ok.h:60 [inline]
  sha1_base_finish include/crypto/sha1_base.h:102 [inline]
  sha1_finup+0x44e/0x4b0 arch/x86/crypto/sha1_ssse3_glue.c:70
  sha1_avx2_finup arch/x86/crypto/sha1_ssse3_glue.c:232 [inline]
  sha1_avx2_final+0x28/0x30 arch/x86/crypto/sha1_ssse3_glue.c:238
  crypto_shash_final+0x104/0x260 crypto/shash.c:152
  kdf_ctr security/keys/dh.c:186 [inline]
  keyctl_dh_compute_kdf security/keys/dh.c:217 [inline]
  __keyctl_dh_compute+0x1184/0x1bc0 security/keys/dh.c:389
  keyctl_dh_compute+0xb9/0x100 security/keys/dh.c:425
  __do_sys_keyctl security/keys/keyctl.c:1741 [inline]
  __se_sys_keyctl security/keys/keyctl.c:1637 [inline]
  __x64_sys_keyctl+0x12a/0x3b0 security/keys/keyctl.c:1637
  do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43ffb9
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 6b 45 00 00 c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffce5fe4808 EFLAGS: 00000217 ORIG_RAX: 00000000000000fa
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 000000000043ffb9
RDX: 0000000020a53ffb RSI: 0000000020000100 RDI: 0000000000000017
RBP: 00000000006ca018 R08: 0000000020c61fc8 R09: 00000000004002c8
R10: 0000000000000005 R11: 0000000000000217 R12: 00000000004018e0
R13: 0000000000401970 R14: 0000000000000000 R15: 0000000000000000

Allocated by task 4553:
  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
  set_track mm/kasan/kasan.c:460 [inline]
  kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
  __do_kmalloc mm/slab.c:3718 [inline]
  __kmalloc+0x14e/0x760 mm/slab.c:3727
  kmalloc include/linux/slab.h:518 [inline]
  keyctl_dh_compute_kdf security/keys/dh.c:211 [inline]
  __keyctl_dh_compute+0xfe9/0x1bc0 security/keys/dh.c:389
  keyctl_dh_compute+0xb9/0x100 security/keys/dh.c:425
  __do_sys_keyctl security/keys/keyctl.c:1741 [inline]
  __se_sys_keyctl security/keys/keyctl.c:1637 [inline]
  __x64_sys_keyctl+0x12a/0x3b0 security/keys/keyctl.c:1637
  do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 2877:
  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
  set_track mm/kasan/kasan.c:460 [inline]
  __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
  kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
  __cache_free mm/slab.c:3498 [inline]
  kfree+0xd9/0x260 mm/slab.c:3813
  single_release+0x8f/0xb0 fs/seq_file.c:609
  __fput+0x353/0x890 fs/file_table.c:209
  ____fput+0x15/0x20 fs/file_table.c:243
  task_work_run+0x1e4/0x290 kernel/task_work.c:113
  tracehook_notify_resume include/linux/tracehook.h:192 [inline]
  exit_to_usermode_loop+0x2bd/0x310 arch/x86/entry/common.c:166
  prepare_exit_to_usermode arch/x86/entry/common.c:196 [inline]
  syscall_return_slowpath arch/x86/entry/common.c:265 [inline]
  do_syscall_64+0x6ac/0x800 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

The buggy address belongs to the object at ffff8801d97e1ac0
  which belongs to the cache kmalloc-32 of size 32
The buggy address is located 24 bytes inside of
  32-byte region [ffff8801d97e1ac0, ffff8801d97e1ae0)
The buggy address belongs to the page:
page:ffffea000765f840 count:1 mapcount:0 mapping:ffff8801d97e1000  
index:0xffff8801d97e1fc1
flags: 0x2fffc0000000100(slab)
raw: 02fffc0000000100 ffff8801d97e1000 ffff8801d97e1fc1 0000000100000016
raw: ffffea00076540e0 ffffea000736cda0 ffff8801da8001c0 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
  ffff8801d97e1980: 00 00 00 00 fc fc fc fc 00 fc fc fc fc fc fc fc
  ffff8801d97e1a00: 00 00 00 00 fc fc fc fc fb fb fb fb fc fc fc fc
> ffff8801d97e1a80: fb fb fb fb fc fc fc fc 00 00 00 fc fc fc fc fc
                                                     ^
  ffff8801d97e1b00: fb fb fb fb fc fc fc fc 00 fc fc fc fc fc fc fc
  ffff8801d97e1b80: 00 fc fc fc fc fc fc fc 00 fc fc fc fc fc fc fc
==================================================================

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] dh key: fix rounding up KDF output length
  2018-06-07 13:07 KASAN: slab-out-of-bounds Write in sha1_finup syzbot
  2018-06-07 14:43 ` syzbot
@ 2018-06-07 19:12 ` Eric Biggers
  2018-06-07 19:16   ` Kees Cook
  2018-06-07 20:28   ` Tycho Andersen
  2018-06-08 15:37 ` David Howells
  2 siblings, 2 replies; 9+ messages in thread
From: Eric Biggers @ 2018-06-07 19:12 UTC (permalink / raw)
  To: David Howells, James Morris, keyrings, linux-security-module
  Cc: linux-crypto, linux-kernel, syzkaller-bugs, Tycho Andersen,
	Kees Cook, Stephan Mueller, Eric Biggers

From: Eric Biggers <ebiggers@google.com>

Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
kdf_ctr() to assume that the length of key material to derive is a
multiple of the digest size.  The length was supposed to be rounded up
accordingly.  However, the round_up() macro was used which only gives
the correct result on power-of-2 arguments, whereas not all hash
algorithms have power-of-2 digest sizes.  In some cases this resulted in
a write past the end of the 'outbuf' buffer.

Fix it by switching to roundup(), which works for non-power-of-2 inputs.

Reported-by: syzbot+486f97f892efeb2075a3@syzkaller.appspotmail.com
Reported-by: syzbot+29d17b7898b41ee120a5@syzkaller.appspotmail.com
Reported-by: syzbot+8a608baf8751184ec727@syzkaller.appspotmail.com
Reported-by: syzbot+d04e58bd384f1fe0b112@syzkaller.appspotmail.com
Fixes: 383203eff718 ("dh key: get rid of stack allocated array")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/dh.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/dh.c b/security/keys/dh.c
index f7403821db7f0..b203f7758f976 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -142,6 +142,8 @@ static void kdf_dealloc(struct kdf_sdesc *sdesc)
  * The src pointer is defined as Z || other info where Z is the shared secret
  * from DH and other info is an arbitrary string (see SP800-56A section
  * 5.8.1.2).
+ *
+ * 'dlen' must be a multiple of the digest size.
  */
 static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
 		   u8 *dst, unsigned int dlen, unsigned int zlen)
@@ -205,8 +207,8 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
 {
 	uint8_t *outbuf = NULL;
 	int ret;
-	size_t outbuf_len = round_up(buflen,
-			             crypto_shash_digestsize(sdesc->shash.tfm));
+	size_t outbuf_len = roundup(buflen,
+				    crypto_shash_digestsize(sdesc->shash.tfm));
 
 	outbuf = kmalloc(outbuf_len, GFP_KERNEL);
 	if (!outbuf) {
-- 
2.17.1.1185.g55be947832-goog

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dh key: fix rounding up KDF output length
  2018-06-07 19:12 ` [PATCH] dh key: fix rounding up KDF output length Eric Biggers
@ 2018-06-07 19:16   ` Kees Cook
  2018-06-07 19:28     ` James Morris
  2018-06-07 19:28     ` Eric Biggers
  2018-06-07 20:28   ` Tycho Andersen
  1 sibling, 2 replies; 9+ messages in thread
From: Kees Cook @ 2018-06-07 19:16 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, James Morris, keyrings, linux-security-module,
	linux-crypto, LKML, syzkaller-bugs, Tycho Andersen,
	Stephan Mueller, Eric Biggers

On Thu, Jun 7, 2018 at 12:12 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> From: Eric Biggers <ebiggers@google.com>
>
> Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
> kdf_ctr() to assume that the length of key material to derive is a
> multiple of the digest size.  The length was supposed to be rounded up
> accordingly.  However, the round_up() macro was used which only gives
> the correct result on power-of-2 arguments, whereas not all hash
> algorithms have power-of-2 digest sizes.  In some cases this resulted in
> a write past the end of the 'outbuf' buffer.
>
> Fix it by switching to roundup(), which works for non-power-of-2 inputs.

round_up() vs roundup(). Wow, that's not confusing. :( I wonder if we
should rename the former to roundup_pow2() or something?

> Reported-by: syzbot+486f97f892efeb2075a3@syzkaller.appspotmail.com
> Reported-by: syzbot+29d17b7898b41ee120a5@syzkaller.appspotmail.com
> Reported-by: syzbot+8a608baf8751184ec727@syzkaller.appspotmail.com
> Reported-by: syzbot+d04e58bd384f1fe0b112@syzkaller.appspotmail.com
> Fixes: 383203eff718 ("dh key: get rid of stack allocated array")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Regardless:

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  security/keys/dh.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/dh.c b/security/keys/dh.c
> index f7403821db7f0..b203f7758f976 100644
> --- a/security/keys/dh.c
> +++ b/security/keys/dh.c
> @@ -142,6 +142,8 @@ static void kdf_dealloc(struct kdf_sdesc *sdesc)
>   * The src pointer is defined as Z || other info where Z is the shared secret
>   * from DH and other info is an arbitrary string (see SP800-56A section
>   * 5.8.1.2).
> + *
> + * 'dlen' must be a multiple of the digest size.
>   */
>  static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen,
>                    u8 *dst, unsigned int dlen, unsigned int zlen)
> @@ -205,8 +207,8 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc,
>  {
>         uint8_t *outbuf = NULL;
>         int ret;
> -       size_t outbuf_len = round_up(buflen,
> -                                    crypto_shash_digestsize(sdesc->shash.tfm));
> +       size_t outbuf_len = roundup(buflen,
> +                                   crypto_shash_digestsize(sdesc->shash.tfm));
>
>         outbuf = kmalloc(outbuf_len, GFP_KERNEL);
>         if (!outbuf) {
> --
> 2.17.1.1185.g55be947832-goog
>



-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dh key: fix rounding up KDF output length
  2018-06-07 19:16   ` Kees Cook
@ 2018-06-07 19:28     ` James Morris
  2018-06-07 19:28     ` Eric Biggers
  1 sibling, 0 replies; 9+ messages in thread
From: James Morris @ 2018-06-07 19:28 UTC (permalink / raw)
  To: Kees Cook
  Cc: Eric Biggers, David Howells, keyrings, linux-security-module,
	linux-crypto, LKML, syzkaller-bugs, Tycho Andersen,
	Stephan Mueller, Eric Biggers

On Thu, 7 Jun 2018, Kees Cook wrote:

> On Thu, Jun 7, 2018 at 12:12 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
> > kdf_ctr() to assume that the length of key material to derive is a
> > multiple of the digest size.  The length was supposed to be rounded up
> > accordingly.  However, the round_up() macro was used which only gives
> > the correct result on power-of-2 arguments, whereas not all hash
> > algorithms have power-of-2 digest sizes.  In some cases this resulted in
> > a write past the end of the 'outbuf' buffer.
> >
> > Fix it by switching to roundup(), which works for non-power-of-2 inputs.
> 
> round_up() vs roundup(). Wow, that's not confusing. :( I wonder if we
> should rename the former to roundup_pow2() or something?

Good idea, in a separate patch(set).

-- 
James Morris
<jmorris@namei.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dh key: fix rounding up KDF output length
  2018-06-07 19:16   ` Kees Cook
  2018-06-07 19:28     ` James Morris
@ 2018-06-07 19:28     ` Eric Biggers
  1 sibling, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2018-06-07 19:28 UTC (permalink / raw)
  To: Kees Cook
  Cc: David Howells, James Morris, keyrings, linux-security-module,
	linux-crypto, LKML, syzkaller-bugs, Tycho Andersen,
	Stephan Mueller, Eric Biggers

On Thu, Jun 07, 2018 at 12:16:16PM -0700, Kees Cook wrote:
> On Thu, Jun 7, 2018 at 12:12 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
> > kdf_ctr() to assume that the length of key material to derive is a
> > multiple of the digest size.  The length was supposed to be rounded up
> > accordingly.  However, the round_up() macro was used which only gives
> > the correct result on power-of-2 arguments, whereas not all hash
> > algorithms have power-of-2 digest sizes.  In some cases this resulted in
> > a write past the end of the 'outbuf' buffer.
> >
> > Fix it by switching to roundup(), which works for non-power-of-2 inputs.
> 
> round_up() vs roundup(). Wow, that's not confusing. :( I wonder if we
> should rename the former to roundup_pow2() or something?

Yes, it's very confusing, and I wish the names were clearer, or that there was
one macro that just did the right thing (but then the power-of-2 optimization
could only be done for constants, where it might not be necessary anyway).
roundup_pow2() would still be confused with roundup_pow_of_two(), unfortunately.

Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dh key: fix rounding up KDF output length
  2018-06-07 19:12 ` [PATCH] dh key: fix rounding up KDF output length Eric Biggers
  2018-06-07 19:16   ` Kees Cook
@ 2018-06-07 20:28   ` Tycho Andersen
  1 sibling, 0 replies; 9+ messages in thread
From: Tycho Andersen @ 2018-06-07 20:28 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, James Morris, keyrings, linux-security-module,
	linux-crypto, linux-kernel, syzkaller-bugs, Kees Cook,
	Stephan Mueller, Eric Biggers

On Thu, Jun 07, 2018 at 12:12:01PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
> kdf_ctr() to assume that the length of key material to derive is a
> multiple of the digest size.  The length was supposed to be rounded up
> accordingly.  However, the round_up() macro was used which only gives
> the correct result on power-of-2 arguments, whereas not all hash
> algorithms have power-of-2 digest sizes.  In some cases this resulted in
> a write past the end of the 'outbuf' buffer.
> 
> Fix it by switching to roundup(), which works for non-power-of-2 inputs.
> 
> Reported-by: syzbot+486f97f892efeb2075a3@syzkaller.appspotmail.com
> Reported-by: syzbot+29d17b7898b41ee120a5@syzkaller.appspotmail.com
> Reported-by: syzbot+8a608baf8751184ec727@syzkaller.appspotmail.com
> Reported-by: syzbot+d04e58bd384f1fe0b112@syzkaller.appspotmail.com
> Fixes: 383203eff718 ("dh key: get rid of stack allocated array")
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Arg, thanks.

Acked-by: Tycho Andersen <tycho@tycho.ws>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dh key: fix rounding up KDF output length
  2018-06-07 13:07 KASAN: slab-out-of-bounds Write in sha1_finup syzbot
  2018-06-07 14:43 ` syzbot
  2018-06-07 19:12 ` [PATCH] dh key: fix rounding up KDF output length Eric Biggers
@ 2018-06-08 15:37 ` David Howells
  2018-06-25 17:14   ` Eric Biggers
  2 siblings, 1 reply; 9+ messages in thread
From: David Howells @ 2018-06-08 15:37 UTC (permalink / raw)
  To: Eric Biggers
  Cc: dhowells, James Morris, keyrings, linux-security-module,
	linux-crypto, linux-kernel, syzkaller-bugs, Tycho Andersen,
	Kees Cook, Stephan Mueller, Eric Biggers

Eric Biggers <ebiggers3@gmail.com> wrote:

> Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
> kdf_ctr() to assume that the length of key material to derive is a
> multiple of the digest size.  The length was supposed to be rounded up
> accordingly.  However, the round_up() macro was used which only gives
> the correct result on power-of-2 arguments, whereas not all hash
> algorithms have power-of-2 digest sizes.  In some cases this resulted in
> a write past the end of the 'outbuf' buffer.
> 
> Fix it by switching to roundup(), which works for non-power-of-2 inputs.

Applied.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] dh key: fix rounding up KDF output length
  2018-06-08 15:37 ` David Howells
@ 2018-06-25 17:14   ` Eric Biggers
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Biggers @ 2018-06-25 17:14 UTC (permalink / raw)
  To: David Howells
  Cc: James Morris, keyrings, linux-security-module, linux-crypto,
	linux-kernel, syzkaller-bugs, Tycho Andersen, Kees Cook,
	Stephan Mueller, Eric Biggers

Hi David,

On Fri, Jun 08, 2018 at 04:37:58PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > Commit 383203eff718 ("dh key: get rid of stack allocated array") changed
> > kdf_ctr() to assume that the length of key material to derive is a
> > multiple of the digest size.  The length was supposed to be rounded up
> > accordingly.  However, the round_up() macro was used which only gives
> > the correct result on power-of-2 arguments, whereas not all hash
> > algorithms have power-of-2 digest sizes.  In some cases this resulted in
> > a write past the end of the 'outbuf' buffer.
> > 
> > Fix it by switching to roundup(), which works for non-power-of-2 inputs.
> 
> Applied.

Applied to where?  When are you planning to send this to Linus?  Note that this
was a regression from v4.17 to v4.18-rc1.

Thanks,

- Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-06-25 17:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-07 13:07 KASAN: slab-out-of-bounds Write in sha1_finup syzbot
2018-06-07 14:43 ` syzbot
2018-06-07 19:12 ` [PATCH] dh key: fix rounding up KDF output length Eric Biggers
2018-06-07 19:16   ` Kees Cook
2018-06-07 19:28     ` James Morris
2018-06-07 19:28     ` Eric Biggers
2018-06-07 20:28   ` Tycho Andersen
2018-06-08 15:37 ` David Howells
2018-06-25 17:14   ` Eric Biggers

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).