crypto: use-after-free in alg_bind

Commit Message

Dmitry Vyukov Dec. 30, 2015, 10:19 a.m. UTC

On Wed, Dec 30, 2015 at 2:24 AM, Herbert Xu <[e-mail]> 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


         ctx->len = len;
@@ -818,7 +852,7 @@ static int skcipher_accept_parent(void *private,
struct sock *sk)

         ask->private = ctx;

-        skcipher_request_set_tfm(&ctx->req, private);
+        skcipher_request_set_tfm(&ctx->req, skcipher);
         skcipher_request_set_callback(&ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
                                       af_alg_complete, &ctx->completion);
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Herbert Xu Dec. 30, 2015, 10:53 a.m. UTC | #1

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.

Thanks,

Patch

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