crypto: use-after-free in alg_bind

[Dmitry Vyukov]

Hello,

On commit 8513342170278468bac126640a5d2d12ffbff106
+ crypto: algif_skcipher - Use new skcipher interface
+ crypto: algif_skcipher - Require setkey before accept(2)
+ crypto: af_alg - Disallow bind/setkey/... after accept(2)

The following program causes use-after-free in alg_bind and later
terminates 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>

int fd;

void *thr(void *arg)
{
switch ((long)arg) {
case 0:
fd = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
case 1:
*(uint16_t*)0x20000000 = (uint16_t)0x26;
memcpy((void*)0x20000002,
"\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
*(uint32_t*)0x20000010 = (uint32_t)0x2a;
*(uint32_t*)0x20000014 = (uint32_t)0x8;
memcpy((void*)0x20000018,
"\x65\x63\x62\x28\x61\x65\x73\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\x00\x00\x00\x00",
64);
syscall(SYS_bind, fd, 0x20000000ul, 0x58ul, 0, 0, 0);
break;
case 2:
syscall(SYS_accept4, fd, 0, 0, 0x80000ul, 0, 0);
break;
}
return 0;
}

int main()
{
long i;
pthread_t th[6];

syscall(SYS_mmap, 0x20000000ul, 0x1000ul, 0x3ul, 0x32ul,
0xfffffffffffffffful, 0x0ul);
for (i = 0; i < 6; i++)
pthread_create(&th[i], 0, thr, (void*)(i%3));
usleep(10000);
return 0;
}


==================================================================
BUG: KASAN: use-after-free in __lock_acquire+0x39db/0x3ca0 at addr
ffff880033e94f60
Read of size 8 by task a.out/7532
=============================================================================
BUG kmalloc-2048 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in sk_prot_alloc+0x1ed/0x340 age=1 cpu=1 pid=7532
[< inline >] kmalloc include/linux/slab.h:463
[< none >] sk_prot_alloc+0x1ed/0x340 net/core/sock.c:1354
[< none >] sk_alloc+0x3a/0x6b0 net/core/sock.c:1419
[< none >] alg_create+0x93/0x170 crypto/af_alg.c:370
[< none >] __sock_create+0x37c/0x640 net/socket.c:1162
[< inline >] sock_create net/socket.c:1202
[< inline >] SYSC_socket net/socket.c:1232
[< none >] SyS_socket+0xef/0x1b0 net/socket.c:1212

INFO: Freed in sk_destruct+0x3d7/0x490 age=1 cpu=0 pid=7531
[< none >] kfree+0x26a/0x290 mm/slub.c:3662
[< inline >] sk_prot_free net/core/sock.c:1391
[< none >] sk_destruct+0x3d7/0x490 net/core/sock.c:1467
[< none >] __sk_free+0x57/0x200 net/core/sock.c:1475
[< none >] sk_free+0x30/0x40 net/core/sock.c:1486
[< inline >] sock_put include/net/sock.h:1627
[< none >] af_alg_release+0x5b/0x70 crypto/af_alg.c:123
[< none >] sock_release+0x8d/0x1d0 net/socket.c:571
[< none >] sock_close+0x16/0x20 net/socket.c:1022
[< none >] __fput+0x233/0x780 fs/file_table.c:208
[< none >] ____fput+0x15/0x20 fs/file_table.c:244
[< none >] task_work_run+0x16b/0x200 kernel/task_work.c:115
[< inline >] tracehook_notify_resume include/linux/tracehook.h:191
[< none >] exit_to_usermode_loop+0x180/0x1a0
arch/x86/entry/common.c:251
[< inline >] prepare_exit_to_usermode arch/x86/entry/common.c:282
[< none >] syscall_return_slowpath+0x19f/0x210
arch/x86/entry/common.c:344
[< none >] int_ret_from_sys_call+0x25/0x9f
arch/x86/entry/entry_64.S:281

INFO: Slab 0xffffea0000cfa400 objects=13 used=8 fp=0xffff880033e94ec0
flags=0x1fffc0000004080
INFO: Object 0xffff880033e94ec0 @offset=20160 fp=0xffff880033e96c48
CPU: 1 PID: 7532 Comm: a.out Tainted: G B 4.4.0-rc7+ #181
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
00000000ffffffff ffff88006a49fa10 ffffffff8289d9dd ffff88003e805200
ffff880033e94ec0 ffff880033e90000 ffff88006a49fa40 ffffffff816c8e24
ffff88003e805200 ffffea0000cfa400 ffff880033e94ec0 ffffffff88b866e0

Call Trace:
[<ffffffff816d239e>] __asan_report_load8_noabort+0x3e/0x40
mm/kasan/report.c:295
[<ffffffff813ee5ab>] __lock_acquire+0x39db/0x3ca0 kernel/locking/lockdep.c:3092
[<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0 kernel/locking/lockdep.c:3585
[< inline >] __raw_spin_lock_bh include/linux/spinlock_api_smp.h:137
[<ffffffff85c8de0f>] _raw_spin_lock_bh+0x3f/0x50 kernel/locking/spinlock.c:175
[< inline >] spin_lock_bh include/linux/spinlock.h:307
[<ffffffff84b654d8>] lock_sock_nested+0x48/0x120 net/core/sock.c:2434
[< inline >] lock_sock include/net/sock.h:1481
[<ffffffff827ddc1a>] alg_bind+0x1aa/0x3f0 crypto/af_alg.c:182
[<ffffffff84b5d84a>] SYSC_bind+0x1ea/0x250 net/socket.c:1376
[<ffffffff84b5ff74>] SyS_bind+0x24/0x30 net/socket.c:1362
==================================================================

[Herbert Xu]

On Tue, Dec 29, 2015 at 09:19:22PM +0100, Dmitry Vyukov wrote:
> Hello,
>
> On commit 8513342170278468bac126640a5d2d12ffbff106
> + crypto: algif_skcipher - Use new skcipher interface
> + crypto: algif_skcipher - Require setkey before accept(2)
> + crypto: af_alg - Disallow bind/setkey/... after accept(2)
>
> The following program causes use-after-free in alg_bind and later
> terminates kernel:

Please double-check that you have the last patch applied correctly,
as I cannot reproduce the crash with your program.

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

[Dmitry Vyukov]

On Wed, Dec 30, 2015 at 2:24 AM, Herbert Xu <her...@gondor.apana.org.au> wrote:
> On Tue, Dec 29, 2015 at 09:19:22PM +0100, Dmitry Vyukov wrote:
>> Hello,
>>
>> On commit 8513342170278468bac126640a5d2d12ffbff106
>> + crypto: algif_skcipher - Use new skcipher interface
>> + crypto: algif_skcipher - Require setkey before accept(2)
>> + crypto: af_alg - Disallow bind/setkey/... after accept(2)
>>
>> The following program causes use-after-free in alg_bind and later
>> terminates kernel:
>
> Please double-check that you have the last patch applied correctly,
> as I cannot reproduce the crash with your program.

I am pretty sure I have the last patch applied. The use-after-frees
that it was supposed to fix have gone. Below are combined changed to
crypto/ files that I have on top of
8513342170278468bac126640a5d2d12ffbff106.

This use-after-free does not reproduce on every run. It seems to be
triggered by some race. Try to run the program in a parallel loop.
I use stress tool for this:
https://github.com/golang/tools/blob/master/cmd/stress/stress.go
If you have Go toolchain installed, then then following will do:
$ go get golang.org/x/tools/cmd/stress
$ stress -p 16 ./a.out


diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a8e7aa3..82a7dcd 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -125,6 +125,23 @@ int af_alg_release(struct socket *sock)
}
EXPORT_SYMBOL_GPL(af_alg_release);

+void af_alg_release_parent(struct sock *sk)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ bool last;
+
+ sk = ask->parent;
+ ask = alg_sk(sk);
+
+ lock_sock(sk);
+ last = !--ask->refcnt;
+ release_sock(sk);
+
+ if (last)
+ sock_put(sk);
+}
+EXPORT_SYMBOL_GPL(af_alg_release_parent);
+
static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
{
const u32 forbidden = CRYPTO_ALG_INTERNAL;
@@ -133,6 +150,7 @@ static int alg_bind(struct socket *sock, struct
sockaddr *uaddr, int addr_len)
struct sockaddr_alg *sa = (void *)uaddr;
const struct af_alg_type *type;
void *private;
+ int err;

if (sock->state == SS_CONNECTED)
return -EINVAL;
@@ -160,16 +178,22 @@ static int alg_bind(struct socket *sock, struct
sockaddr *uaddr, int addr_len)
return PTR_ERR(private);
}

