From patchwork Tue Jun 30 23:09:42 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauricio Faria de Oliveira X-Patchwork-Id: 1320151 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 49xKnf4db0z9sPF; Wed, 1 Jul 2020 09:09:58 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1jqPNx-00061c-QP; Tue, 30 Jun 2020 23:09:53 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jqPNt-00060o-Ph for kernel-team@lists.ubuntu.com; Tue, 30 Jun 2020 23:09:49 +0000 Received: from mail-qt1-f198.google.com ([209.85.160.198]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jqPNt-0004eA-F1 for kernel-team@lists.ubuntu.com; Tue, 30 Jun 2020 23:09:49 +0000 Received: by mail-qt1-f198.google.com with SMTP id i5so15593493qtw.3 for ; Tue, 30 Jun 2020 16:09:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=WybuCiM5GpuHK+4Flnx4eKFlMZABNDaXNEmu3C4TUIo=; b=QtXT+4rL4BHsBsPkSDsmDADkjr2ZpH9b5vK0Yn29PwWjnirfd02ySa1aD5Rcv502s9 GISbe2yTF/uxZTd5XJfsaa8RAA8/Ox3i11lCtV4unBwwrvRVbYsnQxT9DeQ01Tk92ou0 2uYg5pCEsh1ENdh/9z+UuljaHpSkNiMqdISTp6opNSksCU0NLVw70bXe9ON8RRAn/zW5 BjvCP+smMjvdOnzZ2KsWnWGgQxNe1gpId0gSoW4ScOWi8XH5WEFsjtCac/UN034815N5 +DyPwYz0tNaFN5DfWpZxLfbo4FHZyryxdQygFFbqSHxFUHfuU3RDiNtartbHdoiYv5pu s/jQ== X-Gm-Message-State: AOAM531LZOiMYd/57/BzcLQRYHFaPNDt9ajjRQQ7zurDX1RYiW1hXa9f 2jdggxPrVEforVZ0udBzR5UpuFgOb8tbUUONrzwL6jm2LUWGdaFwk5Oql1cfcZThcfvRD4oIPJU 3cfGG4thEtL+zIWAI/d5UQLnl194ol09a80kBGH55tA== X-Received: by 2002:a0c:f88e:: with SMTP id u14mr22870733qvn.81.1593558588393; Tue, 30 Jun 2020 16:09:48 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy/+tOEBRQ4kk3xS7H/+XKCSx+HhkxvUAO8rIjzFsxUQxjJRU3TvgQHauu5KIB8v7yQyMzc5Q== X-Received: by 2002:a0c:f88e:: with SMTP id u14mr22870696qvn.81.1593558588016; Tue, 30 Jun 2020 16:09:48 -0700 (PDT) Received: from localhost.localdomain ([201.82.49.101]) by smtp.gmail.com with ESMTPSA id i28sm4374399qkh.1.2020.06.30.16.09.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jun 2020 16:09:47 -0700 (PDT) From: Mauricio Faria de Oliveira To: kernel-team@lists.ubuntu.com Subject: [E/F/Unstable][PATCH 1/1] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock() Date: Tue, 30 Jun 2020 20:09:42 -0300 Message-Id: <20200630230942.1576682-2-mfo@canonical.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200630230942.1576682-1-mfo@canonical.com> References: <20200630230942.1576682-1-mfo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Herbert Xu BugLink: https://bugs.launchpad.net/bugs/1884766 The locking in af_alg_release_parent is broken as the BH socket lock can only be taken if there is a code-path to handle the case where the lock is owned by process-context. Instead of adding such handling, we can fix this by changing the ref counts to atomic_t. This patch also modifies the main refcnt to include both normal and nokey sockets. This way we don't have to fudge the nokey ref count when a socket changes from nokey to normal. Credits go to Mauricio Faria de Oliveira who diagnosed this bug and sent a patch for it: https://lore.kernel.org/linux-crypto/20200605161657.535043-1-mfo@canonical.com/ Reported-by: Brian Moyles Reported-by: Mauricio Faria de Oliveira Fixes: 37f96694cf73 ("crypto: af_alg - Use bh_lock_sock in...") Cc: Signed-off-by: Herbert Xu (cherry picked from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d) Signed-off-by: Mauricio Faria de Oliveira Acked-by: Stefan Bader Acked-by: Andrea Righi --- crypto/af_alg.c | 26 +++++++++++--------------- crypto/algif_aead.c | 9 +++------ crypto/algif_hash.c | 9 +++------ crypto/algif_skcipher.c | 9 +++------ include/crypto/if_alg.h | 4 ++-- 5 files changed, 22 insertions(+), 35 deletions(-) diff --git a/crypto/af_alg.c b/crypto/af_alg.c index b1cd3535c525..28fc323e3fe3 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -128,21 +128,15 @@ EXPORT_SYMBOL_GPL(af_alg_release); void af_alg_release_parent(struct sock *sk) { struct alg_sock *ask = alg_sk(sk); - unsigned int nokey = ask->nokey_refcnt; - bool last = nokey && !ask->refcnt; + unsigned int nokey = atomic_read(&ask->nokey_refcnt); sk = ask->parent; ask = alg_sk(sk); - local_bh_disable(); - bh_lock_sock(sk); - ask->nokey_refcnt -= nokey; - if (!last) - last = !--ask->refcnt; - bh_unlock_sock(sk); - local_bh_enable(); + if (nokey) + atomic_dec(&ask->nokey_refcnt); - if (last) + if (atomic_dec_and_test(&ask->refcnt)) sock_put(sk); } EXPORT_SYMBOL_GPL(af_alg_release_parent); @@ -187,7 +181,7 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) err = -EBUSY; lock_sock(sk); - if (ask->refcnt | ask->nokey_refcnt) + if (atomic_read(&ask->refcnt)) goto unlock; swap(ask->type, type); @@ -236,7 +230,7 @@ static int alg_setsockopt(struct socket *sock, int level, int optname, int err = -EBUSY; lock_sock(sk); - if (ask->refcnt) + if (atomic_read(&ask->refcnt) != atomic_read(&ask->nokey_refcnt)) goto unlock; type = ask->type; @@ -301,12 +295,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern) if (err) goto unlock; - if (nokey || !ask->refcnt++) + if (atomic_inc_return_relaxed(&ask->refcnt) == 1) sock_hold(sk); - ask->nokey_refcnt += nokey; + if (nokey) { + atomic_inc(&ask->nokey_refcnt); + atomic_set(&alg_sk(sk2)->nokey_refcnt, 1); + } alg_sk(sk2)->parent = sk; alg_sk(sk2)->type = type; - alg_sk(sk2)->nokey_refcnt = nokey; newsock->ops = type->ops; newsock->state = SS_CONNECTED; diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index eb1910b6d434..0ae000a61c7f 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -384,7 +384,7 @@ static int aead_check_key(struct socket *sock) struct alg_sock *ask = alg_sk(sk); lock_sock(sk); - if (ask->refcnt) + if (!atomic_read(&ask->nokey_refcnt)) goto unlock_child; psk = ask->parent; @@ -396,11 +396,8 @@ static int aead_check_key(struct socket *sock) if (crypto_aead_get_flags(tfm->aead) & CRYPTO_TFM_NEED_KEY) goto unlock; - if (!pask->refcnt++) - sock_hold(psk); - - ask->refcnt = 1; - sock_put(psk); + atomic_dec(&pask->nokey_refcnt); + atomic_set(&ask->nokey_refcnt, 0); err = 0; diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c index da1ffa4f7f8d..e71727c25a7d 100644 --- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -301,7 +301,7 @@ static int hash_check_key(struct socket *sock) struct alg_sock *ask = alg_sk(sk); lock_sock(sk); - if (ask->refcnt) + if (!atomic_read(&ask->nokey_refcnt)) goto unlock_child; psk = ask->parent; @@ -313,11 +313,8 @@ static int hash_check_key(struct socket *sock) if (crypto_ahash_get_flags(tfm) & CRYPTO_TFM_NEED_KEY) goto unlock; - if (!pask->refcnt++) - sock_hold(psk); - - ask->refcnt = 1; - sock_put(psk); + atomic_dec(&pask->nokey_refcnt); + atomic_set(&ask->nokey_refcnt, 0); err = 0; diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c index 4c3bdffe0c3a..ec5567c87a6d 100644 --- a/crypto/algif_skcipher.c +++ b/crypto/algif_skcipher.c @@ -211,7 +211,7 @@ static int skcipher_check_key(struct socket *sock) struct alg_sock *ask = alg_sk(sk); lock_sock(sk); - if (ask->refcnt) + if (!atomic_read(&ask->nokey_refcnt)) goto unlock_child; psk = ask->parent; @@ -223,11 +223,8 @@ static int skcipher_check_key(struct socket *sock) if (crypto_skcipher_get_flags(tfm) & CRYPTO_TFM_NEED_KEY) goto unlock; - if (!pask->refcnt++) - sock_hold(psk); - - ask->refcnt = 1; - sock_put(psk); + atomic_dec(&pask->nokey_refcnt); + atomic_set(&ask->nokey_refcnt, 0); err = 0; diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h index 56527c85d122..088c1ded2714 100644 --- a/include/crypto/if_alg.h +++ b/include/crypto/if_alg.h @@ -29,8 +29,8 @@ struct alg_sock { struct sock *parent; - unsigned int refcnt; - unsigned int nokey_refcnt; + atomic_t refcnt; + atomic_t nokey_refcnt; const struct af_alg_type *type; void *private;