Groups > linux.kernel > #1293866 > unrolled thread

GPF in lrw_crypt

Started by	Dmitry Vyukov <dvyukov@google.com>
First post	2015-12-17 14:00 +0100
Last post	2016-01-09 11:20 +0100
Articles	20 on this page of 27 — 5 participants

Back to article view | Back to linux.kernel

  GPF in lrw_crypt Dmitry Vyukov <dvyukov@google.com> - 2015-12-17 14:00 +0100
    Re: GPF in lrw_crypt Stephan Mueller <smueller@chronox.de> - 2015-12-22 00:00 +0100
    Re: GPF in lrw_crypt Herbert Xu <herbert@gondor.apana.org.au> - 2015-12-24 10:40 +0100
      Re: GPF in lrw_crypt Dmitry Vyukov <dvyukov@google.com> - 2015-12-24 12:10 +0100
        [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2) Herbert Xu <herbert@gondor.apana.org.au> - 2015-12-25 08:50 +0100
          Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2) Dmitry Vyukov <dvyukov@google.com> - 2015-12-28 14:50 +0100
            Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Herbert Xu <herbert@gondor.apana.org.au> - 2015-12-29 14:30 +0100
          Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-02 13:00 +0100
            Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-02 15:50 +0100
              Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2) Stephan Mueller <smueller@chronox.de> - 2016-01-02 21:10 +0100
                Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-02 21:20 +0100
                  Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-03 02:40 +0100
                    Re: [PATCH v2] crypto: algif_skcipher - Require setkey before  accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-03 10:50 +0100
                      [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-04 05:40 +0100
                        [PATCH 2/2] crypto: algif_skcipher - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-04 05:40 +0100
                        Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Milan Broz <gmazyland@gmail.com> - 2016-01-04 13:40 +0100
                          Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-08 13:50 +0100
                            Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Milan Broz <gmazyland@gmail.com> - 2016-01-08 19:30 +0100
                              Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-09 06:50 +0100
                                Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path Milan Broz <gmazyland@gmail.com> - 2016-01-09 11:20 +0100
                                  [PATCH 1/2] crypto: skcipher - Add crypto_skcipher_has_setkey Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-11 14:30 +0100
                                    [PATCH 2/2] crypto: algif_skcipher - Add key check exception for  cipher_null Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-11 14:40 +0100
                                      Re: [PATCH 2/2] crypto: algif_skcipher - Add key check exception for  cipher_null Milan Broz <gmazyland@gmail.com> - 2016-01-11 16:00 +0100
                          [PATCH 1/2] crypto: hash - Add crypto_ahash_has_setkey Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-08 14:30 +0100
                            [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2) Herbert Xu <herbert@gondor.apana.org.au> - 2016-01-08 14:40 +0100
                              Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2) kbuild test robot <lkp@intel.com> - 2016-01-08 15:00 +0100
                              Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2) Milan Broz <gmazyland@gmail.com> - 2016-01-09 11:20 +0100

Page 1 of 2 [1] 2 Next page →

#1293866 — GPF in lrw_crypt

From	Dmitry Vyukov <dvyukov@google.com>
Date	2015-12-17 14:00 +0100
Subject	GPF in lrw_crypt
Message-ID	<qGGk2-7YN-17@gated-at.bofh.it>

Hello,

The following program causes GPF in lrw_crypt:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <syscall.h>
#include <string.h>
#include <stdint.h>

