[PATCH 0/4] crypto: async crypto op fixes

[PATCH 0/4] crypto: async crypto op fixes

[Gilad Ben-Yossef]

This patch set fixes various usage and documentation errors
in waiting for async crypto op to complete which can result
in data corruption.

Note: these were discovered in the process of working on a
patch set that replaces these call sites and more with a
generic implementation that will prevent these problems
going forward. These are just the fix ups for current code.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
CC: stable@vger.kernel.org
CC: Eric Biggers <ebiggers3@gmail.com>

Gilad Ben-Yossef (4):
  crypto: handle EBUSY due to backlog correctly
  crypto: drbg wait for crypto op not signal safe
  crypto: gcm wait for crypto op not signal safe
  crypto: Documentation: fix none signal safe sample

 Documentation/crypto/api-samples.rst | 2 +-
 crypto/asymmetric_keys/public_key.c  | 2 +-
 crypto/drbg.c                        | 5 ++---
 crypto/gcm.c                         | 6 ++----
 4 files changed, 6 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH 1/4] crypto: handle EBUSY due to backlog correctly

[Gilad Ben-Yossef]

public_key_verify_signature() was passing the CRYPTO_TFM_REQ_MAY_BACKLOG
flag to akcipher_request_set_callback() but was not handling correctly
the case where a -EBUSY error could be returned from the call to
crypto_akcipher_verify() if backlog was used, possibly casuing
data corruption due to use-after-free of buffers.