+ err = -EBUSY;
lock_sock(sk);
+ if (ask->refcnt)
+ goto unlock;

swap(ask->type, type);
swap(ask->private, private);

+ err = 0;
+
+unlock:
release_sock(sk);

alg_do_release(type, private);

- return 0;
+ return err;
}

static int alg_setkey(struct sock *sk, char __user *ukey,
@@ -196,6 +220,16 @@ out:
return err;
}

+static int alg_setauthsize(struct sock *sk, unsigned int size)
+{
+ int err;
+ struct alg_sock *ask = alg_sk(sk);
+ const struct af_alg_type *type = ask->type;
+
+ err = type->setauthsize(ask->private, size);
+ return err;
+}
+
static int alg_setsockopt(struct socket *sock, int level, int optname,
char __user *optval, unsigned int optlen)
{
@@ -210,6 +244,11 @@ static int alg_setsockopt(struct socket *sock,
int level, int optname,
if (level != SOL_ALG || !type)
goto unlock;

+ if (ask->refcnt) {
+ err = -EBUSY;
+ goto unlock;
+ }
+
switch (optname) {
case ALG_SET_KEY:
if (sock->state == SS_CONNECTED)
@@ -224,7 +263,7 @@ static int alg_setsockopt(struct socket *sock, int
level, int optname,
goto unlock;
if (!type->setauthsize)
goto unlock;
- err = type->setauthsize(ask->private, optlen);
+ err = alg_setauthsize(sk, optlen);
}

unlock:
@@ -264,7 +303,8 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)

sk2->sk_family = PF_ALG;

- sock_hold(sk);
+ if (!ask->refcnt++)
+ sock_hold(sk);
alg_sk(sk2)->parent = sk;
alg_sk(sk2)->type = type;

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 634b4d1..df483f9 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);

[Herbert Xu]

On Wed, Dec 30, 2015 at 11:19:45AM +0100, Dmitry Vyukov wrote:
>
> This use-after-free does not reproduce on every run. It seems to be
> triggered by some race. Try to run the program in a parallel loop.
> I use stress tool for this:
> https://github.com/golang/tools/blob/master/cmd/stress/stress.go
> If you have Go toolchain installed, then then following will do:
> $ go get golang.org/x/tools/cmd/stress
> $ stress -p 16 ./a.out

I've tried a few thousand instances of it but still no luck.

>
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index a8e7aa3..82a7dcd 100644

There are a few missing hunks in your patch and the patch to
if_alg.h is missing.

So please start with the current crypto tree and then apply the
latest version (v2) of "crypto: af_alg - Disallow bind/setkey/...
after accept(2)" and try again.

[Dmitry Vyukov]

On Wed, Dec 30, 2015 at 11:53 AM, Herbert Xu
<her...@gondor.apana.org.au> wrote:
> On Wed, Dec 30, 2015 at 11:19:45AM +0100, Dmitry Vyukov wrote:
>>
>> This use-after-free does not reproduce on every run. It seems to be
>> triggered by some race. Try to run the program in a parallel loop.
>> I use stress tool for this:
>> https://github.com/golang/tools/blob/master/cmd/stress/stress.go
>> If you have Go toolchain installed, then then following will do:
>> $ go get golang.org/x/tools/cmd/stress
>> $ stress -p 16 ./a.out
>
> I've tried a few thousand instances of it but still no luck.
>>
>>
>> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
>> index a8e7aa3..82a7dcd 100644
>
> There are a few missing hunks in your patch and the patch to
> if_alg.h is missing.
>
> So please start with the current crypto tree and then apply the
> latest version (v2) of "crypto: af_alg - Disallow bind/setkey/...
> after accept(2)" and try again.

I forgot to diff include/crypto/if_alg.h, but the changes are there
(otherwise all references to refcnt would not compile). Also I moved
ask->refcnt checks to alg_setsockopt to fix the deadlock, I believe
that's the missing chunks you refer to. I can retest if you wish, but
I don't think that my changes can affect the reported use-after-free.
Do you?


diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 018afb2..589716f 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -30,6 +30,8 @@ struct alg_sock {

struct sock *parent;

+ unsigned int refcnt;
+
const struct af_alg_type *type;
void *private;
};
@@ -67,6 +69,7 @@ int af_alg_register_type(const struct af_alg_type *type);
int af_alg_unregister_type(const struct af_alg_type *type);

int af_alg_release(struct socket *sock);
+void af_alg_release_parent(struct sock *sk);
int af_alg_accept(struct sock *sk, struct socket *newsock);

int af_alg_make_sg(struct af_alg_sgl *sgl, struct iov_iter *iter, int len);
@@ -83,11 +86,6 @@ static inline struct alg_sock *alg_sk(struct sock *sk)
return (struct alg_sock *)sk;
}

