From patchwork Tue Jun 30 23:10:00 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: 1320152 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 49xKnx295Xz9sPF; Wed, 1 Jul 2020 09:10:13 +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 1jqPOD-00067B-1O; Tue, 30 Jun 2020 23:10:09 +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 1jqPOA-00066V-Au for kernel-team@lists.ubuntu.com; Tue, 30 Jun 2020 23:10:06 +0000 Received: from mail-qk1-f198.google.com ([209.85.222.198]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jqPO9-0004iz-VA for kernel-team@lists.ubuntu.com; Tue, 30 Jun 2020 23:10:06 +0000 Received: by mail-qk1-f198.google.com with SMTP id z1so5650885qkz.3 for ; Tue, 30 Jun 2020 16:10:05 -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:mime-version :content-transfer-encoding; bh=gdA0neEtJWnfXwXkTij2xX2LOTCJQgAMLbdbNzRYwo8=; b=HLgN0PSwc7XtvT0gM00f6D0PVyucAQQe/X6/k1VskKB9qxsaNvozcEDSixuyP/cM/f upfklP+a/8U7kuMfiFeU0lkl5/A4LaBLcjRoSdJGdEf61RKco/xONEbMwvm1SID60bmm QVlTpKyl/zd0F7zbj35G/BysKLKzcaW37jwM+zRXw0tu8bWXi6Hl6+6NplOyTViAF6+F 6dYUNldoJeGABiHvW1Fnyfq45y7yigu9X/4Vgzx+d/Pcj552YVNtRDWebn0YKzvCY4SD c9WdTa620hVOoluvQGFakV6m1+rVvDxQ3Gm9MmSU17VOGvObfzF475Qnlw1qR/UonT00 +5tw== X-Gm-Message-State: AOAM531vJWpdoI4KRIkiKi8C0CtOazBgdkp7wyojRTMi6aUjqf0p/IsX IQSuOeNxNBLL0NqoOw0TCHK4qA8tz7RzWJwRpYNaeavCIWLOLhq2xzNHo3aEXzMYvjPXRF5NvvX VgUTw+0uaDqU2jq20Dnvd4xg9UHzJDu7WANZQG0CYQg== X-Received: by 2002:a37:a950:: with SMTP id s77mr22125832qke.171.1593558604731; Tue, 30 Jun 2020 16:10:04 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwfOGgf2UQGJdFJOH37E5MbtqlN7d2KGmg+j5Y891OuRIuKcJ4gBIwe3wMaFW9rHHn93FU3dA== X-Received: by 2002:a37:a950:: with SMTP id s77mr22125807qke.171.1593558604433; Tue, 30 Jun 2020 16:10:04 -0700 (PDT) Received: from localhost.localdomain ([201.82.49.101]) by smtp.gmail.com with ESMTPSA id b10sm3900128qkh.124.2020.06.30.16.10.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jun 2020 16:10:03 -0700 (PDT) From: Mauricio Faria de Oliveira To: kernel-team@lists.ubuntu.com Subject: [D][PATCH] crypto: af_alg - fix use-after-free in af_alg_accept() due to bh_lock_sock() Date: Tue, 30 Jun 2020 20:10:00 -0300 Message-Id: <20200630231000.1576771-1-mfo@canonical.com> X-Mailer: git-send-email 2.25.1 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 (backported from commit 34c86f4c4a7be3b3e35aa48bd18299d4c756064d) [mfo: backport: refresh context lines in af_alg.c hunk 4] 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 4fc8e6a7abb2..272879d7b0d1 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -133,21 +133,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); @@ -192,7 +186,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); @@ -241,7 +235,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; @@ -308,12 +302,14 @@ int af_alg_accept(struct sock *sk, struct socket *newsock, bool kern) sk2->sk_family = PF_ALG; - 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 eb100a04ce9f..e643d268c7ba 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -388,7 +388,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; @@ -400,11 +400,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 d0cde541beb6..73abe6a33f3a 100644 --- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -306,7 +306,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; @@ -318,11 +318,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 cfdaab2b7d76..75444b8beed9 100644 --- a/crypto/algif_skcipher.c +++ b/crypto/algif_skcipher.c @@ -219,7 +219,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; @@ -231,11 +231,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 482461d8931d..11f107df78dc 100644 --- a/include/crypto/if_alg.h +++ b/include/crypto/if_alg.h @@ -34,8 +34,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;