[PATCH 0/3] crypto: arm64/aes-ccm - various bug fixes

[PATCH 0/3] crypto: arm64/aes-ccm - various bug fixes

[Ard Biesheuvel]

Fix a couple of bugs found by Eric's new testing code, and another
issue found by inspection.

Ard Biesheuvel (3):
  crypto: arm64/aes-ccm - fix logical bug in AAD MAC handling
  crypto: arm64/aes-ccm - fix bugs in non-NEON fallback routine
  crypto: arm64/aes-ccm - don't use an atomic walk needlessly

 arch/arm64/crypto/aes-ce-ccm-core.S | 5 +++--
 arch/arm64/crypto/aes-ce-ccm-glue.c | 8 +++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH 1/3] crypto: arm64/aes-ccm - fix logical bug in AAD MAC handling

[Ard Biesheuvel]

The NEON MAC calculation routine fails to handle the case correctly
where there is some data in the buffer, and the input fills it up
exactly. In this case, we enter the loop at the end with w8 == 0,
while a negative value is assumed, and so the loop carries on until
the increment of the 32-bit counter wraps around, which is quite
obviously wrong.

So omit the loop altogether in this case, and exit right away.

Reported-by: Eric Biggers <ebiggers@kernel.org>
Fixes: a3fd82105b9d1 ("arm64/crypto: AES in CCM mode using ARMv8 Crypto ...")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/crypto/aes-ce-ccm-core.S | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/crypto/aes-ce-ccm-core.S b/arch/arm64/crypto/aes-ce-ccm-core.S
index e3a375c4cb83..1b151442dac1 100644
--- a/arch/arm64/crypto/aes-ce-ccm-core.S
+++ b/arch/arm64/crypto/aes-ce-ccm-core.S
@@ -74,12 +74,13 @@ ENTRY(ce_aes_ccm_auth_data)
 	beq	10f
 	ext	v0.16b, v0.16b, v0.16b, #1	/* rotate out the mac bytes */
 	b	7b
-8:	mov	w7, w8
+8:	cbz	w8, 91f
+	mov	w7, w8
 	add	w8, w8, #16
 9:	ext	v1.16b, v1.16b, v1.16b, #1
 	adds	w7, w7, #1
 	bne	9b
-	eor	v0.16b, v0.16b, v1.16b
+91:	eor	v0.16b, v0.16b, v1.16b
 	st1	{v0.16b}, [x0]
 10:	str	w8, [x3]
 	ret
-- 
2.20.1

[PATCH 2/3] crypto: arm64/aes-ccm - fix bugs in non-NEON fallback routine

[Ard Biesheuvel]

Commit 5092fcf34908 ("crypto: arm64/aes-ce-ccm: add non-SIMD generic
fallback") introduced C fallback code to replace the NEON routines
when invoked from a context where the NEON is not available (i.e.,
from the context of a softirq taken while the NEON is already being
used in kernel process context)

Fix two logical flaws in the MAC calculation of the associated data.

Reported-by: Eric Biggers <ebiggers@kernel.org>
Fixes: 5092fcf34908 ("crypto: arm64/aes-ce-ccm: add non-SIMD generic fallback")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/crypto/aes-ce-ccm-glue.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index 68b11aa690e4..986191e8c058 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
@@ -125,7 +125,7 @@ static void ccm_update_mac(struct crypto_aes_ctx *key, u8 mac[], u8 const in[],
 			abytes -= added;
 		}
 
-		while (abytes > AES_BLOCK_SIZE) {
+		while (abytes >= AES_BLOCK_SIZE) {
 			__aes_arm64_encrypt(key->key_enc, mac, mac,
 					    num_rounds(key));
 			crypto_xor(mac, in, AES_BLOCK_SIZE);
@@ -139,8 +139,6 @@ static void ccm_update_mac(struct crypto_aes_ctx *key, u8 mac[], u8 const in[],
 					    num_rounds(key));
 			crypto_xor(mac, in, abytes);
 			*macp = abytes;
-		} else {
-			*macp = 0;
 		}
 	}
 }
-- 
2.20.1

[PATCH 3/3] crypto: arm64/aes-ccm - don't use an atomic walk needlessly

[Ard Biesheuvel]

When the AES-CCM code was first added, the NEON register were saved
and restored eagerly, and so the code avoided doing so, and executed
the scatterwalk in atomic context inside the kernel_neon_begin/end
section.

This has been changed in the meantime, so switch to non-atomic
scatterwalks.

Fixes: bd2ad885e30d ("crypto: arm64/aes-ce-ccm - move kernel mode neon ...")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/crypto/aes-ce-ccm-glue.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index 986191e8c058..5fc6f51908fd 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
@@ -253,7 +253,7 @@ static int ccm_encrypt(struct aead_request *req)
 	/* preserve the original iv for the final round */
 	memcpy(buf, req->iv, AES_BLOCK_SIZE);
 