-static inline void af_alg_release_parent(struct sock *sk)
-{
- sock_put(alg_sk(sk)->parent);
-}
-
static inline void af_alg_init_completion(struct af_alg_completion *completion)
{
init_completion(&completion->completion);

[Herbert Xu]

On Wed, Dec 30, 2015 at 11:58:58AM +0100, Dmitry Vyukov wrote:
>
> I forgot to diff include/crypto/if_alg.h, but the changes are there
> (otherwise all references to refcnt would not compile). Also I moved
> ask->refcnt checks to alg_setsockopt to fix the deadlock, I believe
> that's the missing chunks you refer to. I can retest if you wish, but
> I don't think that my changes can affect the reported use-after-free.
> Do you?

OK I see the problem now. When accept fails we free the socket
twice.

---8<---
Subject: crypto: af_alg - Fix socket double-free when accept fails

When we fail an accept(2) call we will end up freeing the socket
twice, once due to the direct sk_free call and once again through
newsock.

This patch fixes this by removing the sk_free call.

Cc: sta...@vger.kernel.org
Reported-by: Dmitry Vyukov <dvy...@google.com>
Signed-off-by: Herbert Xu <her...@gondor.apana.org.au>

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 7b5b592..eaf98e2 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -285,10 +285,8 @@ int af_alg_accept(struct sock *sk, struct socket *newsock)
security_sk_clone(sk, sk2);

err = type->accept(ask->private, sk2);
- if (err) {
- sk_free(sk2);
+ if (err)
goto unlock;
- }

sk2->sk_family = PF_ALG;

[Dmitry Vyukov]

On Wed, Dec 30, 2015 at 1:24 PM, Herbert Xu <her...@gondor.apana.org.au> wrote:
> On Wed, Dec 30, 2015 at 11:58:58AM +0100, Dmitry Vyukov wrote:
>>
>> I forgot to diff include/crypto/if_alg.h, but the changes are there
>> (otherwise all references to refcnt would not compile). Also I moved
>> ask->refcnt checks to alg_setsockopt to fix the deadlock, I believe
>> that's the missing chunks you refer to. I can retest if you wish, but
>> I don't think that my changes can affect the reported use-after-free.
>> Do you?
>
> OK I see the problem now. When accept fails we free the socket
> twice.

Great!

This seems to be a zero-day. Should we CC sta...@vger.kernel.org ?

Code in 03c8efc1ffeb6b82a22c1af8dd908af349563314 (Oct 19, 2010) contained:

+ sock_init_data(newsock, sk2);
+
+ err = type->accept(ask->private, sk2);
+ if (err) {
+ sk_free(sk2);
+ goto unlock;
+ }

There were no sock_graft call, but sock_init_data also sets
newsock->sk = sk2, which seems to be enough for the double-free.

[Herbert Xu]

On Wed, Dec 30, 2015 at 01:45:15PM +0100, Dmitry Vyukov wrote:
>
> This seems to be a zero-day. Should we CC sta...@vger.kernel.org ?
>
> Code in 03c8efc1ffeb6b82a22c1af8dd908af349563314 (Oct 19, 2010) contained:
>
> + sock_init_data(newsock, sk2);
> +
> + err = type->accept(ask->private, sk2);
> + if (err) {
> + sk_free(sk2);
> + goto unlock;
> + }
>
> There were no sock_graft call, but sock_init_data also sets
> newsock->sk = sk2, which seems to be enough for the double-free.

But accept only started failing because the newly added has_key
check. Otherwise the only way for it to fail is if we run out
of memory.

IOW the bug can only be easily triggered in the current crypto tree.

Cheers,