On Tue, Dec 29, 2015 at 4:28 PM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote: > 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. Doh! Now +Herbert >> ---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@xxxxxxxxxxxxxxxx. >> For more options, visit https://groups.google.com/d/optout. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html