2021-10-21 18:31:37

by Daniel Jordan

Subject: [PATCH] crypto: pcrypt - Delay write to padata->info

These three events can race when pcrypt is used multiple times in a
template ("pcrypt(pcrypt(...))"):

1. [taskA] The caller makes the crypto request via crypto_aead_encrypt()
2. [kworkerB] padata serializes the inner pcrypt request
3. [kworkerC] padata serializes the outer pcrypt request

3 might finish before the call to crypto_aead_encrypt() returns in 1,
resulting in two possible issues.

First, a use-after-free of the crypto request's memory when, for
example, taskA writes to the outer pcrypt request's padata->info in
pcrypt_aead_enc() after kworkerC completes the request.

Second, the outer pcrypt request overwrites the inner pcrypt request's
return code with -EINPROGRESS, making a successful request appear to
fail. For instance, kworkerB writes the outer pcrypt request's
padata->info in pcrypt_aead_done() and then taskA overwrites it
in pcrypt_aead_enc().

Avoid both situations by delaying the write of padata->info until after
the inner crypto request's return code is checked. This prevents the
use-after-free by not touching the crypto request's memory after the
next-inner crypto request is made, and stops padata->info from being
overwritten.

Fixes: 5068c7a883d16 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper")
Reported-by: [email protected]
Signed-off-by: Daniel Jordan <[email protected]>
---

So, pcrypt(pcrypt(...)) doesn't make all that much sense, but apparently
there's not an easy way to prevent it [1], so I'm going with the minimal
fix.

[1] https://lkml.kernel.org/[email protected]/

crypto/pcrypt.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index d569c7ed6c80..9d10b846ccf7 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -78,12 +78,14 @@ static void pcrypt_aead_enc(struct padata_priv *padata)
{
struct pcrypt_request *preq = pcrypt_padata_request(padata);
struct aead_request *req = pcrypt_request_ctx(preq);
+ int ret;

- padata->info = crypto_aead_encrypt(req);
+ ret = crypto_aead_encrypt(req);

- if (padata->info == -EINPROGRESS)
+ if (ret == -EINPROGRESS)
return;

+ padata->info = ret;
padata_do_serial(padata);
}

@@ -123,12 +125,14 @@ static void pcrypt_aead_dec(struct padata_priv *padata)
{
struct pcrypt_request *preq = pcrypt_padata_request(padata);
struct aead_request *req = pcrypt_request_ctx(preq);
+ int ret;

- padata->info = crypto_aead_decrypt(req);
+ ret = crypto_aead_decrypt(req);

- if (padata->info == -EINPROGRESS)
+ if (ret == -EINPROGRESS)
return;

+ padata->info = ret;
padata_do_serial(padata);
}

--
2.27.0

2021-10-29 13:13:17

by Herbert Xu

Subject: Re: [PATCH] crypto: pcrypt - Delay write to padata->info

On Thu, Oct 21, 2021 at 02:30:28PM -0400, Daniel Jordan wrote:
> These three events can race when pcrypt is used multiple times in a
> template ("pcrypt(pcrypt(...))"):
>
> 1. [taskA] The caller makes the crypto request via crypto_aead_encrypt()
> 2. [kworkerB] padata serializes the inner pcrypt request
> 3. [kworkerC] padata serializes the outer pcrypt request
>
> 3 might finish before the call to crypto_aead_encrypt() returns in 1,
> resulting in two possible issues.
>
> First, a use-after-free of the crypto request's memory when, for
> example, taskA writes to the outer pcrypt request's padata->info in
> pcrypt_aead_enc() after kworkerC completes the request.
>
> Second, the outer pcrypt request overwrites the inner pcrypt request's
> return code with -EINPROGRESS, making a successful request appear to
> fail. For instance, kworkerB writes the outer pcrypt request's
> padata->info in pcrypt_aead_done() and then taskA overwrites it
> in pcrypt_aead_enc().
>
> Avoid both situations by delaying the write of padata->info until after
> the inner crypto request's return code is checked. This prevents the
> use-after-free by not touching the crypto request's memory after the
> next-inner crypto request is made, and stops padata->info from being
> overwritten.
>
> Fixes: 5068c7a883d16 ("crypto: pcrypt - Add pcrypt crypto parallelization wrapper")
> Reported-by: [email protected]
> Signed-off-by: Daniel Jordan <[email protected]>
> ---
>
> So, pcrypt(pcrypt(...)) doesn't make all that much sense, but apparently
> there's not an easy way to prevent it [1], so I'm going with the minimal
> fix.
>
> [1] https://lkml.kernel.org/[email protected]/
>
> crypto/pcrypt.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)

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