GPF in lrw_crypt

GPF in lrw_crypt

[Dmitry Vyukov]

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

Re: GPF in lrw_crypt

[Stephan Mueller]

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

Re: GPF in lrw_crypt

[Herbert Xu]

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

Re: GPF in lrw_crypt

[Dmitry Vyukov]

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!

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

[Herbert Xu]

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

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

[Dmitry Vyukov]

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.

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

[Herbert Xu]

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

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

[Milan Broz]

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

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

[Milan Broz]

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

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

[Stephan Mueller]

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

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

[Milan Broz]

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

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

[Herbert Xu]

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

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

[Milan Broz]

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

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

[Herbert Xu]

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

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

[Herbert Xu]

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

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

[Milan Broz]

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

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

[Herbert Xu]

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

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

[Milan Broz]

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

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

[Herbert Xu]

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

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

[Milan Broz]

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

[PATCH 1/2] crypto: skcipher - Add crypto_skcipher_has_setkey

[Herbert Xu]

This patch adds a way for skcipher users to determine whether a key
is required by a transform.

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

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 7591928..d199c0b 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -118,6 +118,7 @@ static int crypto_init_skcipher_ops_blkcipher(struct crypto_tfm *tfm)
 	skcipher->decrypt = skcipher_decrypt_blkcipher;
 
 	skcipher->ivsize = crypto_blkcipher_ivsize(blkcipher);
+	skcipher->has_setkey = calg->cra_blkcipher.max_keysize;
 
 	return 0;
 }
@@ -210,6 +211,7 @@ static int crypto_init_skcipher_ops_ablkcipher(struct crypto_tfm *tfm)
 	skcipher->ivsize = crypto_ablkcipher_ivsize(ablkcipher);
 	skcipher->reqsize = crypto_ablkcipher_reqsize(ablkcipher) +
 			    sizeof(struct ablkcipher_request);
+	skcipher->has_setkey = calg->cra_ablkcipher.max_keysize;
 
 	return 0;
 }
diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index d8dd41f..fd8742a 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -61,6 +61,8 @@ struct crypto_skcipher {
 	unsigned int ivsize;
 	unsigned int reqsize;
 
+	bool has_setkey;
+
 	struct crypto_tfm base;
 };
 
@@ -305,6 +307,11 @@ static inline int crypto_skcipher_setkey(struct crypto_skcipher *tfm,
 	return tfm->setkey(tfm, key, keylen);
 }
 
