Re: use-after-free in hash_sock_destruct

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux