[RFC] crypto: pcrypt - forbid recursive instantiation
diff mbox

Message ID 20180310232231.19191-1-ebiggers3@gmail.com
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Eric Biggers March 10, 2018, 11:22 p.m. UTC
From: Eric Biggers <ebiggers@google.com>

If the pcrypt template is used multiple times in an algorithm, then a
deadlock occurs because all pcrypt instances share the same
padata_instance, which completes requests in the order submitted.  That
is, the inner pcrypt request waits for the outer pcrypt request while
the outer request is already waiting for the inner.

Fix this by making pcrypt forbid instantiation if pcrypt appears in the
underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
simple fix that should be sufficient to prevent the deadlock.

Reproducer:

	#include <linux/if_alg.h>
	#include <sys/socket.h>
	#include <unistd.h>

	int main()
	{
		struct sockaddr_alg addr = {
			.salg_type = "aead",
			.salg_name = "pcrypt(pcrypt(rfc4106-gcm-aesni))"
		};
		int algfd, reqfd;
		char buf[32] = { 0 };

		algfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
		bind(algfd, (void *)&addr, sizeof(addr));
		setsockopt(algfd, SOL_ALG, ALG_SET_KEY, buf, 20);
		reqfd = accept(algfd, 0, 0);
		write(reqfd, buf, 32);
		read(reqfd, buf, 16);
	}

