2019-09-20 14:37:48

by Steffen Klassert

Subject: [PATCH] crypto: pcrypt - forbid recursive instantiation

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 or if an underlying algorithm needs a
fallback. 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: <[email protected]> # v2.6.34+
Signed-off-by: Eric Biggers <[email protected]>
[SK: also require that the underlying algorithm doesn't need a fallback]
Signed-off-by: Steffen Klassert <[email protected]>
---
crypto/pcrypt.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 543792e0ebf0..932a77b61b47 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -198,6 +198,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;
@@ -236,7 +242,7 @@ static int pcrypt_create_aead(struct crypto_template *tmpl, struct rtattr **tb,
ctx = aead_instance_ctx(inst);
crypto_set_aead_spawn(&ctx->spawn, aead_crypto_instance(inst));

- err = crypto_grab_aead(&ctx->spawn, name, 0, 0);
+ err = crypto_grab_aead(&ctx->spawn, name, 0, CRYPTO_ALG_NEED_FALLBACK);
if (err)
goto out_free_inst;

--
2.17.1

2019-11-19 12:54:16

by Herbert Xu

Subject: Re: [PATCH] crypto: pcrypt - forbid recursive instantiation

On Fri, Sep 20, 2019 at 06:35:56AM +0200, Steffen Klassert wrote:
>
> Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> underlying ->cra_driver_name or if an underlying algorithm needs a
> fallback. This is somewhat of a hack, but it's a simple fix that should
> be sufficient to prevent the deadlock.

This still doesn't resolve the case where pcrypt is used in a
non-transparent fashion, e.g., through a fallback. Note that
even adding a NEED_FALLBACK flag won't fix this as you can construct
an instance on top of a NEED_FALLBACK algorithm that itself does
not need a fallback.

Anyway I decided to change pcrypt to use a set of queues per
instance rather than globally and it should resolve the problem.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt