crypto: deadlock in alg_setsockopt

[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 creates an unkillable, deadlocked process:

// 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, 0x10000ul, 0x3ul,
0x32ul, 0xfffffffffffffffful, 0x0ul);
r[1] = syscall(SYS_socket, 0x26ul, 0x5ul, 0x0ul, 0, 0, 0);
*(uint16_t*)0x20000112 = (uint16_t)0x26;
memcpy((void*)0x20000114,
"\x73\x6b\x63\x69\x70\x68\x65\x72\x00\x00\x00\x00\x00\x00", 14);
*(uint32_t*)0x20000122 = (uint32_t)0x209;
*(uint32_t*)0x20000126 = (uint32_t)0x4e;
memcpy((void*)0x2000012a,
"\x65\x63\x62\x28\x61\x72\x63\x34\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",
64);
r[7] = syscall(SYS_bind, r[1], 0x20000112ul, 0x58ul, 0, 0, 0);
r[9] = syscall(SYS_setsockopt, r[1], 0x117ul, 0x1ul,
0x20003000ul, 0xd2ul, 0);
return 0;
}

root 28768 0.0 0.0 1144 4 pts/0 D+ 18:25 0:00 |
\_ ./a.out

# cat /proc/28768/stack
[<ffffffff84b65496>] __lock_sock+0xe6/0x160
[<ffffffff84b6560b>] lock_sock_nested+0xfb/0x120
[<ffffffff827dd899>] alg_setsockopt+0x2a9/0x3d0
[<ffffffff84b605b8>] SyS_setsockopt+0x158/0x240
[<ffffffff85c8ebb6>] entry_SYSCALL_64_fastpath+0x16/0x7a

[Herbert Xu]

On Tue, Dec 29, 2015 at 07:36:14PM +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)

OK there is a silly bug in the last patch. Here is an updated
version.

---8<---
Each af_alg parent socket obtained by socket(2) corresponds to a
tfm object once bind(2) has succeeded. An accept(2) call on that
parent socket creates a context which then uses the tfm object.

Therefore as long as any child sockets created by accept(2) exist
the parent socket must not be modified or freed.

This patch guarantees this by using locks and a reference count
on the parent socket. Any attempt to modify the parent socket will
fail with EBUSY.

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 a8e7aa3..7b5b592 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,
@@ -202,11 +226,15 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
struct sock *sk = sock->sk;
struct alg_sock *ask = alg_sk(sk);
const struct af_alg_type *type;
- int err = -ENOPROTOOPT;
+ int err = -EBUSY;

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

+ err = -ENOPROTOOPT;
if (level != SOL_ALG || !type)
goto unlock;

@@ -264,7 +292,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/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);
--
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 Mittwoch, 30. Dezember 2015, 11:47:53 schrieb Herbert Xu:

Hi Herbert,

> On Tue, Dec 29, 2015 at 07:36:14PM +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)
>
> OK there is a silly bug in the last patch. Here is an updated
> version.

With this patch, the AF_ALG interface stops working. I tested the HMAC
operation and I am unable to set the key with the following call:

ret = setsockopt(handle->tfmfd, SOL_ALG, ALG_SET_KEY, key, keylen);

This call returns EBUSY.

The test can be performed with [1] using the following call:

test/kcapi -x 3 -c "hmac(sha1)" -k 6e77ebd479da794707bc6cde3694f552ea892dab -p
31b62a797adbff6b8a358d2b5206e01fee079de8cdfc4695138bba163b4efbf30127343e7fd4fbc696c3d38d8f27f57c024b5056f726ceeb4c31d98e57751ec8cbe8904ee0f9b031ae6a0c55da5e062475b3d7832191d4057643ef5fa446801d59a04693e573a8159cd2416b7bd39c7f0fe63c599365e04d596c05736beaab58

Without the patch, all works.

[1] http://www.chronox.de/libkcapi.html

--
Ciao
Stephan

[Stephan Mueller]

Am Freitag, 1. Januar 2016, 21:12:40 schrieb Stephan Mueller:

Hi Herbert,

> Am Mittwoch, 30. Dezember 2015, 11:47:53 schrieb Herbert Xu:
>
> Hi Herbert,
>
> > On Tue, Dec 29, 2015 at 07:36:14PM +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)
> >
> > OK there is a silly bug in the last patch. Here is an updated
> > version.
>
> With this patch, the AF_ALG interface stops working. I tested the HMAC
> operation and I am unable to set the key with the following call:
>
> ret = setsockopt(handle->tfmfd, SOL_ALG, ALG_SET_KEY, key, keylen);
>
> This call returns EBUSY.

Please disregard my email. I did not update my library to the newly requested
order of performing the setkey before the accept call. After the update of my
library I can confirm that the modification works for all AF_ALG interfaces.

--
Ciao
Stephan