int main()
{
        long r0 = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
        long r1 = syscall(SYS_mmap, 0x20000000ul, 0x10000ul, 0x2ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
        *(uint16_t*)0x20001000 = 0x26;
        memcpy((void*)0x20001002,
"\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
        *(uint32_t*)0x20001010 = 0xf;
        *(uint32_t*)0x20001014 = 0x100;
        memcpy((void*)0x20001018,
"\x6c\x72\x77\x28\x74\x77\x6f\x66\x69\x73\x68\x29\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
64);
        long r7 = syscall(SYS_bind, r0, 0x20001000ul, 0x58ul, 0, 0, 0);
        long r8 = syscall(SYS_accept4, r0, 0x0ul, 0x200023fdul, 0x800ul, 0, 0);
        memcpy((void*)0x20002fd8,
"\x09\xf1\x98\x36\x3f\x7d\x29\x96\x55\xe6\xa2\x42\x45\x67\x8f\x7c\x27\x44\x51\x6f\xbe\x4d\x52\x6f\x40\xaf\xe0\xd6\x04\x8d\x86\x36\x08\xc8\x55\xb8\xfe\x6e\x89\xef\x15\x2d\x07\x9a\x74\xab\xc7\x47\x26\xb5\x00\x93\x3b\x59\xe2\x1f\x6a\x29\x76\x7f\x9d\x3a\x86\x38\xda\x3e\xfb\xbe\x63\x2e\x38\x2f\x5c\x1c\x4d\xb8\x53\xf9\x97\xdb\x4a\xcc\xad\x55\xb3\xb5\xa0\xb4\xad\xfb\x39\xe5\x44\x12\x0b\x5f\x03\xbf\xc7\x28\x36\x1a\x7b\x4a\xff\x3e\x71\x17\x44\xf3\x09\xfb\x44\x41\x55\x1b\x6e\x6c\xcd\x03\x39\x66\xe2\x87\x65\xdd\x66\x7c\x00\x9f\x46\x54\x66\xf1\xa8\x4b\xd9\x10\xdc\x45\xd0\x57\x5c\x5e\x97\x42\x3a\xc9\x26\x5a\x55\x57\x65\x5f\x38\x54\xdd\x1e\xd8\x89\xe0\x34\xf0\x9e\x65\xf8\x89\x8e\xe4\x02\xdc\xa2\xa8\x45\x9c\xe9\xca\xef\xad\xdc\x74\xb5",
182);
        long r10 = syscall(SYS_write, r8, 0x20002fd8ul, 0xb6ul, 0, 0, 0);
        *(uint64_t*)0x20000fb8 = 0x20004fff;
        *(uint64_t*)0x20000fc0 = 0x94;
        *(uint64_t*)0x20000fc8 = 0x20000000;
        *(uint64_t*)0x20000fd0 = 0x5d;
        *(uint64_t*)0x20000fd8 = 0x20000f1b;
        *(uint64_t*)0x20000fe0 = 0xe5;
        *(uint64_t*)0x20000fe8 = 0x20004b08;
        *(uint64_t*)0x20000ff0 = 0xe9;
        *(uint64_t*)0x20000ff8 = 0x20002000;
        *(uint64_t*)0x20001000 = 0x1000;
        long r21 = syscall(SYS_readv, r8, 0x20000fb8ul, 0x5ul, 0, 0, 0);
        return 0;
}


================================================================================
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory
accessgeneral protection fault: 0000 [#1] SMP KASAN
Modules linked in:
CPU: 2 PID: 6912 Comm: a.out Not tainted 4.4.0-rc3+ #151
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: ffff88005ee61680 ti: ffff88005fef0000 task.ti: ffff88005fef0000
RIP: 0010:[<ffffffff82bf9609>]  [<ffffffff82bf9609>] gf128mul_64k_bbe+0x89/0x3f0
RSP: 0018:ffff88005fef7590  EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000001 RCX: 0000000000000000
RDX: ffffed000bfdee9f RSI: ffff88005ee61e40 RDI: ffffed000bfdeeab
RBP: ffff88005fef7650 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff88005fef7870
R13: 0000000000000000 R14: 0000000000000000 R15: 1ffff1000bfdeeb8
FS:  00000000026a3880(0063) GS:ffff88006ce00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000020000fb8 CR3: 000000005ffa0000 CR4: 00000000000006e0
Stack:
 ffff88005fef7730 ffff880000000fff 1ffff1000bfdeeb8 ffff88005fef7870
 ffff88005fef7710 0000000000000000 0000000041b58ab3 ffffffff87ab3a79
 ffffffff82bf9580 ffffffff82bafa9b ffff88005ee61680 0000000000000001
Call Trace:
 [<ffffffff82c04b9d>] lrw_crypt+0x29d/0xd20 crypto/lrw.c:248
 [<ffffffff812c6332>] lrw_decrypt+0xf2/0x150
arch/x86/crypto/serpent_avx2_glue.c:270
 [<ffffffff82babe81>] async_decrypt+0x1c1/0x2c0 crypto/blkcipher.c:443
 [<     inline     >] crypto_ablkcipher_decrypt include/linux/crypto.h:921
 [<     inline     >] skcipher_recvmsg_sync crypto/algif_skcipher.c:676
 [<ffffffff82d2e7b9>] skcipher_recvmsg+0x1029/0x1f10 crypto/algif_skcipher.c:706
 [<     inline     >] sock_recvmsg_nosec net/socket.c:712
 [<ffffffff856b1c8a>] sock_recvmsg+0xaa/0xe0 net/socket.c:720
 [<ffffffff856b1f33>] sock_read_iter+0x273/0x3d0 net/socket.c:797
 [<ffffffff8189655e>] do_iter_readv_writev+0x14e/0x2c0 fs/read_write.c:664
 [<ffffffff81898d62>] do_readv_writev+0x2e2/0x880 fs/read_write.c:808
 [<ffffffff81899395>] vfs_readv+0x95/0xd0 fs/read_write.c:834
 [<     inline     >] SYSC_readv fs/read_write.c:860
 [<ffffffff8189c181>] SyS_readv+0x111/0x2f0 fs/read_write.c:852
 [<ffffffff86a89fb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
Code: 04 00 00 f4 f4 c7 40 08 f3 f3 f3 f3 e8 d1 e9 a0 fe 4d 85 f6 0f
84 f6 01 00 00 4c 89 f1 48 b8 00 00 00 00 00 fc ff df 48 c1 e9 03 <80>
3c 01 00 0f 85 48 03 00 00 48 8b 5c 24 18 4d 8b 26 48 83 c3
RIP  [<ffffffff82bf9609>] gf128mul_64k_bbe+0x89/0x3f0 crypto/gf128mul.c:379
 RSP <ffff88005fef7590>
---[ end trace 018c54843b88ec1d ]---


On upstream commit 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8 (Nov 29).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1296276

From	Stephan Mueller <smueller@chronox.de>
Date	2015-12-22 00:00 +0100
Message-ID	<qIhAS-46k-19@gated-at.bofh.it>
In reply to	#1293866

Am Donnerstag, 17. Dezember 2015, 13:59:11 schrieb Dmitry Vyukov:

Hi Dmitry,

> Hello,
> 
> The following program causes GPF in lrw_crypt:

Here we are: this problem looks very much like the error reprt about a GFP in 
gf128mul_64_bbe.
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include <syscall.h>
> #include <string.h>
> #include <stdint.h>
> 
> int main()
> {
>         long r0 = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
>         long r1 = syscall(SYS_mmap, 0x20000000ul, 0x10000ul, 0x2ul,
> 0x32ul, 0xfffffffffffffffful, 0x0ul);
>         *(uint16_t*)0x20001000 = 0x26;
>         memcpy((void*)0x20001002,
> "\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
>         *(uint32_t*)0x20001010 = 0xf;
>         *(uint32_t*)0x20001014 = 0x100;
>         memcpy((void*)0x20001018,
> "\x6c\x72\x77\x28\x74\x77\x6f\x66\x69\x73\x68\x29\x00\x00\x00\x00\x00\x00\x0
> 0\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0
> 0\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0
> 0\x00\x00\x00\x00\x00\x00\x00", 64);
>         long r7 = syscall(SYS_bind, r0, 0x20001000ul, 0x58ul, 0, 0, 0);
>         long r8 = syscall(SYS_accept4, r0, 0x0ul, 0x200023fdul, 0x800ul, 0,
> 0); memcpy((void*)0x20002fd8,
> "\x09\xf1\x98\x36\x3f\x7d\x29\x96\x55\xe6\xa2\x42\x45\x67\x8f\x7c\x27\x44\x5
> 1\x6f\xbe\x4d\x52\x6f\x40\xaf\xe0\xd6\x04\x8d\x86\x36\x08\xc8\x55\xb8\xfe\x6
> e\x89\xef\x15\x2d\x07\x9a\x74\xab\xc7\x47\x26\xb5\x00\x93\x3b\x59\xe2\x1f\x6
> a\x29\x76\x7f\x9d\x3a\x86\x38\xda\x3e\xfb\xbe\x63\x2e\x38\x2f\x5c\x1c\x4d\xb
> 8\x53\xf9\x97\xdb\x4a\xcc\xad\x55\xb3\xb5\xa0\xb4\xad\xfb\x39\xe5\x44\x12\x0
> b\x5f\x03\xbf\xc7\x28\x36\x1a\x7b\x4a\xff\x3e\x71\x17\x44\xf3\x09\xfb\x44\x4
> 1\x55\x1b\x6e\x6c\xcd\x03\x39\x66\xe2\x87\x65\xdd\x66\x7c\x00\x9f\x46\x54\x6
> 6\xf1\xa8\x4b\xd9\x10\xdc\x45\xd0\x57\x5c\x5e\x97\x42\x3a\xc9\x26\x5a\x55\x5
> 7\x65\x5f\x38\x54\xdd\x1e\xd8\x89\xe0\x34\xf0\x9e\x65\xf8\x89\x8e\xe4\x02\xd
> c\xa2\xa8\x45\x9c\xe9\xca\xef\xad\xdc\x74\xb5", 182);
>         long r10 = syscall(SYS_write, r8, 0x20002fd8ul, 0xb6ul, 0, 0, 0);
>         *(uint64_t*)0x20000fb8 = 0x20004fff;
>         *(uint64_t*)0x20000fc0 = 0x94;
>         *(uint64_t*)0x20000fc8 = 0x20000000;
>         *(uint64_t*)0x20000fd0 = 0x5d;
>         *(uint64_t*)0x20000fd8 = 0x20000f1b;
>         *(uint64_t*)0x20000fe0 = 0xe5;
>         *(uint64_t*)0x20000fe8 = 0x20004b08;
>         *(uint64_t*)0x20000ff0 = 0xe9;
>         *(uint64_t*)0x20000ff8 = 0x20002000;
>         *(uint64_t*)0x20001000 = 0x1000;
>         long r21 = syscall(SYS_readv, r8, 0x20000fb8ul, 0x5ul, 0, 0, 0);
>         return 0;
> }
> 
> 
> ============================================================================
> ==== kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory
> accessgeneral protection fault: 0000 [#1] SMP KASAN
> Modules linked in:
> CPU: 2 PID: 6912 Comm: a.out Not tainted 4.4.0-rc3+ #151
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: ffff88005ee61680 ti: ffff88005fef0000 task.ti: ffff88005fef0000 RIP:
> 0010:[<ffffffff82bf9609>]  [<ffffffff82bf9609>] gf128mul_64k_bbe+0x89/0x3f0
> RSP: 0018:ffff88005fef7590  EFLAGS: 00010246
> RAX: dffffc0000000000 RBX: 0000000000000001 RCX: 0000000000000000
> RDX: ffffed000bfdee9f RSI: ffff88005ee61e40 RDI: ffffed000bfdeeab
> RBP: ffff88005fef7650 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff88005fef7870
> R13: 0000000000000000 R14: 0000000000000000 R15: 1ffff1000bfdeeb8
> FS:  00000000026a3880(0063) GS:ffff88006ce00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000020000fb8 CR3: 000000005ffa0000 CR4: 00000000000006e0
> Stack:
>  ffff88005fef7730 ffff880000000fff 1ffff1000bfdeeb8 ffff88005fef7870
>  ffff88005fef7710 0000000000000000 0000000041b58ab3 ffffffff87ab3a79
>  ffffffff82bf9580 ffffffff82bafa9b ffff88005ee61680 0000000000000001
> Call Trace:
>  [<ffffffff82c04b9d>] lrw_crypt+0x29d/0xd20 crypto/lrw.c:248
>  [<ffffffff812c6332>] lrw_decrypt+0xf2/0x150
> arch/x86/crypto/serpent_avx2_glue.c:270
>  [<ffffffff82babe81>] async_decrypt+0x1c1/0x2c0 crypto/blkcipher.c:443
>  [<     inline     >] crypto_ablkcipher_decrypt include/linux/crypto.h:921
>  [<     inline     >] skcipher_recvmsg_sync crypto/algif_skcipher.c:676
>  [<ffffffff82d2e7b9>] skcipher_recvmsg+0x1029/0x1f10
> crypto/algif_skcipher.c:706 [<     inline     >] sock_recvmsg_nosec
> net/socket.c:712
>  [<ffffffff856b1c8a>] sock_recvmsg+0xaa/0xe0 net/socket.c:720
>  [<ffffffff856b1f33>] sock_read_iter+0x273/0x3d0 net/socket.c:797
>  [<ffffffff8189655e>] do_iter_readv_writev+0x14e/0x2c0 fs/read_write.c:664
>  [<ffffffff81898d62>] do_readv_writev+0x2e2/0x880 fs/read_write.c:808
>  [<ffffffff81899395>] vfs_readv+0x95/0xd0 fs/read_write.c:834
>  [<     inline     >] SYSC_readv fs/read_write.c:860
>  [<ffffffff8189c181>] SyS_readv+0x111/0x2f0 fs/read_write.c:852
>  [<ffffffff86a89fb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> Code: 04 00 00 f4 f4 c7 40 08 f3 f3 f3 f3 e8 d1 e9 a0 fe 4d 85 f6 0f
> 84 f6 01 00 00 4c 89 f1 48 b8 00 00 00 00 00 fc ff df 48 c1 e9 03 <80>
> 3c 01 00 0f 85 48 03 00 00 48 8b 5c 24 18 4d 8b 26 48 83 c3
> RIP  [<ffffffff82bf9609>] gf128mul_64k_bbe+0x89/0x3f0 crypto/gf128mul.c:379
>  RSP <ffff88005fef7590>
> ---[ end trace 018c54843b88ec1d ]---
> 
> 
> On upstream commit 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8 (Nov 29).
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297796

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2015-12-24 10:40 +0100
Message-ID	<qJaxj-4ZU-1@gated-at.bofh.it>
In reply to	#1293866

On Thu, Dec 17, 2015 at 01:59:11PM +0100, Dmitry Vyukov wrote:
> 
> The following program causes GPF in lrw_crypt:

OK, this is a result of certain implementations (such as lrw)
not coping with there being no key gracefully.

I think the easiest way is probably to check this in algif_skcipher.

---8<---
Subject: crypto: algif_skcipher - Require setkey before accept(2)

Some cipher implementations will crash if you try to use them
without calling setkey first.  This patch adds a check so that
the accept(2) call will fail with -ENOKEY if setkey hasn't been
done on the socket yet.

Cc: stable@vger.kernel.org
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 5c756b3..b62c973 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -31,6 +31,11 @@ struct skcipher_sg_list {
 	struct scatterlist sg[0];
 };
 
+struct skcipher_tfm {
+	struct crypto_skcipher *skcipher;
+	bool has_key;
+};
+
 struct skcipher_ctx {
 	struct list_head tsgl;
 	struct af_alg_sgl rsgl;
@@ -750,17 +755,38 @@ static struct proto_ops algif_skcipher_ops = {
 
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
-	return crypto_alloc_skcipher(name, type, mask);
+	struct skcipher_tfm *tfm;
+
+	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+	if (!tfm)
+		return ERR_PTR(-ENOMEM);
+
+	tfm->skcipher = crypto_alloc_skcipher(name, type, mask);
+	if (IS_ERR(tfm->skcipher)) {
+		kfree(tfm);
+		return ERR_CAST(tfm->skcipher);
+	}
+
+	return tfm;
 }
 
 static void skcipher_release(void *private)
 {
-	crypto_free_skcipher(private);
+	struct skcipher_tfm *tfm = private;
+
+	crypto_free_skcipher(tfm->skcipher);
+	kfree(tfm);
 }
 
 static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	return crypto_skcipher_setkey(private, key, keylen);
+	struct skcipher_tfm *tfm = private;
+	int err;
+
+	err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
+	tfm->has_key = !err;
+
+	return err;
 }
 
 static void skcipher_wait(struct sock *sk)
@@ -792,20 +818,25 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(private);
+	struct skcipher_tfm *tfm = private;
+	struct crypto_skcipher *skcipher = tfm->skcipher;
+	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
+
+	if (!tfm->has_key)
+		return -ENOKEY;
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
+	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
 			       GFP_KERNEL);
 	if (!ctx->iv) {
 		sock_kfree_s(sk, ctx, len);
 		return -ENOMEM;
 	}
 
-	memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
+	memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
 
 	INIT_LIST_HEAD(&ctx->tsgl);
 	ctx->len = len;
@@ -818,7 +849,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 
 	ask->private = ctx;
 
-	skcipher_request_set_tfm(&ctx->req, private);
+	skcipher_request_set_tfm(&ctx->req, skcipher);
 	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				      af_alg_complete, &ctx->completion);
 
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1297817

From	Dmitry Vyukov <dvyukov@google.com>
Date	2015-12-24 12:10 +0100
Message-ID	<qJbWq-5Z3-15@gated-at.bofh.it>
In reply to	#1297796

On Thu, Dec 24, 2015 at 10:39 AM, Herbert Xu
<herbert@gondor.apana.org.au> wrote:
> On Thu, Dec 17, 2015 at 01:59:11PM +0100, Dmitry Vyukov wrote:
>>
>> The following program causes GPF in lrw_crypt:
>
> OK, this is a result of certain implementations (such as lrw)
> not coping with there being no key gracefully.
>
> I think the easiest way is probably to check this in algif_skcipher.
>
> ---8<---
> Subject: crypto: algif_skcipher - Require setkey before accept(2)
>
> Some cipher implementations will crash if you try to use them
> without calling setkey first.  This patch adds a check so that
> the accept(2) call will fail with -ENOKEY if setkey hasn't been
> done on the socket yet.
>
> Cc: stable@vger.kernel.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 5c756b3..b62c973 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -31,6 +31,11 @@ struct skcipher_sg_list {
>         struct scatterlist sg[0];
>  };
>
> +struct skcipher_tfm {
> +       struct crypto_skcipher *skcipher;
> +       bool has_key;
> +};
> +
>  struct skcipher_ctx {
>         struct list_head tsgl;
>         struct af_alg_sgl rsgl;
> @@ -750,17 +755,38 @@ static struct proto_ops algif_skcipher_ops = {
>
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> -       return crypto_alloc_skcipher(name, type, mask);
> +       struct skcipher_tfm *tfm;
> +
> +       tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> +       if (!tfm)
> +               return ERR_PTR(-ENOMEM);
> +
> +       tfm->skcipher = crypto_alloc_skcipher(name, type, mask);
> +       if (IS_ERR(tfm->skcipher)) {
> +               kfree(tfm);
> +               return ERR_CAST(tfm->skcipher);
> +       }
> +
> +       return tfm;
>  }
>
>  static void skcipher_release(void *private)
>  {
> -       crypto_free_skcipher(private);
> +       struct skcipher_tfm *tfm = private;
> +
> +       crypto_free_skcipher(tfm->skcipher);
> +       kfree(tfm);
>  }
>
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> -       return crypto_skcipher_setkey(private, key, keylen);
> +       struct skcipher_tfm *tfm = private;
> +       int err;
> +
> +       err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
> +       tfm->has_key = !err;
> +
> +       return err;
>  }
>
>  static void skcipher_wait(struct sock *sk)
> @@ -792,20 +818,25 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  {
>         struct skcipher_ctx *ctx;
>         struct alg_sock *ask = alg_sk(sk);
> -       unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(private);
> +       struct skcipher_tfm *tfm = private;
> +       struct crypto_skcipher *skcipher = tfm->skcipher;
> +       unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
> +
> +       if (!tfm->has_key)
> +               return -ENOKEY;
>
>         ctx = sock_kmalloc(sk, len, GFP_KERNEL);
>         if (!ctx)
>                 return -ENOMEM;
>
> -       ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
> +       ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
>                                GFP_KERNEL);
>         if (!ctx->iv) {
>                 sock_kfree_s(sk, ctx, len);
>                 return -ENOMEM;
>         }
>
> -       memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
> +       memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
>
>         INIT_LIST_HEAD(&ctx->tsgl);
>         ctx->len = len;
> @@ -818,7 +849,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>
>         ask->private = ctx;
>
> -       skcipher_request_set_tfm(&ctx->req, private);
> +       skcipher_request_set_tfm(&ctx->req, skcipher);
>         skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>                                       af_alg_complete, &ctx->completion);
>


Hi Herbert,

I am testing with your two patches:
crypto: algif_skcipher - Use new skcipher interface
crypto: algif_skcipher - Require setkey before accept(2)
on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).

Now the following program causes a bunch of use-after-frees and them
kills kernel:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <unistd.h>
#include <sys/syscall.h>
#include <string.h>
#include <stdint.h>
#include <pthread.h>

long r[10];

int main()
{
        memset(r, -1, sizeof(r));
        r[0] = syscall(SYS_mmap, 0x20000000ul, 0xca7000ul, 0x3ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
        memcpy((void*)0x20ca6bc1,
"\x2e\x2f\x63\x6f\x6e\x74\x72\x6f\x6c\x00", 10);
        r[2] = syscall(SYS_creat, 0x20ca6bc1ul, 0x28ul, 0, 0, 0, 0);
        r[3] = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
        *(uint16_t*)0x20000986 = (uint16_t)0x26;
        memcpy((void*)0x20000988,
"\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
        *(uint32_t*)0x20000996 = (uint32_t)0x1;
        *(uint32_t*)0x2000099a = (uint32_t)0xf;
        memcpy((void*)0x2000099e,
"\x5f\x5f\x65\x63\x62\x2d\x73\x65\x72\x70\x65\x6e\x74\x2d\x73\x73\x65\x32\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00",
64);
        r[9] = syscall(SYS_bind, r[3], 0x20000986ul, 0x58ul, 0, 0, 0);
        return 0;
}

==================================================================
BUG: KASAN: use-after-free in skcipher_bind+0xe1/0x100 at addr ffff88003398bab0
Read of size 8 by task syz-executor/15123
=============================================================================
BUG kmalloc-16 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

Disabling lock debugging due to kernel taint
INFO: Allocated in skcipher_bind+0x55/0x100 age=36 cpu=1 pid=15123
[<      none      >] ___slab_alloc+0x489/0x4e0 mm/slub.c:2468
[<      none      >] __slab_alloc+0x4c/0x90 mm/slub.c:2497
[<     inline     >] slab_alloc_node mm/slub.c:2560
[<     inline     >] slab_alloc mm/slub.c:2602
[<      none      >] kmem_cache_alloc_trace+0x264/0x2f0 mm/slub.c:2619
[<     inline     >] kmalloc include/linux/slab.h:458
[<     inline     >] kzalloc include/linux/slab.h:602
[<      none      >] skcipher_bind+0x55/0x100 crypto/algif_skcipher.c:760
[<      none      >] alg_bind+0x18e/0x3a0 crypto/af_alg.c:155
[<      none      >] SYSC_bind+0x1ea/0x250 net/socket.c:1376
[<      none      >] SyS_bind+0x24/0x30 net/socket.c:1362
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Freed in skcipher_bind+0xbd/0x100 age=7 cpu=2 pid=15123
[<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
[<     inline     >] slab_free mm/slub.c:2833
[<      none      >] kfree+0x26a/0x290 mm/slub.c:3662
[<      none      >] skcipher_bind+0xbd/0x100 crypto/algif_skcipher.c:766
[<      none      >] alg_bind+0x18e/0x3a0 crypto/af_alg.c:155
[<      none      >] SYSC_bind+0x1ea/0x250 net/socket.c:1376
[<      none      >] SyS_bind+0x24/0x30 net/socket.c:1362
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff8289a21d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [<ffffffff816c56f4>] print_trailer+0xf4/0x150 mm/slub.c:659
 [<ffffffff816cbeaf>] object_err+0x2f/0x40 mm/slub.c:689
 [<     inline     >] print_address_description mm/kasan/report.c:138
 [<ffffffff816ce86d>] kasan_report_error+0x25d/0x560 mm/kasan/report.c:251
 [<     inline     >] kasan_report mm/kasan/report.c:274
 [<ffffffff816cec6e>] __asan_report_load8_noabort+0x3e/0x40
mm/kasan/report.c:295
 [<ffffffff827ddb41>] skcipher_bind+0xe1/0x100 crypto/algif_skcipher.c:767
 [<ffffffff827da38e>] alg_bind+0x18e/0x3a0 crypto/af_alg.c:155
 [<ffffffff84b5a0ba>] SYSC_bind+0x1ea/0x250 net/socket.c:1376
 [<ffffffff84b5c7e4>] SyS_bind+0x24/0x30 net/socket.c:1362
 [<ffffffff85c8b376>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
==================================================================
==================================================================
BUG: KASAN: use-after-free in skcipher_release+0x43/0x50 at addr
ffff88003398b4f8
Read of size 8 by task syz-executor/15123
=============================================================================
BUG kmalloc-16 (Tainted: G    B          ): kasan: bad access detected
-----------------------------------------------------------------------------
INFO: Allocated in kstrdup_const+0x46/0x60 age=28033 cpu=0 pid=14775
[<      none      >] ___slab_alloc+0x489/0x4e0 mm/slub.c:2468
[<      none      >] __slab_alloc+0x4c/0x90 mm/slub.c:2497
[<     inline     >] slab_alloc mm/slub.c:2560
[<      none      >] __kmalloc_track_caller+0x278/0x310 mm/slub.c:4066
[<      none      >] kstrdup+0x39/0x70 mm/util.c:53
[<      none      >] kstrdup_const+0x46/0x60 mm/util.c:74
[<      none      >] alloc_vfsmnt+0xe7/0x760 fs/namespace.c:212
[<      none      >] clone_mnt+0x74/0xa20 fs/namespace.c:973
[<      none      >] copy_tree+0x3c0/0x940 fs/namespace.c:1713
[<      none      >] copy_mnt_ns+0x110/0x8b0 fs/namespace.c:2800
[<      none      >] create_new_namespaces+0xd0/0x610 kernel/nsproxy.c:70
[<      none      >] unshare_nsproxy_namespaces+0xa9/0x1d0 kernel/nsproxy.c:190
[<     inline     >] SYSC_unshare kernel/fork.c:2008
[<      none      >] SyS_unshare+0x3b0/0x800 kernel/fork.c:1958
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Freed in kfree_const+0x39/0x50 age=28020 cpu=1 pid=7161
[<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
[<     inline     >] slab_free mm/slub.c:2833
[<      none      >] kfree+0x26a/0x290 mm/slub.c:3662
[<      none      >] kfree_const+0x39/0x50 mm/util.c:35
[<      none      >] free_vfsmnt+0x37/0x80 fs/namespace.c:580
[<      none      >] delayed_free_vfsmnt+0x16/0x20 fs/namespace.c:589
[<     inline     >] __rcu_reclaim kernel/rcu/rcu.h:118
[<     inline     >] rcu_do_batch kernel/rcu/tree.c:2694
[<     inline     >] invoke_rcu_callbacks kernel/rcu/tree.c:2962
[<     inline     >] __rcu_process_callbacks kernel/rcu/tree.c:2929
[<      none      >] rcu_process_callbacks+0xc71/0x1380 kernel/rcu/tree.c:2946
[<      none      >] __do_softirq+0x23b/0x8a0 kernel/softirq.c:273
[<     inline     >] invoke_softirq kernel/softirq.c:350
[<      none      >] irq_exit+0x15d/0x190 kernel/softirq.c:391
[<     inline     >] exiting_irq ./arch/x86/include/asm/apic.h:653
[<      none      >] smp_apic_timer_interrupt+0x83/0xa0
arch/x86/kernel/apic/apic.c:926
[<      none      >] apic_timer_interrupt+0x8c/0xa0
arch/x86/entry/entry_64.S:520
[<      none      >] percpu_down_read_trylock+0x22/0xc0
kernel/locking/percpu-rwsem.c:87
[<      none      >] __sb_start_write+0x116/0x130 fs/super.c:1200
[<     inline     >] sb_start_write_trylock include/linux/fs.h:1454
[<      none      >] touch_atime+0x130/0x280 fs/inode.c:1643
[<     inline     >] file_accessed include/linux/fs.h:1916
[<      none      >] pipe_read+0x70d/0xb20 fs/pipe.c:328
[<     inline     >] new_sync_read fs/read_write.c:422
[<      none      >] __vfs_read+0x2fd/0x460 fs/read_write.c:434
[<      none      >] vfs_read+0x106/0x310 fs/read_write.c:454

Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff8289a21d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [<ffffffff816c56f4>] print_trailer+0xf4/0x150 mm/slub.c:659
 [<ffffffff816cbeaf>] object_err+0x2f/0x40 mm/slub.c:689
 [<     inline     >] print_address_description mm/kasan/report.c:138
 [<ffffffff816ce86d>] kasan_report_error+0x25d/0x560 mm/kasan/report.c:251
 [<     inline     >] kasan_report mm/kasan/report.c:274
 [<ffffffff816cec6e>] __asan_report_load8_noabort+0x3e/0x40
mm/kasan/report.c:295
 [<ffffffff827dda53>] skcipher_release+0x43/0x50 crypto/algif_skcipher.c:777
 [<     inline     >] alg_do_release crypto/af_alg.c:116
 [<ffffffff827d9d8c>] alg_sock_destruct+0x8c/0xe0 crypto/af_alg.c:315
 [<ffffffff84b6bb0a>] sk_destruct+0x4a/0x490 net/core/sock.c:1447
 [<ffffffff84b6bfa7>] __sk_free+0x57/0x200 net/core/sock.c:1475
 [<ffffffff84b6c180>] sk_free+0x30/0x40 net/core/sock.c:1486
 [<     inline     >] sock_put include/net/sock.h:1627
 [<ffffffff827d95db>] af_alg_release+0x5b/0x70 crypto/af_alg.c:123
 [<ffffffff84b55aed>] sock_release+0x8d/0x1d0 net/socket.c:571
 [<ffffffff84b55c46>] sock_close+0x16/0x20 net/socket.c:1022
 [<ffffffff81716103>] __fput+0x233/0x780 fs/file_table.c:208
 [<ffffffff817166d5>] ____fput+0x15/0x20 fs/file_table.c:244
 [<ffffffff8134679b>] task_work_run+0x16b/0x200 kernel/task_work.c:115
 [<     inline     >] exit_task_work include/linux/task_work.h:21
 [<ffffffff812f4d3b>] do_exit+0x8bb/0x2b20 kernel/exit.c:750
 [<ffffffff812f7118>] do_group_exit+0x108/0x320 kernel/exit.c:880
 [<     inline     >] SYSC_exit_group kernel/exit.c:891
 [<ffffffff812f734d>] SyS_exit_group+0x1d/0x20 kernel/exit.c:889
 [<ffffffff85c8b376>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
==================================================================

... a bunch of reports skipped ...

BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
Fixing recursive fault but reboot is needed!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298097 — [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2015-12-25 08:50 +0100
Subject	[PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qJviq-EO-5@gated-at.bofh.it>
In reply to	#1297817

Dmitry Vyukov <dvyukov@google.com> wrote:
>
> I am testing with your two patches:
> crypto: algif_skcipher - Use new skcipher interface
> crypto: algif_skcipher - Require setkey before accept(2)
> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).

You sent the email to everyone on the original CC list except me.
Please don't do that.

> Now the following program causes a bunch of use-after-frees and them
> kills kernel:

Yes there is an obvious bug in the patch that Julia Lawall has
responded to in another thread.  Here is a fixed version.

---8<--
Some cipher implementations will crash if you try to use them
without calling setkey first.  This patch adds a check so that
the accept(2) call will fail with -ENOKEY if setkey hasn't been
done on the socket yet.

Cc: stable@vger.kernel.org
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 5c756b3..f4431bc 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -31,6 +31,11 @@ struct skcipher_sg_list {
 	struct scatterlist sg[0];
 };
 
+struct skcipher_tfm {
+	struct crypto_skcipher *skcipher;
+	bool has_key;
+};
+
 struct skcipher_ctx {
 	struct list_head tsgl;
 	struct af_alg_sgl rsgl;
@@ -750,17 +755,41 @@ static struct proto_ops algif_skcipher_ops = {
 
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
-	return crypto_alloc_skcipher(name, type, mask);
+	struct skcipher_tfm *tfm;
+	struct crypto_skcipher *skcipher;
+
+	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+	if (!tfm)
+		return ERR_PTR(-ENOMEM);
+
+	skcipher = crypto_alloc_skcipher(name, type, mask);
+	if (IS_ERR(skcipher)) {
+		kfree(tfm);
+		return ERR_CAST(skcipher);
+	}
+
+	tfm->skcipher = skcipher;
+
+	return tfm;
 }
 
 static void skcipher_release(void *private)
 {
-	crypto_free_skcipher(private);
+	struct skcipher_tfm *tfm = private;
+
+	crypto_free_skcipher(tfm->skcipher);
+	kfree(tfm);
 }
 
 static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	return crypto_skcipher_setkey(private, key, keylen);
+	struct skcipher_tfm *tfm = private;
+	int err;
+
+	err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
+	tfm->has_key = !err;
+
+	return err;
 }
 
 static void skcipher_wait(struct sock *sk)
@@ -792,20 +821,25 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(private);
+	struct skcipher_tfm *tfm = private;
+	struct crypto_skcipher *skcipher = tfm->skcipher;
+	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
+
+	if (!tfm->has_key)
+		return -ENOKEY;
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
+	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
 			       GFP_KERNEL);
 	if (!ctx->iv) {
 		sock_kfree_s(sk, ctx, len);
 		return -ENOMEM;
 	}
 
-	memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
+	memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
 
 	INIT_LIST_HEAD(&ctx->tsgl);
 	ctx->len = len;
@@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 
 	ask->private = ctx;
 
-	skcipher_request_set_tfm(&ctx->req, private);
+	skcipher_request_set_tfm(&ctx->req, skcipher);
 	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				      af_alg_complete, &ctx->completion);
 
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298643 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Dmitry Vyukov <dvyukov@google.com>
Date	2015-12-28 14:50 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qKGls-2JC-17@gated-at.bofh.it>
In reply to	#1298097

On Fri, Dec 25, 2015 at 8:40 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> I am testing with your two patches:
>> crypto: algif_skcipher - Use new skcipher interface
>> crypto: algif_skcipher - Require setkey before accept(2)
>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
>
> You sent the email to everyone on the original CC list except me.
> Please don't do that.

Hi Herbert,

My email client just followed instructions in your email. You've said
to Reply-to: syzkaller@googlegroups.com.

This version of the patch does fix the crash.

Tested-by: Dmitry Vyukov <dvyukov@google.com>

However, crypto is still considerably unstable. I will post reports
that I see separately.


>> Now the following program causes a bunch of use-after-frees and them
>> kills kernel:
>
> Yes there is an obvious bug in the patch that Julia Lawall has
> responded to in another thread.  Here is a fixed version.
>
> ---8<--
> Some cipher implementations will crash if you try to use them
> without calling setkey first.  This patch adds a check so that
> the accept(2) call will fail with -ENOKEY if setkey hasn't been
> done on the socket yet.
>
> Cc: stable@vger.kernel.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 5c756b3..f4431bc 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -31,6 +31,11 @@ struct skcipher_sg_list {
>         struct scatterlist sg[0];
>  };
>
> +struct skcipher_tfm {
> +       struct crypto_skcipher *skcipher;
> +       bool has_key;
> +};
> +
>  struct skcipher_ctx {
>         struct list_head tsgl;
>         struct af_alg_sgl rsgl;
> @@ -750,17 +755,41 @@ static struct proto_ops algif_skcipher_ops = {
>
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> -       return crypto_alloc_skcipher(name, type, mask);
> +       struct skcipher_tfm *tfm;
> +       struct crypto_skcipher *skcipher;
> +
> +       tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> +       if (!tfm)
> +               return ERR_PTR(-ENOMEM);
> +
> +       skcipher = crypto_alloc_skcipher(name, type, mask);
> +       if (IS_ERR(skcipher)) {
> +               kfree(tfm);
> +               return ERR_CAST(skcipher);
> +       }
> +
> +       tfm->skcipher = skcipher;
> +
> +       return tfm;
>  }
>
>  static void skcipher_release(void *private)
>  {
> -       crypto_free_skcipher(private);
> +       struct skcipher_tfm *tfm = private;
> +
> +       crypto_free_skcipher(tfm->skcipher);
> +       kfree(tfm);
>  }
>
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> -       return crypto_skcipher_setkey(private, key, keylen);
> +       struct skcipher_tfm *tfm = private;
> +       int err;
> +
> +       err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
> +       tfm->has_key = !err;
> +
> +       return err;
>  }
>
>  static void skcipher_wait(struct sock *sk)
> @@ -792,20 +821,25 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  {
>         struct skcipher_ctx *ctx;
>         struct alg_sock *ask = alg_sk(sk);
> -       unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(private);
> +       struct skcipher_tfm *tfm = private;
> +       struct crypto_skcipher *skcipher = tfm->skcipher;
> +       unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
> +
> +       if (!tfm->has_key)
> +               return -ENOKEY;
>
>         ctx = sock_kmalloc(sk, len, GFP_KERNEL);
>         if (!ctx)
>                 return -ENOMEM;
>
> -       ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
> +       ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
>                                GFP_KERNEL);
>         if (!ctx->iv) {
>                 sock_kfree_s(sk, ctx, len);
>                 return -ENOMEM;
>         }
>
> -       memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
> +       memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
>
>         INIT_LIST_HEAD(&ctx->tsgl);
>         ctx->len = len;
> @@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>
>         ask->private = ctx;
>
> -       skcipher_request_set_tfm(&ctx->req, private);
> +       skcipher_request_set_tfm(&ctx->req, skcipher);
>         skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>                                       af_alg_complete, &ctx->completion);
>
> --
> 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
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To post to this group, send email to syzkaller@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/20151225074005.GA14690%40gondor.apana.org.au.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298987 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2015-12-29 14:30 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qL2vF-13G-11@gated-at.bofh.it>
In reply to	#1298643

Dmitry Vyukov <dvyukov@google.com> wrote:
>
> My email client just followed instructions in your email. You've said
> to Reply-to: syzkaller@googlegroups.com.

I did not set this Reply-To header.  It's most likely set by your
broken Google Groups setup.  So either you should start actually
replying to my email address or your future emails will most
likely not be read by me.

Cheers,
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300067 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-02 13:00 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qMt0L-7Eq-19@gated-at.bofh.it>
In reply to	#1298097

On 12/25/2015 08:40 AM, Herbert Xu wrote:
> Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> I am testing with your two patches:
>> crypto: algif_skcipher - Use new skcipher interface
>> crypto: algif_skcipher - Require setkey before accept(2)
>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
> 
> You sent the email to everyone on the original CC list except me.
> Please don't do that.
> 
>> Now the following program causes a bunch of use-after-frees and them
>> kills kernel:
> 
> Yes there is an obvious bug in the patch that Julia Lawall has
> responded to in another thread.  Here is a fixed version.
> 
> ---8<--
> Some cipher implementations will crash if you try to use them
> without calling setkey first.  This patch adds a check so that
> the accept(2) call will fail with -ENOKEY if setkey hasn't been
> done on the socket yet.


Hi Herbert,

this patch breaks userspace in cryptsetup...

We use algif_skcipher in cryptsetup (for years, even before
there was Stephan's library) and with this patch applied
I see fail in ALG_SET_IV call (patch from your git).

I can fix it upstream, but for thousands of installations it will
be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
it will be unusable. Also people who configured kernel crypto API as default
backend will have non-working cryptsetup).

Is it really thing for stable branch?

Milan

> 
> Cc: stable@vger.kernel.org
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 5c756b3..f4431bc 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -31,6 +31,11 @@ struct skcipher_sg_list {
>  	struct scatterlist sg[0];
>  };
>  
> +struct skcipher_tfm {
> +	struct crypto_skcipher *skcipher;
> +	bool has_key;
> +};
> +
>  struct skcipher_ctx {
>  	struct list_head tsgl;
>  	struct af_alg_sgl rsgl;
> @@ -750,17 +755,41 @@ static struct proto_ops algif_skcipher_ops = {
>  
>  static void *skcipher_bind(const char *name, u32 type, u32 mask)
>  {
> -	return crypto_alloc_skcipher(name, type, mask);
> +	struct skcipher_tfm *tfm;
> +	struct crypto_skcipher *skcipher;
> +
> +	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
> +	if (!tfm)
> +		return ERR_PTR(-ENOMEM);
> +
> +	skcipher = crypto_alloc_skcipher(name, type, mask);
> +	if (IS_ERR(skcipher)) {
> +		kfree(tfm);
> +		return ERR_CAST(skcipher);
> +	}
> +
> +	tfm->skcipher = skcipher;
> +
> +	return tfm;
>  }
>  
>  static void skcipher_release(void *private)
>  {
> -	crypto_free_skcipher(private);
> +	struct skcipher_tfm *tfm = private;
> +
> +	crypto_free_skcipher(tfm->skcipher);
> +	kfree(tfm);
>  }
>  
>  static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
>  {
> -	return crypto_skcipher_setkey(private, key, keylen);
> +	struct skcipher_tfm *tfm = private;
> +	int err;
> +
> +	err = crypto_skcipher_setkey(tfm->skcipher, key, keylen);
> +	tfm->has_key = !err;
> +
> +	return err;
>  }
>  
>  static void skcipher_wait(struct sock *sk)
> @@ -792,20 +821,25 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  {
>  	struct skcipher_ctx *ctx;
>  	struct alg_sock *ask = alg_sk(sk);
> -	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(private);
> +	struct skcipher_tfm *tfm = private;
> +	struct crypto_skcipher *skcipher = tfm->skcipher;
> +	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
> +
> +	if (!tfm->has_key)
> +		return -ENOKEY;
>  
>  	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
> +	ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(skcipher),
>  			       GFP_KERNEL);
>  	if (!ctx->iv) {
>  		sock_kfree_s(sk, ctx, len);
>  		return -ENOMEM;
>  	}
>  
> -	memset(ctx->iv, 0, crypto_skcipher_ivsize(private));
> +	memset(ctx->iv, 0, crypto_skcipher_ivsize(skcipher));
>  
>  	INIT_LIST_HEAD(&ctx->tsgl);
>  	ctx->len = len;
> @@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  
>  	ask->private = ctx;
>  
> -	skcipher_request_set_tfm(&ctx->req, private);
> +	skcipher_request_set_tfm(&ctx->req, skcipher);
>  	skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>  				      af_alg_complete, &ctx->completion);
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300079 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-02 15:50 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qMvFg-RW-7@gated-at.bofh.it>
In reply to	#1300067

On 01/02/2016 12:52 PM, Milan Broz wrote:
> On 12/25/2015 08:40 AM, Herbert Xu wrote:
>> Dmitry Vyukov <dvyukov@google.com> wrote:
>>>
>>> I am testing with your two patches:
>>> crypto: algif_skcipher - Use new skcipher interface
>>> crypto: algif_skcipher - Require setkey before accept(2)
>>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
>>
>> You sent the email to everyone on the original CC list except me.
>> Please don't do that.
>>
>>> Now the following program causes a bunch of use-after-frees and them
>>> kills kernel:
>>
>> Yes there is an obvious bug in the patch that Julia Lawall has
>> responded to in another thread.  Here is a fixed version.
>>
>> ---8<--
>> Some cipher implementations will crash if you try to use them
>> without calling setkey first.  This patch adds a check so that
>> the accept(2) call will fail with -ENOKEY if setkey hasn't been
>> done on the socket yet.
> 
> 
> Hi Herbert,
> 
> this patch breaks userspace in cryptsetup...
> 
> We use algif_skcipher in cryptsetup (for years, even before
> there was Stephan's library) and with this patch applied
> I see fail in ALG_SET_IV call (patch from your git).

(Obviously this was because of failing accept() call here, not set_iv.)

> 
> I can fix it upstream, but for thousands of installations it will
> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
> it will be unusable. Also people who configured kernel crypto API as default
> backend will have non-working cryptsetup).
> 
> Is it really thing for stable branch?

Also how it is supposed to work for cipher_null, where there is no key?
Why it should call set_key if it is noop? (and set key length 0 is not possible).

(We are using cipher_null for testing and for offline re-encryption tool
to create temporary "fake" header for not-yet encrypted device...)

Milan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300113 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Stephan Mueller <smueller@chronox.de>
Date	2016-01-02 21:10 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qMAEW-4bL-17@gated-at.bofh.it>
In reply to	#1300079

Am Samstag, 2. Januar 2016, 15:41:34 schrieb Milan Broz:

Hi Milan,

> On 01/02/2016 12:52 PM, Milan Broz wrote:
> > On 12/25/2015 08:40 AM, Herbert Xu wrote:
> >> Dmitry Vyukov <dvyukov@google.com> wrote:
> >>> I am testing with your two patches:
> >>> crypto: algif_skcipher - Use new skcipher interface
> >>> crypto: algif_skcipher - Require setkey before accept(2)
> >>> on top of a88164345b81292b55a8d4829fdd35c8d611cd7d (Dec 23).
> >> 
> >> You sent the email to everyone on the original CC list except me.
> >> Please don't do that.
> >> 
> >>> Now the following program causes a bunch of use-after-frees and them
> >> 
> >>> kills kernel:
> >> Yes there is an obvious bug in the patch that Julia Lawall has
> >> responded to in another thread.  Here is a fixed version.
> >> 
> >> ---8<--
> >> Some cipher implementations will crash if you try to use them
> >> without calling setkey first.  This patch adds a check so that
> >> the accept(2) call will fail with -ENOKEY if setkey hasn't been
> >> done on the socket yet.
> > 
> > Hi Herbert,
> > 
> > this patch breaks userspace in cryptsetup...
> > 
> > We use algif_skcipher in cryptsetup (for years, even before
> > there was Stephan's library) and with this patch applied
> > I see fail in ALG_SET_IV call (patch from your git).
> 
> (Obviously this was because of failing accept() call here, not set_iv.)
> 
> > I can fix it upstream, but for thousands of installations it will
> > be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
> > it will be unusable. Also people who configured kernel crypto API as
> > default backend will have non-working cryptsetup).
> > 
> > Is it really thing for stable branch?
> 
> Also how it is supposed to work for cipher_null, where there is no key?
> Why it should call set_key if it is noop? (and set key length 0 is not
> possible).
> 
> (We are using cipher_null for testing and for offline re-encryption tool
> to create temporary "fake" header for not-yet encrypted device...)

The change implies that any setkey or set IV operations (i.e. any operations 
on the tfmfd) are done before the opfd(s) are created with one or more accept 
calls.

Thus, after a bind that returns the tfmfd, the setkey and setiv operations 
shall be called. This is followed by accept. If you change the order of 
invocations in your code, it should work.
> 
> Milan


-- 
Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300116 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-02 21:20 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qMAOC-4f6-15@gated-at.bofh.it>
In reply to	#1300113

On 01/02/2016 09:03 PM, Stephan Mueller wrote:
> Am Samstag, 2. Januar 2016, 15:41:34 schrieb Milan Broz:
> 
> Hi Milan,
> 

...
>>> Hi Herbert,
>>>
>>> this patch breaks userspace in cryptsetup...
>>>
>>> We use algif_skcipher in cryptsetup (for years, even before
>>> there was Stephan's library) and with this patch applied
>>> I see fail in ALG_SET_IV call (patch from your git).
>>
>> (Obviously this was because of failing accept() call here, not set_iv.)
>>
>>> I can fix it upstream, but for thousands of installations it will
>>> be broken (for LUKS there is a fallback, cor TrueCrypt compatible devices
>>> it will be unusable. Also people who configured kernel crypto API as
>>> default backend will have non-working cryptsetup).
>>>
>>> Is it really thing for stable branch?
>>
>> Also how it is supposed to work for cipher_null, where there is no key?
>> Why it should call set_key if it is noop? (and set key length 0 is not
>> possible).
>>
>> (We are using cipher_null for testing and for offline re-encryption tool
>> to create temporary "fake" header for not-yet encrypted device...)
> 
> The change implies that any setkey or set IV operations (i.e. any operations 
> on the tfmfd) are done before the opfd(s) are created with one or more accept 
> calls.
> 
> Thus, after a bind that returns the tfmfd, the setkey and setiv operations 
> shall be called. This is followed by accept. If you change the order of 
> invocations in your code, it should work.

Hi,

I already changed it in cryptsetup upstream this way.

But I cannot change thousands of cryptsetup installations that are actively using that code.
This is clear userspace breakage which should not happen this way.

(Moreover it still doesn't work for cipher_null that has min/max key size 0.)

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300166 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2016-01-03 02:40 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qMFOi-7gt-1@gated-at.bofh.it>
In reply to	#1300116

On Sat, Jan 02, 2016 at 09:18:30PM +0100, Milan Broz wrote:
>
> But I cannot change thousands of cryptsetup installations that are actively using that code.
> This is clear userspace breakage which should not happen this way.

I'll try to add some compatibility code for your case, assuming
your modus operandi is accept(2) followed by a single setkey before
proceeding to encryption/decryption.

> (Moreover it still doesn't work for cipher_null that has min/max key size 0.)

Setkey works just fine on cipher_null.

Cheers,
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300282 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-03 10:50 +0100
Subject	Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)
Message-ID	<qMNsu-3DD-3@gated-at.bofh.it>
In reply to	#1300166

On 01/03/2016 02:31 AM, Herbert Xu wrote:
> On Sat, Jan 02, 2016 at 09:18:30PM +0100, Milan Broz wrote:
>>
>> But I cannot change thousands of cryptsetup installations that are actively using that code.
>> This is clear userspace breakage which should not happen this way.
> 
> I'll try to add some compatibility code for your case, assuming
> your modus operandi is accept(2) followed by a single setkey before
> proceeding to encryption/decryption.

Hi,

yes, basically it prepares socket()/bind()/accept() and then it calls setkey once.
(I'll try to fix in next releases to call setkey first though.)

I am doing exactly the same for AF_ALG HMAC (hmac(<hash>) key,
does this requirement for order if accept/setkey applies there as well?
(It is not enforced yet.)

Anyway, you can easily simulate that skcipher API call just with running "cryptsetup benchmark"
(with accept() patch it will print N/A for all ciphers while without patch it measures some
more-or-less magic performance numbers :)

> 
>> (Moreover it still doesn't work for cipher_null that has min/max key size 0.)
> 
> Setkey works just fine on cipher_null.

Yes, it works if ALG_SET_KEY is set to zero-length key.
I just re-introduced old bug to code, sorry.

Thanks!
Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300479 — [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2016-01-04 05:40 +0100
Subject	[PATCH 1/2] crypto: af_alg - Add nokey compatibility path
Message-ID	<qN562-6Ig-15@gated-at.bofh.it>
In reply to	#1300282

On Sun, Jan 03, 2016 at 10:42:28AM +0100, Milan Broz wrote:
> 
> yes, basically it prepares socket()/bind()/accept() and then it calls setkey once.
> (I'll try to fix in next releases to call setkey first though.)

OK please try these two patches (warning, totally untested).

---8<---
This patch adds a compatibility path to support old applications
that do acept(2) before setkey.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index eaf98e2..6566d2e 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -76,6 +76,8 @@ int af_alg_register_type(const struct af_alg_type *type)
 		goto unlock;
 
 	type->ops->owner = THIS_MODULE;
+	if (type->ops_nokey)
+		type->ops_nokey->owner = THIS_MODULE;
 	node->type = type;
 	list_add(&node->list, &alg_types);
 	err = 0;
@@ -267,6 +269,7 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
 	const struct af_alg_type *type;
 	struct sock *sk2;
 	int err;
+	bool nokey;
 
 	lock_sock(sk);
 	type = ask->type;
@@ -285,12 +288,17 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
 	security_sk_clone(sk, sk2);
 
 	err = type->accept(ask->private, sk2);
+
+	nokey = err == -ENOKEY;
+	if (nokey && type->accept_nokey)
+		err = type->accept_nokey(ask->private, sk2);
+
 	if (err)
 		goto unlock;
 
 	sk2->sk_family = PF_ALG;
 
-	if (!ask->refcnt++)
+	if (nokey || !ask->refcnt++)
 		sock_hold(sk);
 	alg_sk(sk2)->parent = sk;
 	alg_sk(sk2)->type = type;
@@ -298,6 +306,9 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
 	newsock->ops = type->ops;
 	newsock->state = SS_CONNECTED;
 
+	if (nokey)
+		newsock->ops = type->ops_nokey;
+
 	err = 0;
 
 unlock:
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 589716f..df82844 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -52,9 +52,11 @@ struct af_alg_type {
 	void (*release)(void *private);
 	int (*setkey)(void *private, const u8 *key, unsigned int keylen);
 	int (*accept)(void *private, struct sock *sk);
+	int (*accept_nokey)(void *private, struct sock *sk);
 	int (*setauthsize)(void *private, unsigned int authsize);
 
 	struct proto_ops *ops;
+	struct proto_ops *ops_nokey;
 	struct module *owner;
 	char name[14];
 };
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300482 — [PATCH 2/2] crypto: algif_skcipher - Add nokey compatibility path

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2016-01-04 05:40 +0100
Subject	[PATCH 2/2] crypto: algif_skcipher - Add nokey compatibility path
Message-ID	<qN562-6Ig-23@gated-at.bofh.it>
In reply to	#1300479

This patch adds a compatibility path to support old applications
that do acept(2) before setkey.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index f4431bc..110bab4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -753,6 +753,99 @@ static struct proto_ops algif_skcipher_ops = {
 	.poll		=	skcipher_poll,
 };
 
+static int skcipher_check_key(struct socket *sock)
+{
+	int err;
+	struct sock *psk;
+	struct alg_sock *pask;
+	struct skcipher_tfm *tfm;
+	struct sock *sk = sock->sk;
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (ask->refcnt)
+		return 0;
+
+	psk = ask->parent;
+	pask = alg_sk(ask->parent);
+	tfm = pask->private;
+
+	err = -ENOKEY;
+	lock_sock(psk);
+	if (!tfm->has_key)
+		goto unlock;
+
+	if (!pask->refcnt++)
+		sock_hold(psk);
+
+	ask->refcnt = 1;
+	sock_put(psk);
+
+	err = 0;
+
+unlock:
+	release_sock(psk);
+
+	return err;
+}
+
+static int skcipher_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t size)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendmsg(sock, msg, size);
+}
+
+static ssize_t skcipher_sendpage_nokey(struct socket *sock, struct page *page,
+				       int offset, size_t size, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_sendpage(sock, page, offset, size, flags);
+}
+
+static int skcipher_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+				  size_t ignored, int flags)
+{
+	int err;
+
+	err = skcipher_check_key(sock);
+	if (err)
+		return err;
+
+	return skcipher_recvmsg(sock, msg, ignored, flags);
+}
+
+static struct proto_ops algif_skcipher_ops_nokey = {
+	.family		=	PF_ALG,
+
+	.connect	=	sock_no_connect,
+	.socketpair	=	sock_no_socketpair,
+	.getname	=	sock_no_getname,
+	.ioctl		=	sock_no_ioctl,
+	.listen		=	sock_no_listen,
+	.shutdown	=	sock_no_shutdown,
+	.getsockopt	=	sock_no_getsockopt,
+	.mmap		=	sock_no_mmap,
+	.bind		=	sock_no_bind,
+	.accept		=	sock_no_accept,
+	.setsockopt	=	sock_no_setsockopt,
+
+	.release	=	af_alg_release,
+	.sendmsg	=	skcipher_sendmsg_nokey,
+	.sendpage	=	skcipher_sendpage_nokey,
+	.recvmsg	=	skcipher_recvmsg_nokey,
+	.poll		=	skcipher_poll,
+};
+
 static void *skcipher_bind(const char *name, u32 type, u32 mask)
 {
 	struct skcipher_tfm *tfm;
@@ -802,7 +895,7 @@ static void skcipher_wait(struct sock *sk)
 		msleep(100);
 }
 
-static void skcipher_sock_destruct(struct sock *sk)
+static void skcipher_sock_destruct_common(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct skcipher_ctx *ctx = ask->private;
@@ -814,10 +907,33 @@ static void skcipher_sock_destruct(struct sock *sk)
 	skcipher_free_sgl(sk);
 	sock_kzfree_s(sk, ctx->iv, crypto_skcipher_ivsize(tfm));
 	sock_kfree_s(sk, ctx, ctx->len);
+}
+
+static void skcipher_sock_destruct(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
 	af_alg_release_parent(sk);
 }
 
-static int skcipher_accept_parent(void *private, struct sock *sk)
+static void skcipher_release_parent_nokey(struct sock *sk)
+{
+	struct alg_sock *ask = alg_sk(sk);
+
+	if (!ask->refcnt) {
+		sock_put(ask->parent);
+		return;
+	}
+
+	af_alg_release_parent(sk);
+}
+
+static void skcipher_sock_destruct_nokey(struct sock *sk)
+{
+	skcipher_sock_destruct_common(sk);
+	skcipher_release_parent_nokey(sk);
+}
+
+static int skcipher_accept_parent_common(void *private, struct sock *sk)
 {
 	struct skcipher_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
@@ -825,9 +941,6 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	struct crypto_skcipher *skcipher = tfm->skcipher;
 	unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(skcipher);
 
-	if (!tfm->has_key)
-		return -ENOKEY;
-
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
@@ -861,12 +974,38 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 	return 0;
 }
 
+static int skcipher_accept_parent(void *private, struct sock *sk)
+{
+	struct skcipher_tfm *tfm = private;
+
+	if (!tfm->has_key)
+		return -ENOKEY;
+
+	return skcipher_accept_parent_common(private, sk);
+}
+
+static int skcipher_accept_parent_nokey(void *private, struct sock *sk)
+{
+	int err;
+
+	err = skcipher_accept_parent_common(private, sk);
+	if (err)
+		goto out;
+
+	sk->sk_destruct = skcipher_sock_destruct_nokey;
+
+out:
+	return err;
+}
+
 static const struct af_alg_type algif_type_skcipher = {
 	.bind		=	skcipher_bind,
 	.release	=	skcipher_release,
 	.setkey		=	skcipher_setkey,
 	.accept		=	skcipher_accept_parent,
+	.accept_nokey	=	skcipher_accept_parent_nokey,
 	.ops		=	&algif_skcipher_ops,
+	.ops_nokey	=	&algif_skcipher_ops_nokey,
 	.name		=	"skcipher",
 	.owner		=	THIS_MODULE
 };
-- 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300720 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-04 13:40 +0100
Subject	Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path
Message-ID	<qNcAy-3to-17@gated-at.bofh.it>
In reply to	#1300479

On 01/04/2016 05:35 AM, Herbert Xu wrote:
> On Sun, Jan 03, 2016 at 10:42:28AM +0100, Milan Broz wrote:
>>
>> yes, basically it prepares socket()/bind()/accept() and then it calls setkey once.
>> (I'll try to fix in next releases to call setkey first though.)
> 
> OK please try these two patches (warning, totally untested).

Well, it is not much better.

I had to apply another two patches that are not mentioned and are not in your tree yet
before it:
  crypto: af_alg - Disallow bind_setkey_... after accept(2)
  crypto: use-after-free in alg_bind

then it at least compiles correctly.

skcipher works, But I still see two compatibility problems:

- hmac() is now failing the same way (SETKEY after accept())
(I initially tested without two patches above, these are not in linux-next yet.)
This breaks all cryptsetup TrueCrypt support (and moreover all systems if
kernel crypto API is set as a default vcrypto backend - but that's not default).

- cipher_null before worked without setkey, now it requires to set key
(either before or after accept().
This was actually probably bad workaround in cryptsetup, anyway it will now cause
old cryptsetup-reencrypt tool failing with your patches.
(Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
but not requiring setkey for "cipher" that has no key internally seems ok for me...)

As I said, I'll fix it in cryptsetup upstream but we are breaking  a lot of existing systems.
Isn't there better way, how to fix it?

Milan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1304504 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2016-01-08 13:50 +0100
Subject	Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path
Message-ID	<qOEEq-6N7-25@gated-at.bofh.it>
In reply to	#1300720

On Mon, Jan 04, 2016 at 01:33:54PM +0100, Milan Broz wrote:
>
> - hmac() is now failing the same way (SETKEY after accept())
> (I initially tested without two patches above, these are not in linux-next yet.)
> This breaks all cryptsetup TrueCrypt support (and moreover all systems if
> kernel crypto API is set as a default vcrypto backend - but that's not default).

Yes algif_hash would need the same compatibility patch and I'm
working on that.

> - cipher_null before worked without setkey, now it requires to set key
> (either before or after accept().
> This was actually probably bad workaround in cryptsetup, anyway it will now cause
> old cryptsetup-reencrypt tool failing with your patches.
> (Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
> but not requiring setkey for "cipher" that has no key internally seems ok for me...)

Is cipher_null actually used in production or is this just a
benchmark? Using the kernel crypto API to perform no encryption
sounds crazy.

> As I said, I'll fix it in cryptsetup upstream but we are breaking  a lot of existing systems.
> Isn't there better way, how to fix it?

Setting the key after accept has always been wrong.  It's just
that it hasn't been noticed until now.  The main crypto socket
corresponds to the tfm object and is shared by all the child
sockets produced by accept(2).  So once you have child sockets
which may then be used by another thread you must not modify
the parent socket/tfm in any way, and in particular you must
not change the key.

Cheers,
-- 
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

[toc] | [prev] | [next] | [standalone]

#1304860 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-08 19:30 +0100
Subject	Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path
Message-ID	<qOJXt-29h-29@gated-at.bofh.it>
In reply to	#1304504

On 01/08/2016 01:48 PM, Herbert Xu wrote:
> On Mon, Jan 04, 2016 at 01:33:54PM +0100, Milan Broz wrote:
>>
>> - hmac() is now failing the same way (SETKEY after accept())
>> (I initially tested without two patches above, these are not in linux-next yet.)
>> This breaks all cryptsetup TrueCrypt support (and moreover all systems if
>> kernel crypto API is set as a default vcrypto backend - but that's not default).
> 
> Yes algif_hash would need the same compatibility patch and I'm
> working on that.

Ok, I fixed this already.

> 
>> - cipher_null before worked without setkey, now it requires to set key
>> (either before or after accept().
>> This was actually probably bad workaround in cryptsetup, anyway it will now cause
>> old cryptsetup-reencrypt tool failing with your patches.
>> (Try "cryptsetup benchmark -c cipher_null-ecb". I am not sure what to do here,
>> but not requiring setkey for "cipher" that has no key internally seems ok for me...)
> 
> Is cipher_null actually used in production or is this just a
> benchmark? Using the kernel crypto API to perform no encryption
> sounds crazy.

Except benchmarks (I do not care about this) we use cipher_null in cryptsetup-reencrypt
when adding encryption in-place (plaintext-only device is converted
to LUKS, cipher_null is used during conversion for original plainext device,
then offline converted to some real cipher with key).
(It reuses the same logic as re-encryption, IOW volume key change.)

So it is used in production but just for this specific case.

Thanks,
Milan

[toc] | [prev] | [next] | [standalone]

#1305177 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

From	Herbert Xu <herbert@gondor.apana.org.au>
Date	2016-01-09 06:50 +0100
Subject	Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path
Message-ID	<qOUzv-14L-3@gated-at.bofh.it>
In reply to	#1304860

On Fri, Jan 08, 2016 at 07:21:12PM +0100, Milan Broz wrote:
>
> > Yes algif_hash would need the same compatibility patch and I'm
> > working on that.
> 
> Ok, I fixed this already.

Can you please test the two patches that I sent for algif_hash?

> So it is used in production but just for this specific case.

OK I will add something similar to the algif_hash has_setkey
logic for cipher_null.

Cheers,
-- 
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

[toc] | [prev] | [next] | [standalone]

#1305203 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

From	Milan Broz <gmazyland@gmail.com>
Date	2016-01-09 11:20 +0100
Subject	Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path
Message-ID	<qOYMN-44U-1@gated-at.bofh.it>
In reply to	#1305177

On 01/09/2016 06:41 AM, Herbert Xu wrote:
> On Fri, Jan 08, 2016 at 07:21:12PM +0100, Milan Broz wrote:
>>
>>> Yes algif_hash would need the same compatibility patch and I'm
>>> working on that.
>>
>> Ok, I fixed this already.
> 
> Can you please test the two patches that I sent for algif_hash?

I have stack of 7 patches on top of Linus' 4.4.0-rc8 tree, just to be sure
that I am testing the right versions, I created a git branch with
imported series here:
  http://git.kernel.org/cgit/linux/kernel/git/mbroz/linux.git/log/?h=crypto-api-compat

With these patches it works (except that cipher_null setkey issue).
I'll reply with tested-by to the patch thread.

> 
>> So it is used in production but just for this specific case.
> 
> OK I will add something similar to the algif_hash has_setkey
> logic for cipher_null.

Thanks!

Milan

[toc] | [prev] | [next] | [standalone]

Page 1 of 2 [1] 2 Next page →

csiph-web

GPF in lrw_crypt

Contents

#1293866 — GPF in lrw_crypt

#1296276

#1297796

#1297817

#1298097 — [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1298643 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1298987 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300067 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300079 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300113 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300116 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300166 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300282 — Re: [PATCH v2] crypto: algif_skcipher - Require setkey before accept(2)

#1300479 — [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

#1300482 — [PATCH 2/2] crypto: algif_skcipher - Add nokey compatibility path

#1300720 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

#1304504 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

#1304860 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

#1305177 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path

#1305203 — Re: [PATCH 1/2] crypto: af_alg - Add nokey compatibility path