WARNING in crypto_wait_for_test

[Dmitry Vyukov]

Hello,

The following program triggers a WARNING in crypto_wait_for_test:

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

int main()
{
long r0 = syscall(SYS_mmap, 0x20000000ul, 0x1000ul, 0x3ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
long r1 = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
*(uint16_t*)0x20000000 = 0x26;
memcpy((void*)0x20000002,
"\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
*(uint32_t*)0x20000010 = 0x1008;
*(uint32_t*)0x20000014 = 0x469b167b45d89a6;
memcpy((void*)0x20000018,
"\x63\x74\x72\x28\x64\x65\x73\x33\x5f\x65\x64\x65\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",
64);
long r7 = syscall(SYS_bind, r1, 0x20000000ul, 0x58ul, 0, 0, 0);
return 0;
}


------------[ cut here ]------------
WARNING: CPU: 1 PID: 11087 at crypto/algapi.c:343
crypto_wait_for_test+0xc4/0xf0()
Modules linked in:
CPU: 1 PID: 11087 Comm: a.out Tainted: G W 4.4.0-rc3+ #151
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
0000000000000001 ffff88006ca07a78 ffffffff82e0f4b8 0000000041b58ab3
ffffffff87a9a265 ffffffff82e0f406 ffff88003711e080 00000000ffffffff
ffffffff89913aa0 0000000000000001 0000000000000001 0000000000002b4f
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffff82e0f4b8>] dump_stack+0xb2/0xfa lib/dump_stack.c:50
[<ffffffff81373336>] warn_slowpath_common+0xe6/0x170 kernel/panic.c:460
[<ffffffff81373589>] warn_slowpath_null+0x29/0x30 kernel/panic.c:493
[<ffffffff82ba29e4>] crypto_wait_for_test+0xc4/0xf0 crypto/algapi.c:343
[<ffffffff82ba2d20>] crypto_register_instance+0x220/0x350 crypto/algapi.c:558
[<ffffffff82ba9564>] crypto_givcipher_default+0x4f4/0x620
crypto/ablkcipher.c:601
[<ffffffff82ba984a>] crypto_lookup_skcipher+0x1ba/0x2f0 crypto/ablkcipher.c:658
[<ffffffff82ba9aae>] crypto_alloc_ablkcipher+0x5e/0x1f0 crypto/ablkcipher.c:693
[<ffffffff82d2a875>] skcipher_bind+0x25/0x30 crypto/algif_skcipher.c:754
[<ffffffff82d28129>] alg_bind+0x1a9/0x410 crypto/af_alg.c:155
[<ffffffff856b3a6a>] SYSC_bind+0x20a/0x2c0 net/socket.c:1383
[<ffffffff856b70f4>] SyS_bind+0x24/0x30 net/socket.c:1369
[<ffffffff86a89fb6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
---[ end trace 49f86739a736fa2b ]---


strace:
socket(PF_ALG, SOCK_SEQPACKET, 0) = 3
bind(3, {sa_family=AF_ALG, sa_data="skcipher\0\0\0\0\0\0"}, 88) = -1
ENOENT (No such file or directory)


On commit 31ade3b83e1821da5fbb2f11b5b3d4ab2ec39db8 (Nov 29).

[Stephan Mueller]

Am Dienstag, 8. Dezember 2015, 12:12:27 schrieb Dmitry Vyukov:

Hi Dmitry,

>Hello,
>
>The following program triggers a WARNING in crypto_wait_for_test:
>
>// autogenerated by syzkaller (http://github.com/google/syzkaller)
>#include <syscall.h>
>#include <string.h>
>#include <stdint.h>
>
>int main()
>{
> long r0 = syscall(SYS_mmap, 0x20000000ul, 0x1000ul, 0x3ul,
>0x32ul, 0xfffffffffffffffful, 0x0ul);
> long r1 = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
> *(uint16_t*)0x20000000 = 0x26;
> memcpy((void*)0x20000002,
>"\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
> *(uint32_t*)0x20000010 = 0x1008;
> *(uint32_t*)0x20000014 = 0x469b167b45d89a6;

The error is triggered by this ^^^^ line. This line sets sockaddr_alg->mask to
some strange value. I assume that the mask does not allow the crypto API to
find the test.

Herbert, alg_bind currently blacklists one bit in the mask and type bit array.
Shouldn't we instead white-list the allowed bits?

>--
>To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
>the body of a message to majo...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html


Ciao
Stephan

[Herbert Xu]

On Thu, Dec 10, 2015 at 03:14:24AM +0100, Stephan Mueller wrote:
>
> Herbert, alg_bind currently blacklists one bit in the mask and type bit array.
> Shouldn't we instead white-list the allowed bits?

Well a bogus mask shouldn't lead to the warning anyway.

The warning in question is triggered only if crypto_schedule_test
returns NOTIFY_OK, which can only happen if try_module_get fails,
kzalloc fails, or if kthread_run fails. All of these would be
real bugs (well except for the kzalloc failure perhaps).

Thanks,
--
Email: Herbert Xu <her...@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[Stephan Mueller]

Am Montag, 14. Dezember 2015, 20:52:19 schrieb Herbert Xu:

Hi Herbert,

>On Thu, Dec 10, 2015 at 03:14:24AM +0100, Stephan Mueller wrote:
>> Herbert, alg_bind currently blacklists one bit in the mask and type bit
>> array. Shouldn't we instead white-list the allowed bits?
>
>Well a bogus mask shouldn't lead to the warning anyway.
>
>The warning in question is triggered only if crypto_schedule_test
>returns NOTIFY_OK, which can only happen if try_module_get fails,
>kzalloc fails, or if kthread_run fails. All of these would be
>real bugs (well except for the kzalloc failure perhaps).

But with the given code, when you remove the bogus mask setting (which in turn
leaves it as 0), the code works flawless.

>
>Thanks,


Ciao
Stephan

[Herbert Xu]

On Mon, Dec 14, 2015 at 11:45:02PM +0100, Stephan Mueller wrote:
>
> But with the given code, when you remove the bogus mask setting (which in turn
> leaves it as 0), the code works flawless.

Please find out exactly why this would be triggering the warning
because your previous explanation of not being able to find the
algorithm due to the mask does not make sense.

[Herbert Xu]

On Mon, Dec 14, 2015 at 11:45:02PM +0100, Stephan Mueller wrote:
>

> But with the given code, when you remove the bogus mask setting (which in turn
> leaves it as 0), the code works flawless.

OK turns out I was looking at the wrong WARN_ON line.

This particular bug is a legacy of the geniv construct we have
for ablkcipher.

It should disappear once we remove geniv which I will do as part
of the skcipher conversion.

[Herbert Xu]

On Fri, Dec 18, 2015 at 04:49:17PM +0800, Herbert Xu wrote:
>
> OK turns out I was looking at the wrong WARN_ON line.
>
> This particular bug is a legacy of the geniv construct we have
> for ablkcipher.
>
> It should disappear once we remove geniv which I will do as part
> of the skcipher conversion.

This patch should fix the problem.

---8<---
Subject: crypto: algif_skcipher - Use new skcipher interface

This patch replaces uses of ablkcipher with the new skcipher
interface.

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

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index bbb1b66..5c877d4 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -47,7 +47,7 @@ struct skcipher_ctx {
bool merge;
bool enc;

- struct ablkcipher_request req;
+ struct skcipher_request req;
};

struct skcipher_async_rsgl {
@@ -64,13 +64,13 @@ struct skcipher_async_req {
};

#define GET_SREQ(areq, ctx) (struct skcipher_async_req *)((char *)areq + \
- crypto_ablkcipher_reqsize(crypto_ablkcipher_reqtfm(&ctx->req)))
+ crypto_skcipher_reqsize(crypto_skcipher_reqtfm(&ctx->req)))

#define GET_REQ_SIZE(ctx) \
- crypto_ablkcipher_reqsize(crypto_ablkcipher_reqtfm(&ctx->req))
+ crypto_skcipher_reqsize(crypto_skcipher_reqtfm(&ctx->req))

#define GET_IV_SIZE(ctx) \
- crypto_ablkcipher_ivsize(crypto_ablkcipher_reqtfm(&ctx->req))
+ crypto_skcipher_ivsize(crypto_skcipher_reqtfm(&ctx->req))

#define MAX_SGL_ENTS ((4096 - sizeof(struct skcipher_sg_list)) / \
sizeof(struct scatterlist) - 1)
@@ -302,8 +302,8 @@ static int skcipher_sendmsg(struct socket *sock, struct msghdr *msg,
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct skcipher_ctx *ctx = ask->private;
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(&ctx->req);
- unsigned ivsize = crypto_ablkcipher_ivsize(tfm);
+ struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(&ctx->req);
+ unsigned ivsize = crypto_skcipher_ivsize(tfm);
struct skcipher_sg_list *sgl;
struct af_alg_control con = {};
long copied = 0;
@@ -507,7 +507,7 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
struct skcipher_sg_list *sgl;
struct scatterlist *sg;
struct skcipher_async_req *sreq;
- struct ablkcipher_request *req;
+ struct skcipher_request *req;
struct skcipher_async_rsgl *last_rsgl = NULL;
unsigned int txbufs = 0, len = 0, tx_nents = skcipher_all_sg_nents(ctx);
unsigned int reqlen = sizeof(struct skcipher_async_req) +
@@ -531,9 +531,9 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
}
sg_init_table(sreq->tsg, tx_nents);
memcpy(sreq->iv, ctx->iv, GET_IV_SIZE(ctx));
- ablkcipher_request_set_tfm(req, crypto_ablkcipher_reqtfm(&ctx->req));
- ablkcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
- skcipher_async_cb, sk);
+ skcipher_request_set_tfm(req, crypto_skcipher_reqtfm(&ctx->req));
+ skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+ skcipher_async_cb, sk);

