Re: use-after-free in hash_sock_destruct
From: Dmitry Vyukov
Date: Tue Dec 29 2015 - 10:29:12 EST
On Tue, Dec 29, 2015 at 3:40 PM, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Dec 17, 2015 at 01:59:50PM +0100, Dmitry Vyukov wrote:
>>
>> The following program causes use-after-free in hash_sock_destruct:
>
> This patch should fix the problem. AFAIK everything that you have
> reported should now be fixed. If you still have issues please
> resubmit them (and please cc me). Thanks!
Thanks Herbert!
I will do another round of crypto testing with this patch (on top of
the other two patches) and report if I see any bugs.
> ---8<---
> Subject: crypto: af_alg - Disallow bind/setkey/... after accept(2)
>
> 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: stable@xxxxxxxxxxxxxxx
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index a8e7aa3..f5a2426 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,
> @@ -188,14 +212,41 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
> if (copy_from_user(key, ukey, keylen))
> goto out;
>
> + err = -EBUSY;
> + lock_sock(sk);
> + if (ask->refcnt)
> + goto unlock;
> +
> err = type->setkey(ask->private, key, keylen);
>
> +unlock:
> + release_sock(sk);
> +
> out:
> sock_kzfree_s(sk, key, keylen);
>
> 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 = -EBUSY;
> + lock_sock(sk);
> + if (ask->refcnt)
> + goto unlock;
> +
> + err = type->setauthsize(ask->private, size);
> +
> +unlock:
> + release_sock(sk);
> +
> + return err;
> +}
> +
> static int alg_setsockopt(struct socket *sock, int level, int optname,
> char __user *optval, unsigned int optlen)
> {
> @@ -224,7 +275,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 +315,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 <herbert@xxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxx
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/