2018-06-07 13:07:02

by syzbot

[permalink] [raw]
Subject: KASAN: slab-out-of-bounds Write in sha1_finup

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: [email protected]

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 [email protected].

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


2018-06-07 14:43:02

by syzbot

[permalink] [raw]
Subject: Re: KASAN: slab-out-of-bounds Write in sha1_finup

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: [email protected]

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
==================================================================

2018-06-07 19:12:01

by Eric Biggers

[permalink] [raw]
Subject: [PATCH] dh key: fix rounding up KDF output length

From: Eric Biggers <[email protected]>

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: [email protected]
Reported-by: [email protected]
Reported-by: [email protected]
Reported-by: [email protected]
Fixes: 383203eff718 ("dh key: get rid of stack allocated array")
Signed-off-by: Eric Biggers <[email protected]>
---
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

2018-06-07 19:16:16

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] dh key: fix rounding up KDF output length

On Thu, Jun 7, 2018 at 12:12 PM, Eric Biggers <[email protected]> wrote:
> From: Eric Biggers <[email protected]>
>
> 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: [email protected]
> Reported-by: [email protected]
> Reported-by: [email protected]
> Reported-by: [email protected]
> Fixes: 383203eff718 ("dh key: get rid of stack allocated array")
> Signed-off-by: Eric Biggers <[email protected]>

Regardless:

Acked-by: Kees Cook <[email protected]>

-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

2018-06-07 19:28:01

by James Morris

[permalink] [raw]
Subject: Re: [PATCH] dh key: fix rounding up KDF output length

On Thu, 7 Jun 2018, Kees Cook wrote:

> On Thu, Jun 7, 2018 at 12:12 PM, Eric Biggers <[email protected]> wrote:
> > From: Eric Biggers <[email protected]>
> >
> > 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
<[email protected]>

2018-06-07 19:28:48

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH] dh key: fix rounding up KDF output length

On Thu, Jun 07, 2018 at 12:16:16PM -0700, Kees Cook wrote:
> On Thu, Jun 7, 2018 at 12:12 PM, Eric Biggers <[email protected]> wrote:
> > From: Eric Biggers <[email protected]>
> >
> > 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

2018-06-07 20:28:31

by Tycho Andersen

[permalink] [raw]
Subject: Re: [PATCH] dh key: fix rounding up KDF output length

On Thu, Jun 07, 2018 at 12:12:01PM -0700, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> 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: [email protected]
> Reported-by: [email protected]
> Reported-by: [email protected]
> Reported-by: [email protected]
> Fixes: 383203eff718 ("dh key: get rid of stack allocated array")
> Signed-off-by: Eric Biggers <[email protected]>

Arg, thanks.

Acked-by: Tycho Andersen <[email protected]>

2018-06-08 15:37:58

by David Howells

[permalink] [raw]
Subject: Re: [PATCH] dh key: fix rounding up KDF output length

Eric Biggers <[email protected]> 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.

2018-06-25 17:14:26

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH] dh key: fix rounding up KDF output length

Hi David,

On Fri, Jun 08, 2018 at 04:37:58PM +0100, David Howells wrote:
> Eric Biggers <[email protected]> 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