while (iov_iter_count(&msg->msg_iter)) {
struct skcipher_async_rsgl *rsgl;
@@ -608,10 +608,10 @@ static int skcipher_recvmsg_async(struct socket *sock, struct msghdr *msg,
if (mark)
sg_mark_end(sreq->tsg + txbufs - 1);

- ablkcipher_request_set_crypt(req, sreq->tsg, sreq->first_sgl.sgl.sg,
- len, sreq->iv);
- err = ctx->enc ? crypto_ablkcipher_encrypt(req) :
- crypto_ablkcipher_decrypt(req);
+ skcipher_request_set_crypt(req, sreq->tsg, sreq->first_sgl.sgl.sg,
+ len, sreq->iv);
+ err = ctx->enc ? crypto_skcipher_encrypt(req) :
+ crypto_skcipher_decrypt(req);
if (err == -EINPROGRESS) {
atomic_inc(&ctx->inflight);
err = -EIOCBQUEUED;
@@ -632,7 +632,7 @@ static int skcipher_recvmsg_sync(struct socket *sock, struct msghdr *msg,
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
struct skcipher_ctx *ctx = ask->private;
- unsigned bs = crypto_ablkcipher_blocksize(crypto_ablkcipher_reqtfm(
+ unsigned bs = crypto_skcipher_blocksize(crypto_skcipher_reqtfm(
&ctx->req));
struct skcipher_sg_list *sgl;
struct scatterlist *sg;
@@ -669,14 +669,13 @@ static int skcipher_recvmsg_sync(struct socket *sock, struct msghdr *msg,
if (!used)
goto free;

- ablkcipher_request_set_crypt(&ctx->req, sg,
- ctx->rsgl.sg, used,
- ctx->iv);
+ skcipher_request_set_crypt(&ctx->req, sg, ctx->rsgl.sg, used,
+ ctx->iv);

err = af_alg_wait_for_completion(
ctx->enc ?
- crypto_ablkcipher_encrypt(&ctx->req) :
- crypto_ablkcipher_decrypt(&ctx->req),
+ crypto_skcipher_encrypt(&ctx->req) :
+ crypto_skcipher_decrypt(&ctx->req),
&ctx->completion);

free:
@@ -751,17 +750,17 @@ static struct proto_ops algif_skcipher_ops = {

static void *skcipher_bind(const char *name, u32 type, u32 mask)
{
- return crypto_alloc_ablkcipher(name, type, mask);
+ return crypto_alloc_skcipher(name, type, mask);
}

static void skcipher_release(void *private)
{
- crypto_free_ablkcipher(private);
+ crypto_free_skcipher(private);
}

static int skcipher_setkey(void *private, const u8 *key, unsigned int keylen)
{
- return crypto_ablkcipher_setkey(private, key, keylen);
+ return crypto_skcipher_setkey(private, key, keylen);
}

static void skcipher_wait(struct sock *sk)
@@ -778,13 +777,13 @@ static void skcipher_sock_destruct(struct sock *sk)
{
struct alg_sock *ask = alg_sk(sk);
struct skcipher_ctx *ctx = ask->private;
- struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(&ctx->req);
+ struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(&ctx->req);

if (atomic_read(&ctx->inflight))
skcipher_wait(sk);

skcipher_free_sgl(sk);
- sock_kzfree_s(sk, ctx->iv, crypto_ablkcipher_ivsize(tfm));
+ sock_kzfree_s(sk, ctx->iv, crypto_skcipher_ivsize(tfm));
sock_kfree_s(sk, ctx, ctx->len);
af_alg_release_parent(sk);
}
@@ -793,20 +792,20 @@ 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_ablkcipher_reqsize(private);
+ unsigned int len = sizeof(*ctx) + crypto_skcipher_reqsize(private);

ctx = sock_kmalloc(sk, len, GFP_KERNEL);
if (!ctx)
return -ENOMEM;

- ctx->iv = sock_kmalloc(sk, crypto_ablkcipher_ivsize(private),
+ ctx->iv = sock_kmalloc(sk, crypto_skcipher_ivsize(private),
GFP_KERNEL);
if (!ctx->iv) {
sock_kfree_s(sk, ctx, len);
return -ENOMEM;
}

- memset(ctx->iv, 0, crypto_ablkcipher_ivsize(private));
+ memset(ctx->iv, 0, crypto_skcipher_ivsize(private));

INIT_LIST_HEAD(&ctx->tsgl);
ctx->len = len;
@@ -819,9 +818,9 @@ static int skcipher_accept_parent(void *private, struct sock *sk)

ask->private = ctx;

- ablkcipher_request_set_tfm(&ctx->req, private);
- ablkcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
- af_alg_complete, &ctx->completion);
+ skcipher_request_set_tfm(&ctx->req, private);
+ skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+ af_alg_complete, &ctx->completion);

sk->sk_destruct = skcipher_sock_destruct;

[Stephan Mueller]

Am Freitag, 18. Dezember 2015, 19:16:57 schrieb Herbert Xu:

Hi Herbert,

> On Fri, Dec 18, 2015 at 04:49:17PM +0800, Herbert Xu wrote:
> > OK turns out I was looking at the wrong WARN_ON line.
> >
> > This particular bug is a legacy of the geniv construct we have
> > for ablkcipher.
> >
> > It should disappear once we remove geniv which I will do as part
> > of the skcipher conversion.
>
> This patch should fix the problem.

Yes, it does. Thanks.

Tested-by: <smue...@chronox.de>

--
Ciao
Stephan