-	err = skcipher_walk_aead_encrypt(&walk, req, true);
+	err = skcipher_walk_aead_encrypt(&walk, req, false);
 
 	if (may_use_simd()) {
 		while (walk.nbytes) {
@@ -311,7 +311,7 @@ static int ccm_decrypt(struct aead_request *req)
 	/* preserve the original iv for the final round */
 	memcpy(buf, req->iv, AES_BLOCK_SIZE);
 
-	err = skcipher_walk_aead_decrypt(&walk, req, true);
+	err = skcipher_walk_aead_decrypt(&walk, req, false);
 
 	if (may_use_simd()) {
 		while (walk.nbytes) {
-- 
2.20.1

Re: [PATCH 0/3] crypto: arm64/aes-ccm - various bug fixes

[Eric Biggers]

On Thu, Jan 24, 2019 at 05:33:44PM +0100, Ard Biesheuvel wrote:
> Fix a couple of bugs found by Eric's new testing code, and another
> issue found by inspection.
> 
> Ard Biesheuvel (3):
>   crypto: arm64/aes-ccm - fix logical bug in AAD MAC handling
>   crypto: arm64/aes-ccm - fix bugs in non-NEON fallback routine
>   crypto: arm64/aes-ccm - don't use an atomic walk needlessly
> 
>  arch/arm64/crypto/aes-ce-ccm-core.S | 5 +++--
>  arch/arm64/crypto/aes-ce-ccm-glue.c | 8 +++-----
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 

Thanks Ard.  For all three patches:

Reviewed-by: Eric Biggers <ebiggers@kernel.org>

However, can you add Cc stable to the first two?

BTW: I notice the !may_use_simd() case is still not covered by the self-tests.
Do you happen to have any suggestion for how to do it?  If there is a way to
make may_use_simd() temporarily return false, I can easily put a flag in 'struct
testvec_config' that makes the self-tests sometimes use that context to execute
the actual encryption/decryption/hashing.

It looks like on arm64, wrapping the operation in local_irq_{disable,enable}()
or kernel_fpu_{begin,end}() would be sufficient?  But x86 it doesn't look as
easy as x86 needs a context where 'in_interrupt() && !interrupted_user_mode() &&
!interrupted_kernel_fpu_idle()'.  Likewise arm32 requires 'in_interrupt()'.

It would be worth doing just for arm64, but ideally it would work on most/all
architectures.

- Eric

Re: [PATCH 0/3] crypto: arm64/aes-ccm - various bug fixes

[Herbert Xu]

On Thu, Jan 24, 2019 at 10:42:18PM -0800, Eric Biggers wrote:
> 
> Reviewed-by: Eric Biggers <ebiggers@kernel.org>
> 
> However, can you add Cc stable to the first two?

I will add them when I apply the patches.

Cheers,
-- 
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 0/3] crypto: arm64/aes-ccm - various bug fixes

[Ard Biesheuvel]

On Fri, 25 Jan 2019 at 07:47, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Jan 24, 2019 at 10:42:18PM -0800, Eric Biggers wrote:
> >
> > Reviewed-by: Eric Biggers <ebiggers@kernel.org>
> >
> > However, can you add Cc stable to the first two?
>
> I will add them when I apply the patches.
>

Thanks. Also, Greg and Sasha tend to pick anything with a fixes tag these days.

Re: [PATCH 0/3] crypto: arm64/aes-ccm - various bug fixes

[Ard Biesheuvel]

On Fri, 25 Jan 2019 at 07:42, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Jan 24, 2019 at 05:33:44PM +0100, Ard Biesheuvel wrote:
> > Fix a couple of bugs found by Eric's new testing code, and another
> > issue found by inspection.
> >
> > Ard Biesheuvel (3):
> >   crypto: arm64/aes-ccm - fix logical bug in AAD MAC handling
> >   crypto: arm64/aes-ccm - fix bugs in non-NEON fallback routine
> >   crypto: arm64/aes-ccm - don't use an atomic walk needlessly
> >
> >  arch/arm64/crypto/aes-ce-ccm-core.S | 5 +++--
> >  arch/arm64/crypto/aes-ce-ccm-glue.c | 8 +++-----
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> >
>
> Thanks Ard.  For all three patches:
>
> Reviewed-by: Eric Biggers <ebiggers@kernel.org>
>
> However, can you add Cc stable to the first two?
>
> BTW: I notice the !may_use_simd() case is still not covered by the self-tests.
> Do you happen to have any suggestion for how to do it?  If there is a way to
> make may_use_simd() temporarily return false, I can easily put a flag in 'struct
> testvec_config' that makes the self-tests sometimes use that context to execute
> the actual encryption/decryption/hashing.
>
> It looks like on arm64, wrapping the operation in local_irq_{disable,enable}()
> or kernel_fpu_{begin,end}() would be sufficient?  But x86 it doesn't look as
> easy as x86 needs a context where 'in_interrupt() && !interrupted_user_mode() &&
> !interrupted_kernel_fpu_idle()'.  Likewise arm32 requires 'in_interrupt()'.
>
> It would be worth doing just for arm64, but ideally it would work on most/all
> architectures.
>

I think the best way to address this is to [conditionally] wrap
may_use_simd() into something cryptoapi specific, which is under the
control of the test framework.

Re: [PATCH 0/3] crypto: arm64/aes-ccm - various bug fixes

[Herbert Xu]

On Thu, Jan 24, 2019 at 05:33:44PM +0100, Ard Biesheuvel wrote:
> Fix a couple of bugs found by Eric's new testing code, and another
> issue found by inspection.
> 
> Ard Biesheuvel (3):
>   crypto: arm64/aes-ccm - fix logical bug in AAD MAC handling
>   crypto: arm64/aes-ccm - fix bugs in non-NEON fallback routine
>   crypto: arm64/aes-ccm - don't use an atomic walk needlessly
> 
>  arch/arm64/crypto/aes-ce-ccm-core.S | 5 +++--
>  arch/arm64/crypto/aes-ce-ccm-glue.c | 8 +++-----
>  2 files changed, 6 insertions(+), 7 deletions(-)

All applied.  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