From patchwork Wed Dec 30 10:19:45 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Vyukov X-Patchwork-Id: 7932381 X-Patchwork-Delegate: herbert@gondor.apana.org.au Return-Path: X-Original-To: patchwork-linux-crypto@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 0D8AA9F349 for ; Wed, 30 Dec 2015 10:20:13 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id DE3BA20109 for ; Wed, 30 Dec 2015 10:20:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7B445202EC for ; Wed, 30 Dec 2015 10:20:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754176AbbL3KUI (ORCPT ); Wed, 30 Dec 2015 05:20:08 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:35084 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752857AbbL3KUG (ORCPT ); Wed, 30 Dec 2015 05:20:06 -0500 Received: by mail-wm0-f47.google.com with SMTP id f206so33116999wmf.0 for ; Wed, 30 Dec 2015 02:20:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type; bh=iXE2vL9Wuef6hujk0SrXS1lmEY33LlAWY8v6gRULpKk=; b=ZNwtWHKFSV2R5xzY4xSbVLskUajc0sftmAdr58MDRHx8jpKkSivAd0/VML9bMb1KP2 4yxTeK0ievgEumB2ryvZm2QLixzMYChpEY+vghf3mnBedVTi2Sbou5t6HQTn7lEj3sIE mCohT3Z0WNQsTgxmw2N99X7SEJRyvN9fguC1OhxfkvlHYFjJjXzwM9jXhtK6whrSt5tV UkUUx/hfWk7eQ2xkHrUjNu9kmuy2FZVAv3/NSuPxWxIFrE15h4K8wJfFv4JfEYcLXbTM tmbDMWl5XH1a0EjTJm2UoViXsyvClYpXR1JYteT/SdoupsWzx10v6gRXxJBfzAx0Xswr 0GIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=iXE2vL9Wuef6hujk0SrXS1lmEY33LlAWY8v6gRULpKk=; b=aUt6LfWmFxct0jhKrrvQUR3NVSJtUDWPeO11xKSQubTi2WAKdvalYMUA8g7XU0j4En 9R9KMMcQGfjNKVjfyoOqR0A0NSwl2w66P/pRL8L/CCexCpbEa7289zGemuw4gbDm5N+v A40SLKulVEV40I/Vr55yhtEGOoa5faWrt8Zhg5UeWrFl/afw3DcOroKyuGotWYJ4GEU9 yiUuLUGTmJ+ihzIUkrpBgZBxxu978WF9TiE8QnX/nenMkFuE4eJA7pl6gAr4o68LMFcm 8MZclTCsDJM0vk1o583mixAE5vgGKcvliVIUeAtzrzS0cZqQnOZLNZlf17zNKRgLgnw4 OFzA== X-Gm-Message-State: ALoCoQm6j1+rt7tNtwZCzbpeLlncF2dF85g8bznFpsnWATxlXzw4+3aIiId2TMETe9ICD2+5x2r7XGh4cG/t0R6L2fvs9U2IZPQPqLsZ6gusD2seyS9JMlc= X-Received: by 10.194.158.135 with SMTP id wu7mr70334989wjb.142.1451470804616; Wed, 30 Dec 2015 02:20:04 -0800 (PST) MIME-Version: 1.0 Received: by 10.194.172.130 with HTTP; Wed, 30 Dec 2015 02:19:45 -0800 (PST) In-Reply-To: <20151230012436.GA8173@gondor.apana.org.au> References: <20151230012436.GA8173@gondor.apana.org.au> From: Dmitry Vyukov Date: Wed, 30 Dec 2015 11:19:45 +0100 Message-ID: Subject: Re: crypto: use-after-free in alg_bind To: Herbert Xu Cc: "David S. Miller" , linux-crypto@vger.kernel.org, LKML , Kostya Serebryany , Alexander Potapenko , Sasha Levin , Eric Dumazet , syzkaller Sender: linux-crypto-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-crypto@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Dec 30, 2015 at 2:24 AM, Herbert Xu 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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);