[PATCH] crypto: af_alg - Use bh_lock_sock in sk_destruct

From: Herbert Xu <herbert@gondor.apana.org.au>
To: Eric Dumazet <eric.dumazet@gmail.com>
Cc: syzbot <syzbot+c2f1558d49e25cc36e5e@syzkaller.appspotmail.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	syzkaller-bugs@googlegroups.com, viro@zeniv.linux.org.uk,
	"David S. Miller" <davem@davemloft.net>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>
Subject: [PATCH] crypto: af_alg - Use bh_lock_sock in sk_destruct
Date: Thu, 5 Dec 2019 13:45:05 +0800	[thread overview]
Message-ID: <20191205054505.wulhkajz64lwwffc@gondor.apana.org.au> (raw)
In-Reply-To: <f7009e8d-a488-c6df-6875-e872265efec0@gmail.com>

On Wed, Dec 04, 2019 at 08:59:11PM -0800, Eric Dumazet wrote:
>
> crypto layer (hash_sock_destruct()) is called from rcu callback (this in BH context) but tries to grab a socket lock.
> 
> A socket lock can schedule, which is illegal in BH context.

Fair enough.  Although I was rather intrigued as to how the RCU call
occured in the first place.  After some digging my theory is that
this is due to a SO_ATTACH_REUSEPORT_CBPF or SO_ATTACH_REUSEPORT_EBPF
setsockopt on the crypto socket.

What are these filters even suppposed to do on an af_alg socket?

Anyhow, this is a bug that could have been triggered even without
this, but it would have been almost impossible to do it through
syzbot as you need to have an outstanding async skcipher/aead request
that is freed in BH context.

---8<---
As af_alg_release_parent may be called from BH context (most notably
due to an async request that only completes after socket closure,
or as reported here because of an RCU-delayed sk_destruct call), we
must use bh_lock_sock instead of lock_sock.

Reported-by: syzbot+c2f1558d49e25cc36e5e@syzkaller.appspotmail.com
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: c840ac6af3f8 ("crypto: af_alg - Disallow bind/setkey/...")
Cc: <stable@vger.kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 0dceaabc6321..3d8e53010cda 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -134,11 +134,13 @@ void af_alg_release_parent(struct sock *sk)
 	sk = ask->parent;
 	ask = alg_sk(sk);
 
-	lock_sock(sk);
+	local_bh_disable();
+	bh_lock_sock(sk);
 	ask->nokey_refcnt -= nokey;
 	if (!last)
 		last = !--ask->refcnt;
-	release_sock(sk);
+	bh_unlock_sock(sk);
+	local_bh_enable();
 
 	if (last)
 		sock_put(sk);
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