Reported-by: syzbot+56c7151cad94eec37c521f0e47d2eee53f9361c4@syzkaller.appspotmail.com
Fixes: 5068c7a883d1 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper")
Cc: <stable@vger.kernel.org> # v2.6.34+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/pcrypt.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Steffen Klassert March 16, 2018, 7:10 a.m. UTC | #1
On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> If the pcrypt template is used multiple times in an algorithm, then a
> deadlock occurs because all pcrypt instances share the same
> padata_instance, which completes requests in the order submitted.  That
> is, the inner pcrypt request waits for the outer pcrypt request while
> the outer request is already waiting for the inner.
> 
> Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> simple fix that should be sufficient to prevent the deadlock.
> 
> Reproducer:
> 
> 	#include <linux/if_alg.h>
> 	#include <sys/socket.h>
> 	#include <unistd.h>
> 
> 	int main()
> 	{
> 		struct sockaddr_alg addr = {
> 			.salg_type = "aead",
> 			.salg_name = "pcrypt(pcrypt(rfc4106-gcm-aesni))"
> 		};
> 		int algfd, reqfd;
> 		char buf[32] = { 0 };
> 
> 		algfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
> 		bind(algfd, (void *)&addr, sizeof(addr));
> 		setsockopt(algfd, SOL_ALG, ALG_SET_KEY, buf, 20);
> 		reqfd = accept(algfd, 0, 0);
> 		write(reqfd, buf, 32);
> 		read(reqfd, buf, 16);
> 	}
> 
> Reported-by: syzbot+56c7151cad94eec37c521f0e47d2eee53f9361c4@syzkaller.appspotmail.com
> Fixes: 5068c7a883d1 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper")
> Cc: <stable@vger.kernel.org> # v2.6.34+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/pcrypt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
> index f8ec3d4ba4a80..3ec64604f6a56 100644
> --- a/crypto/pcrypt.c
> +++ b/crypto/pcrypt.c
> @@ -265,6 +265,12 @@ static void pcrypt_free(struct aead_instance *inst)
>  static int pcrypt_init_instance(struct crypto_instance *inst,
>  				struct crypto_alg *alg)
>  {
> +	/* Recursive pcrypt deadlocks due to the shared padata_instance */
> +	if (!strncmp(alg->cra_driver_name, "pcrypt(", 7) ||
> +	    strstr(alg->cra_driver_name, "(pcrypt(") ||
> +	    strstr(alg->cra_driver_name, ",pcrypt("))
> +		return -EINVAL;
> +

This looks ok to me.

Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
Herbert Xu March 23, 2018, 12:21 a.m. UTC | #2
On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> If the pcrypt template is used multiple times in an algorithm, then a
> deadlock occurs because all pcrypt instances share the same
> padata_instance, which completes requests in the order submitted.  That
> is, the inner pcrypt request waits for the outer pcrypt request while
> the outer request is already waiting for the inner.
> 
> Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> simple fix that should be sufficient to prevent the deadlock.

I'm a bit uneasy with this fix.  What if pcrypt is used in the
underlying algorithm as a fallback? Wouldn't it still dead-lock
without triggering this check?

Thanks,
Eric Biggers April 8, 2018, 10:55 p.m. UTC | #3
On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote:
> On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > If the pcrypt template is used multiple times in an algorithm, then a
> > deadlock occurs because all pcrypt instances share the same
> > padata_instance, which completes requests in the order submitted.  That
> > is, the inner pcrypt request waits for the outer pcrypt request while
> > the outer request is already waiting for the inner.
> > 
> > Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> > underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> > simple fix that should be sufficient to prevent the deadlock.
> 
> I'm a bit uneasy with this fix.  What if pcrypt is used in the
> underlying algorithm as a fallback? Wouldn't it still dead-lock
> without triggering this check?
> 

Yep, I think you're right.

Steffen, how feasible do you think would it be to make each pcrypt instance use
its own padata instance, so different pcrypt instances aren't ordered with
respect to each other?  Or do you think there is a better solution?  This really
needs to be fixed; syzbot has hit it over 11000 times already.

Eric
Steffen Klassert April 9, 2018, 8:58 a.m. UTC | #4
On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote:
> On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote:
> > On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > If the pcrypt template is used multiple times in an algorithm, then a
> > > deadlock occurs because all pcrypt instances share the same
> > > padata_instance, which completes requests in the order submitted.  That
> > > is, the inner pcrypt request waits for the outer pcrypt request while
> > > the outer request is already waiting for the inner.
> > > 
> > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> > > underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> > > simple fix that should be sufficient to prevent the deadlock.
> > 
> > I'm a bit uneasy with this fix.  What if pcrypt is used in the
> > underlying algorithm as a fallback? Wouldn't it still dead-lock
> > without triggering this check?
> > 
> 
> Yep, I think you're right.
> 
> Steffen, how feasible do you think would it be to make each pcrypt instance use
> its own padata instance, so different pcrypt instances aren't ordered with
> respect to each other?

It should be possible at least, but requires some work.
I already had patches to get multiple independent pcrypt
insatance to better support pcrypt for different workloads
and NUMA mchines. I never got finalized with is because
of the lack of NUMA hardware to test but the code is 
still availabe, maybe it can help:

https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt

> Or do you think there is a better solution?

Not really.

Btw. is there a valid usecase where a certain template
is used twice in one algorithm instance?
Eric Biggers April 9, 2018, 7:07 p.m. UTC | #5
On Mon, Apr 09, 2018 at 10:58:08AM +0200, Steffen Klassert wrote:
> On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote:
> > On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote:
> > > On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@google.com>
> > > > 
> > > > If the pcrypt template is used multiple times in an algorithm, then a
> > > > deadlock occurs because all pcrypt instances share the same
> > > > padata_instance, which completes requests in the order submitted.  That
> > > > is, the inner pcrypt request waits for the outer pcrypt request while
> > > > the outer request is already waiting for the inner.
> > > > 
> > > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> > > > underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> > > > simple fix that should be sufficient to prevent the deadlock.
> > > 
> > > I'm a bit uneasy with this fix.  What if pcrypt is used in the
> > > underlying algorithm as a fallback? Wouldn't it still dead-lock
> > > without triggering this check?
> > > 
> > 
> > Yep, I think you're right.
> > 
> > Steffen, how feasible do you think would it be to make each pcrypt instance use
> > its own padata instance, so different pcrypt instances aren't ordered with
> > respect to each other?
> 
> It should be possible at least, but requires some work.
> I already had patches to get multiple independent pcrypt
> insatance to better support pcrypt for different workloads
> and NUMA mchines. I never got finalized with is because
> of the lack of NUMA hardware to test but the code is 
> still availabe, maybe it can help:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt
> 
> > Or do you think there is a better solution?
> 
> Not really.
> 
> Btw. is there a valid usecase where a certain template
> is used twice in one algorithm instance?
> 

None that I can think of right now.  The problem is that it's hard to prevent
template recursion when users can specify arbitrary algorithms using AF_ALG, and
some algorithms even use fallbacks which may use templates not explicitly listed
in the driver name.

Remember, a kernel deadlock is a bug even if it's "not a valid use case"...
In this case it can even be triggered by an unprivileged user via AF_ALG.

Eric
Steffen Klassert April 18, 2018, 5:35 a.m. UTC | #6
On Mon, Apr 09, 2018 at 12:07:39PM -0700, Eric Biggers wrote:
> On Mon, Apr 09, 2018 at 10:58:08AM +0200, Steffen Klassert wrote:
> > On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote:
> > > 
> > > Steffen, how feasible do you think would it be to make each pcrypt instance use
> > > its own padata instance, so different pcrypt instances aren't ordered with
> > > respect to each other?
> > 
> > It should be possible at least, but requires some work.
> > I already had patches to get multiple independent pcrypt
> > insatance to better support pcrypt for different workloads
> > and NUMA mchines. I never got finalized with is because
> > of the lack of NUMA hardware to test but the code is 
> > still availabe, maybe it can help:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt
> > 
> > > Or do you think there is a better solution?
> > 
> > Not really.
> > 
> > Btw. is there a valid usecase where a certain template
> > is used twice in one algorithm instance?
> > 
> 
> None that I can think of right now.  The problem is that it's hard to prevent
> template recursion when users can specify arbitrary algorithms using AF_ALG, and
> some algorithms even use fallbacks which may use templates not explicitly listed
> in the driver name.
> 
> Remember, a kernel deadlock is a bug even if it's "not a valid use case"...
> In this case it can even be triggered by an unprivileged user via AF_ALG.

Yes sure, I just wanted to know if it is worth to think about
preventing template recursions. If there is a valid usecase,
then we don't even need to think in this direction.

While I think each pcrypt instance should have it's own
padata instance on the long run, it would be good to have
a not so intrusive fix that can be backported to the stable
trees.

Patch
diff mbox

diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index f8ec3d4ba4a80..3ec64604f6a56 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -265,6 +265,12 @@  static void pcrypt_free(struct aead_instance *inst)
 static int pcrypt_init_instance(struct crypto_instance *inst,
 				struct crypto_alg *alg)
 {
+	/* Recursive pcrypt deadlocks due to the shared padata_instance */
+	if (!strncmp(alg->cra_driver_name, "pcrypt(", 7) ||
+	    strstr(alg->cra_driver_name, "(pcrypt(") ||
+	    strstr(alg->cra_driver_name, ",pcrypt("))
+		return -EINVAL;
+
 	if (snprintf(inst->alg.cra_driver_name, CRYPTO_MAX_ALG_NAME,
 		     "pcrypt(%s)", alg->cra_driver_name) >= CRYPTO_MAX_ALG_NAME)
 		return -ENAMETOOLONG;