+static inline bool crypto_skcipher_has_setkey(struct crypto_skcipher *tfm)
+{
+	return tfm->has_setkey;
+}
+
 /**
  * crypto_skcipher_reqtfm() - obtain cipher handle from request
  * @req: skcipher_request out of which the cipher handle is to be obtained
-- 
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

[PATCH 2/2] crypto: algif_skcipher - Add key check exception for cipher_null

[Herbert Xu]

This patch adds an exception to the key check so that cipher_null
users may continue to use algif_skcipher without setting a key.

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

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 110bab4..4a5bdb6 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -978,7 +978,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
 {
 	struct skcipher_tfm *tfm = private;
 
-	if (!tfm->has_key)
+	if (!tfm->has_key && crypto_skcipher_has_setkey(tfm->skcipher))
 		return -ENOKEY;
 
 	return skcipher_accept_parent_common(private, sk);
-- 
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

Re: [PATCH 2/2] crypto: algif_skcipher - Add key check exception for cipher_null

[Milan Broz]

On 01/11/2016 02:29 PM, Herbert Xu wrote:
> This patch adds an exception to the key check so that cipher_null
> users may continue to use algif_skcipher without setting a key.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
> index 110bab4..4a5bdb6 100644
> --- a/crypto/algif_skcipher.c
> +++ b/crypto/algif_skcipher.c
> @@ -978,7 +978,7 @@ static int skcipher_accept_parent(void *private, struct sock *sk)
>  {
>  	struct skcipher_tfm *tfm = private;
>  
> -	if (!tfm->has_key)
> +	if (!tfm->has_key && crypto_skcipher_has_setkey(tfm->skcipher))
>  		return -ENOKEY;
>  
>  	return skcipher_accept_parent_common(private, sk);

Reported-and-tested-by: Milan Broz <gmazyland@gmail.com>

Thanks,
Milan

[PATCH 1/2] crypto: hash - Add crypto_ahash_has_setkey

[Herbert Xu]

This patch adds a way for ahash users to determine whether a key
is required by a crypto_ahash transform.

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

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 9c1dc8d..d19b523 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -451,6 +451,7 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 	struct ahash_alg *alg = crypto_ahash_alg(hash);
 
 	hash->setkey = ahash_nosetkey;
+	hash->has_setkey = false;
 	hash->export = ahash_no_export;
 	hash->import = ahash_no_import;
 
@@ -463,8 +464,10 @@ static int crypto_ahash_init_tfm(struct crypto_tfm *tfm)
 	hash->finup = alg->finup ?: ahash_def_finup;
 	hash->digest = alg->digest;
 
-	if (alg->setkey)
+	if (alg->setkey) {
 		hash->setkey = alg->setkey;
+		hash->has_setkey = true;
+	}
 	if (alg->export)
 		hash->export = alg->export;
 	if (alg->import)
diff --git a/crypto/shash.c b/crypto/shash.c
index ecb1e3d..88a27de 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -355,8 +355,10 @@ int crypto_init_shash_ops_async(struct crypto_tfm *tfm)
 	crt->finup = shash_async_finup;
 	crt->digest = shash_async_digest;
 
-	if (alg->setkey)
+	if (alg->setkey) {
 		crt->setkey = shash_async_setkey;
+		crt->has_setkey = true;
+	}
 	if (alg->export)
 		crt->export = shash_async_export;
 	if (alg->import)
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 3d69c93..6361892 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -204,6 +204,7 @@ struct crypto_ahash {
 		      unsigned int keylen);
 
 	unsigned int reqsize;
+	bool has_setkey;
 	struct crypto_tfm base;
 };
 
@@ -375,6 +376,11 @@ static inline void *ahash_request_ctx(struct ahash_request *req)
 int crypto_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
 			unsigned int keylen);
 
+static inline bool crypto_ahash_has_setkey(struct crypto_ahash *tfm)
+{
+	return tfm->has_setkey;
+}
+
 /**
  * crypto_ahash_finup() - update and finalize message digest
  * @req: reference to the ahash_request handle that holds all information
-- 
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

[PATCH 2/2] crypto: algif_hash - Require setkey before accept(2)

[Herbert Xu]

Hash implementations that require a key may crash if you use
them without setting a key.  This patch adds the necessary checks
so that if you do attempt to use them without a key that we return
-ENOKEY instead of proceeding.

This patch also 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_hash.c b/crypto/algif_hash.c
index b4c24fe..46637be 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -34,6 +34,11 @@ struct hash_ctx {
 	struct ahash_request req;
 };
 
+struct algif_hash_tfm {
+	struct crypto_ahash *hash;
+	bool has_key;
+};
+
 static int hash_sendmsg(struct socket *sock, struct msghdr *msg,
 			size_t ignored)
 {
@@ -235,22 +240,151 @@ static struct proto_ops algif_hash_ops = {
 	.accept		=	hash_accept,
 };
 
+static int hash_check_key(struct socket *sock)
+{
+	int err;
+	struct sock *psk;
+	struct alg_sock *pask;
+	struct algif_hash_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 hash_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
+			      size_t size)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_sendmsg(sock, msg, size);
+}
+
+static ssize_t hash_sendpage_nokey(struct socket *sock, struct page *page,
+				   int offset, size_t size, int flags)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_sendpage(sock, page, offset, size, flags);
+}
+
+static int hash_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
+			      size_t ignored, int flags)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_recvmsg(sock, msg, ignored, flags);
+}
+
+static int hash_accept_nokey(struct socket *sock, struct socket *newsock,
+			     int flags)
+{
+	int err;
+
+	err = hash_check_key(sock);
+	if (err)
+		return err;
+
+	return hash_accept(sock, newsock, flags);
+}
+
+static struct proto_ops algif_hash_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,
+	.setsockopt	=	sock_no_setsockopt,
+	.poll		=	sock_no_poll,
+
+	.release	=	af_alg_release,
+	.sendmsg	=	hash_sendmsg_nokey,
+	.sendpage	=	hash_sendpage_nokey,
+	.recvmsg	=	hash_recvmsg_nokey,
+	.accept		=	hash_accept_nokey,
+};
+
 static void *hash_bind(const char *name, u32 type, u32 mask)
 {
-	return crypto_alloc_ahash(name, type, mask);
+	struct algif_hash_tfm *tfm;
+	struct crypto_ahash *hash;
+
+	tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
+	if (!tfm)
+		return ERR_PTR(-ENOMEM);
+
+	hash = crypto_alloc_ahash(name, type, mask);
+	if (IS_ERR(hash)) {
+		kfree(tfm);
+		return ERR_CAST(hash);
+	}
+
+	tfm->hash = hash;
+
+	return tfm;
 }
 
 static void hash_release(void *private)
 {
-	crypto_free_ahash(private);
+	struct algif_hash_tfm *tfm = private;
+
+	crypto_free_ahash(tfm->hash);
+	kfree(tfm);
 }
 
 static int hash_setkey(void *private, const u8 *key, unsigned int keylen)
 {
-	return crypto_ahash_setkey(private, key, keylen);
+	struct algif_hash_tfm *tfm = private;
+	int err;
+
+	err = crypto_ahash_setkey(tfm->hash, key, keylen);
+	tfm->has_key = !err;
+
+	return err;
 }
 
-static void hash_sock_destruct(struct sock *sk)
+static void hash_sock_destruct_common(struct sock *sk)
 {
 	struct alg_sock *ask = alg_sk(sk);
 	struct hash_ctx *ctx = ask->private;
@@ -258,15 +392,40 @@ static void hash_sock_destruct(struct sock *sk)
 	sock_kzfree_s(sk, ctx->result,
 		      crypto_ahash_digestsize(crypto_ahash_reqtfm(&ctx->req)));
 	sock_kfree_s(sk, ctx, ctx->len);
+}
+
+static void hash_sock_destruct(struct sock *sk)
+{
+	hash_sock_destruct_common(sk);
 	af_alg_release_parent(sk);
 }
 
-static int hash_accept_parent(void *private, struct sock *sk)
+static void hash_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 hash_sock_destruct_nokey(struct sock *sk)
+{
+	hash_sock_destruct_common(sk);
+	hash_release_parent_nokey(sk);
+}
+
+static int hash_accept_parent_common(void *private, struct sock *sk)
 {
 	struct hash_ctx *ctx;
 	struct alg_sock *ask = alg_sk(sk);
-	unsigned len = sizeof(*ctx) + crypto_ahash_reqsize(private);
-	unsigned ds = crypto_ahash_digestsize(private);
+	struct algif_hash_tfm *tfm = private;
+	struct crypto_ahash *hash = tfm->hash;
+	unsigned len = sizeof(*ctx) + crypto_ahash_reqsize(hash);
+	unsigned ds = crypto_ahash_digestsize(hash);
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
@@ -286,7 +445,7 @@ static int hash_accept_parent(void *private, struct sock *sk)
 
 	ask->private = ctx;
 
-	ahash_request_set_tfm(&ctx->req, private);
+	ahash_request_set_tfm(&ctx->req, hash);
 	ahash_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				   af_alg_complete, &ctx->completion);
 
@@ -295,12 +454,38 @@ static int hash_accept_parent(void *private, struct sock *sk)
 	return 0;
 }
 
+static int hash_accept_parent(void *private, struct sock *sk)
+{
+	struct algif_hash_tfm *tfm = private;
+
+	if (!tfm->has_key && crypto_ahash_has_setkey(tfm->hash))
+		return -ENOKEY;
+
+	return hash_accept_parent_common(private, sk);
+}
+
+static int hash_accept_parent_nokey(void *private, struct sock *sk)
+{
+	int err;
+
+	err = hash_accept_parent_common(private, sk);
+	if (err)
+		goto out;
+
+	sk->sk_destruct = hash_sock_destruct_nokey;
+
+out:
+	return err;
+}
+
 static const struct af_alg_type algif_type_hash = {
 	.bind		=	hash_bind,
 	.release	=	hash_release,
 	.setkey		=	hash_setkey,
 	.accept		=	hash_accept_parent,
+	.accept_nokey	=	hash_accept_parent_nokey,
 	.ops		=	&algif_hash_ops,
+	.ops_nokey	=	&algif_hash_ops_nokey,
 	.name		=	"hash",
 	.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

Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2)

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6524 bytes --]

Hi Herbert,

[auto build test ERROR on crypto/master]
[also build test ERROR on v4.4-rc8 next-20160108]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Herbert-Xu/crypto-hash-Add-crypto_ahash_has_setkey/20160108-213436
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git master
config: x86_64-randconfig-i0-201601 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   crypto/algif_hash.c: In function 'hash_check_key':
>> crypto/algif_hash.c:252:9: error: 'struct alg_sock' has no member named 'refcnt'
     if (ask->refcnt)
            ^
   crypto/algif_hash.c:264:11: error: 'struct alg_sock' has no member named 'refcnt'
     if (!pask->refcnt++)
              ^
   crypto/algif_hash.c:267:5: error: 'struct alg_sock' has no member named 'refcnt'
     ask->refcnt = 1;
        ^
   crypto/algif_hash.c: In function 'hash_release_parent_nokey':
   crypto/algif_hash.c:407:10: error: 'struct alg_sock' has no member named 'refcnt'
     if (!ask->refcnt) {
             ^
   crypto/algif_hash.c: At top level:
>> crypto/algif_hash.c:486:2: error: unknown field 'accept_nokey' specified in initializer
     .accept_nokey = hash_accept_parent_nokey,
     ^
>> crypto/algif_hash.c:486:18: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .accept_nokey = hash_accept_parent_nokey,
                     ^
   crypto/algif_hash.c:486:18: note: (near initialization for 'algif_type_hash.setauthsize')
>> crypto/algif_hash.c:488:2: error: unknown field 'ops_nokey' specified in initializer
     .ops_nokey = &algif_hash_ops_nokey,
     ^
   crypto/algif_hash.c:488:15: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
     .ops_nokey = &algif_hash_ops_nokey,
                  ^
   crypto/algif_hash.c:488:15: note: (near initialization for 'algif_type_hash.owner')

vim +252 crypto/algif_hash.c

   246		struct sock *psk;
   247		struct alg_sock *pask;
   248		struct algif_hash_tfm *tfm;
   249		struct sock *sk = sock->sk;
   250		struct alg_sock *ask = alg_sk(sk);
   251	
 > 252		if (ask->refcnt)
   253			return 0;
   254	
   255		psk = ask->parent;
   256		pask = alg_sk(ask->parent);
   257		tfm = pask->private;
   258	
   259		err = -ENOKEY;
   260		lock_sock(psk);
   261		if (!tfm->has_key)
   262			goto unlock;
   263	
   264		if (!pask->refcnt++)
   265			sock_hold(psk);
   266	
 > 267		ask->refcnt = 1;
   268		sock_put(psk);
   269	
   270		err = 0;
   271	
   272	unlock:
   273		release_sock(psk);
   274	
   275		return err;
   276	}
   277	
   278	static int hash_sendmsg_nokey(struct socket *sock, struct msghdr *msg,
   279				      size_t size)
   280	{
   281		int err;
   282	
   283		err = hash_check_key(sock);
   284		if (err)
   285			return err;
   286	
   287		return hash_sendmsg(sock, msg, size);
   288	}
   289	
   290	static ssize_t hash_sendpage_nokey(struct socket *sock, struct page *page,
   291					   int offset, size_t size, int flags)
   292	{
   293		int err;
   294	
   295		err = hash_check_key(sock);
   296		if (err)
   297			return err;
   298	
   299		return hash_sendpage(sock, page, offset, size, flags);
   300	}
   301	
   302	static int hash_recvmsg_nokey(struct socket *sock, struct msghdr *msg,
   303				      size_t ignored, int flags)
   304	{
   305		int err;
   306	
   307		err = hash_check_key(sock);
   308		if (err)
   309			return err;
   310	
   311		return hash_recvmsg(sock, msg, ignored, flags);
   312	}
   313	
   314	static int hash_accept_nokey(struct socket *sock, struct socket *newsock,
   315				     int flags)
   316	{
   317		int err;
   318	
   319		err = hash_check_key(sock);
   320		if (err)
   321			return err;
   322	
   323		return hash_accept(sock, newsock, flags);
   324	}
   325	
   326	static struct proto_ops algif_hash_ops_nokey = {
   327		.family		=	PF_ALG,
   328	
   329		.connect	=	sock_no_connect,
   330		.socketpair	=	sock_no_socketpair,
   331		.getname	=	sock_no_getname,
   332		.ioctl		=	sock_no_ioctl,
   333		.listen		=	sock_no_listen,
   334		.shutdown	=	sock_no_shutdown,
   335		.getsockopt	=	sock_no_getsockopt,
   336		.mmap		=	sock_no_mmap,
   337		.bind		=	sock_no_bind,
   338		.setsockopt	=	sock_no_setsockopt,
   339		.poll		=	sock_no_poll,
   340	
   341		.release	=	af_alg_release,
   342		.sendmsg	=	hash_sendmsg_nokey,
   343		.sendpage	=	hash_sendpage_nokey,
   344		.recvmsg	=	hash_recvmsg_nokey,
   345		.accept		=	hash_accept_nokey,
   346	};
   347	
   348	static void *hash_bind(const char *name, u32 type, u32 mask)
   349	{
   350		struct algif_hash_tfm *tfm;
   351		struct crypto_ahash *hash;
   352	
   353		tfm = kzalloc(sizeof(*tfm), GFP_KERNEL);
   354		if (!tfm)
   355			return ERR_PTR(-ENOMEM);
   356	
   357		hash = crypto_alloc_ahash(name, type, mask);
   358		if (IS_ERR(hash)) {
   359			kfree(tfm);
   360			return ERR_CAST(hash);
   361		}
   362	
   363		tfm->hash = hash;
   364	
   365		return tfm;
   366	}
   367	
   368	static void hash_release(void *private)
   369	{
   370		struct algif_hash_tfm *tfm = private;
   371	
   372		crypto_free_ahash(tfm->hash);
   373		kfree(tfm);
   374	}
   375	
   376	static int hash_setkey(void *private, const u8 *key, unsigned int keylen)
   377	{
   378		struct algif_hash_tfm *tfm = private;
   379		int err;
   380	
   381		err = crypto_ahash_setkey(tfm->hash, key, keylen);
   382		tfm->has_key = !err;
   383	
   384		return err;
   385	}
   386	
   387	static void hash_sock_destruct_common(struct sock *sk)
   388	{
   389		struct alg_sock *ask = alg_sk(sk);
   390		struct hash_ctx *ctx = ask->private;
   391	
   392		sock_kzfree_s(sk, ctx->result,
   393			      crypto_ahash_digestsize(crypto_ahash_reqtfm(&ctx->req)));
   394		sock_kfree_s(sk, ctx, ctx->len);
   395	}
   396	
   397	static void hash_sock_destruct(struct sock *sk)
   398	{
   399		hash_sock_destruct_common(sk);
   400		af_alg_release_parent(sk);
   401	}
   402	
   403	static void hash_release_parent_nokey(struct sock *sk)
   404	{
   405		struct alg_sock *ask = alg_sk(sk);
   406	
 > 407		if (!ask->refcnt) {
   408			sock_put(ask->parent);
   409			return;
   410		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23798 bytes --]

Re: [PATCH 2/2] crypto: algif_hash - Require setkey before accept(2)

[Milan Broz]

On 01/08/2016 02:31 PM, Herbert Xu wrote:
> Hash implementations that require a key may crash if you use
> them without setting a key.  This patch adds the necessary checks
> so that if you do attempt to use them without a key that we return
> -ENOKEY instead of proceeding.
> 
> This patch also adds a compatibility path to support old applications
> that do acept(2) before setkey.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Reported-and-tested-by: Milan Broz <gmazyland@gmail.com>

(tested both patches, ditto for previous skcipher series)

Thanks,
Milan