Resolve this by handling -EBUSY correctly.

Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
CC: stable@vger.kernel.org
---
 crypto/asymmetric_keys/public_key.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index d3a989e..3cd6e12 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -141,7 +141,7 @@ int public_key_verify_signature(const struct public_key *pkey,
 	 * signature and returns that to us.
 	 */
 	ret = crypto_akcipher_verify(req);
-	if (ret == -EINPROGRESS) {
+	if ((ret == -EINPROGRESS) || (ret == -EBUSY)) {
 		wait_for_completion(&compl.completion);
 		ret = compl.err;
 	}
-- 
2.1.4

[PATCH 2/4] crypto: drbg wait for crypto op not signal safe

[Gilad Ben-Yossef]

drbg_kcapi_sym_ctr() was using wait_for_completion_interruptible() to
wait for completion of async crypto op but if a signal occurs it
may return before DMA ops of HW crypto provider finish, thus
corrupting the output buffer.

Resolve this by using wait_for_completion() instead.

Reported-by: Eric Biggers <ebiggers3@gmail.com>
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
CC: stable@vger.kernel.org
---
 crypto/drbg.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index fa749f4..fa9054d 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1767,8 +1767,7 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 			break;
 		case -EINPROGRESS:
 		case -EBUSY:
-			ret = wait_for_completion_interruptible(
-				&drbg->ctr_completion);
+			ret = wait_for_completion(&drbg->ctr_completion);
 			if (!ret && !drbg->ctr_async_err) {
 				reinit_completion(&drbg->ctr_completion);
 				break;
-- 
2.1.4

Re: [PATCH 2/4] crypto: drbg wait for crypto op not signal safe

[Eric Biggers]

Hi Gilad,

On Thu, May 11, 2017 at 02:53:43PM +0300, Gilad Ben-Yossef wrote:
> drbg_kcapi_sym_ctr() was using wait_for_completion_interruptible() to
> wait for completion of async crypto op but if a signal occurs it
> may return before DMA ops of HW crypto provider finish, thus
> corrupting the output buffer.
> 
> Resolve this by using wait_for_completion() instead.
> 
> Reported-by: Eric Biggers <ebiggers3@gmail.com>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> CC: stable@vger.kernel.org
> ---
>  crypto/drbg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index fa749f4..fa9054d 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1767,8 +1767,7 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
>  			break;
>  		case -EINPROGRESS:
>  		case -EBUSY:
> -			ret = wait_for_completion_interruptible(
> -				&drbg->ctr_completion);
> +			ret = wait_for_completion(&drbg->ctr_completion);
>  			if (!ret && !drbg->ctr_async_err) {
>  				reinit_completion(&drbg->ctr_completion);
>  				break;
> -- 

wait_for_completion() doesn't return a value.  This was fixed in the next patch,
but it should be done in this patch.

Eric

Re: [PATCH 2/4] crypto: drbg wait for crypto op not signal safe

[Herbert Xu]

On Thu, May 11, 2017 at 02:53:43PM +0300, Gilad Ben-Yossef wrote:
> drbg_kcapi_sym_ctr() was using wait_for_completion_interruptible() to
> wait for completion of async crypto op but if a signal occurs it
> may return before DMA ops of HW crypto provider finish, thus
> corrupting the output buffer.
> 
> Resolve this by using wait_for_completion() instead.
> 
> Reported-by: Eric Biggers <ebiggers3@gmail.com>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> CC: stable@vger.kernel.org

This patch doesn't even compile.  Please test your work first.

Thanks,
-- 
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

Re: [PATCH 2/4] crypto: drbg wait for crypto op not signal safe

[Gilad Ben-Yossef]

On Thu, May 18, 2017 at 8:09 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, May 11, 2017 at 02:53:43PM +0300, Gilad Ben-Yossef wrote:
>> drbg_kcapi_sym_ctr() was using wait_for_completion_interruptible() to
>> wait for completion of async crypto op but if a signal occurs it
>> may return before DMA ops of HW crypto provider finish, thus
>> corrupting the output buffer.
>>
>> Resolve this by using wait_for_completion() instead.
>>
>> Reported-by: Eric Biggers <ebiggers3@gmail.com>
>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>> CC: stable@vger.kernel.org
>
> This patch doesn't even compile.  Please test your work first.

Sigh... I've noticed it, fixed it, compiled it and than went ahead and
squashed the fix with the next patch in series instead of this one
like an idiot.

Please accept my apologies for wasting your time. I'll send a fixed version.

Gilad

>
> Thanks,
> --
> 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



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

[PATCH 3/4] crypto: gcm wait for crypto op not signal safe

[Gilad Ben-Yossef]

crypto_gcm_setkey() was using wait_for_completion_interruptible() to
wait for completion of async crypto op but if a signal occurs it
may return before DMA ops of HW crypto provider finish, thus
corrupting the data buffer that is kfree'ed in this case.

Resolve this by using wait_for_completion() instead.

Reported-by: Eric Biggers <ebiggers3@gmail.com>
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
CC: stable@vger.kernel.org
---
 crypto/drbg.c | 4 ++--
 crypto/gcm.c  | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/crypto/drbg.c b/crypto/drbg.c
index fa9054d..cdb27ac 100644
--- a/crypto/drbg.c
+++ b/crypto/drbg.c
@@ -1767,8 +1767,8 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
 			break;
 		case -EINPROGRESS:
 		case -EBUSY:
-			ret = wait_for_completion(&drbg->ctr_completion);
-			if (!ret && !drbg->ctr_async_err) {
+			wait_for_completion(&drbg->ctr_completion);
+			if (!drbg->ctr_async_err) {
 				reinit_completion(&drbg->ctr_completion);
 				break;
 			}
diff --git a/crypto/gcm.c b/crypto/gcm.c
index b7ad808..3841b5e 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -152,10 +152,8 @@ static int crypto_gcm_setkey(struct crypto_aead *aead, const u8 *key,
 
 	err = crypto_skcipher_encrypt(&data->req);
 	if (err == -EINPROGRESS || err == -EBUSY) {
-		err = wait_for_completion_interruptible(
-			&data->result.completion);
-		if (!err)
-			err = data->result.err;
+		wait_for_completion(&data->result.completion);
+		err = data->result.err;
 	}
 
 	if (err)
-- 
2.1.4

[PATCH 4/4] crypto: Documentation: fix none signal safe sample

[Gilad Ben-Yossef]

The sample code was showing use of wait_for_completion_interruptible()
for waiting for an async. crypto op to finish. However, if a signal
arrived it would free the buffers used even while crypto HW might
still DMA from/into the buffers.

Resolve this by using wait_for_completion() instead.

Reported-by: Eric Biggers <ebiggers3@gmail.com>
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
 Documentation/crypto/api-samples.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/crypto/api-samples.rst b/Documentation/crypto/api-samples.rst
index d021fd9..944f08b 100644
--- a/Documentation/crypto/api-samples.rst
+++ b/Documentation/crypto/api-samples.rst
@@ -48,7 +48,7 @@ Code Example For Symmetric Key Cipher Operation
             break;
         case -EINPROGRESS:
         case -EBUSY:
-            rc = wait_for_completion_interruptible(
+            rc = wait_for_completion(
                 &sk->result.completion);
             if (!rc && !sk->result.err) {
                 reinit_completion(&sk->result.completion);
-- 
2.1.4

Re: [PATCH 4/4] crypto: Documentation: fix none signal safe sample

[Eric Biggers]

On Thu, May 11, 2017 at 02:53:45PM +0300, Gilad Ben-Yossef wrote:
> The sample code was showing use of wait_for_completion_interruptible()
> for waiting for an async. crypto op to finish. However, if a signal
> arrived it would free the buffers used even while crypto HW might
> still DMA from/into the buffers.
> 
> Resolve this by using wait_for_completion() instead.
> 
> Reported-by: Eric Biggers <ebiggers3@gmail.com>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> ---
>  Documentation/crypto/api-samples.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/crypto/api-samples.rst b/Documentation/crypto/api-samples.rst
> index d021fd9..944f08b 100644
> --- a/Documentation/crypto/api-samples.rst
> +++ b/Documentation/crypto/api-samples.rst
> @@ -48,7 +48,7 @@ Code Example For Symmetric Key Cipher Operation
>              break;
>          case -EINPROGRESS:
>          case -EBUSY:
> -            rc = wait_for_completion_interruptible(
> +            rc = wait_for_completion(
>                  &sk->result.completion);
>              if (!rc && !sk->result.err) {
>                  reinit_completion(&sk->result.completion);
> -- 
> 2.1.4
> 

Same issue here: wait_for_completion() doesn't return a value.

Eric