[PATCH 1/4] crypto: arm/aes-ce: fix broken monolithic build

When building the arm64 kernel with both CONFIG_CRYPTO_AES_ARM64_CE_BLK=y
and CONFIG_CRYPTO_AES_ARM64_NEON_BLK=y configured, the build breaks with
the following error:

arch/arm64/crypto/aes-neon-blk.o:(.bss+0x0): multiple definition of `aes_simd_algs'
arch/arm64/crypto/aes-ce-blk.o:(.bss+0x0): first defined here

Fix this by making aes_simd_algs 'static'.

Signed-off-by: Ard Biesheuvel <redacted>
---
 arch/arm64/crypto/aes-glue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/crypto/aes-glue.c b/arch/arm64/crypto/aes-glue.c
index 24f6137c1a6e..5c43b92b3714 100644
--- a/arch/arm64/crypto/aes-glue.c
+++ b/arch/arm64/crypto/aes-glue.c

@@ -343,7 +343,7 @@ static struct skcipher_alg aes_algs[] = { {
 	.decrypt	= xts_decrypt,
 } };
 
-struct simd_skcipher_alg *aes_simd_algs[ARRAY_SIZE(aes_algs)];
+static struct simd_skcipher_alg *aes_simd_algs[ARRAY_SIZE(aes_algs)];
 
 static void aes_exit(void)
 {

-- 
2.7.4

The new skcipher_walk_aead() may crash in the following way due to
the walk flag SKCIPHER_WALK_PHYS not being cleared at the start of the
walk:

Unable to handle kernel NULL pointer dereference at virtual address 00000001
[..]
Internal error: Oops: 96000044 [#1] PREEMPT SMP
[..]
PC is at skcipher_walk_next+0x208/0x450
LR is at skcipher_walk_next+0x1e4/0x450
pc : [<ffff2b93b7104e20>] lr : [<ffff2b93b7104dfc>] pstate: 40000045
sp : ffffb925fa517940
[...]
[<ffff2b93b7104e20>] skcipher_walk_next+0x208/0x450
[<ffff2b93b710535c>] skcipher_walk_first+0x54/0x148
[<ffff2b93b7105664>] skcipher_walk_aead+0xd4/0x108
[<ffff2b93b6e77928>] ccm_encrypt+0x68/0x158

So clear the flag at the appropriate time.

Signed-off-by: Ard Biesheuvel <redacted>
---
 crypto/skcipher.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 0f3071991b13..5367f817b40e 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c

@@ -506,6 +506,8 @@ int skcipher_walk_aead(struct skcipher_walk *walk, struct aead_request *req,
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	int err;
 
+	walk->flags &= ~SKCIPHER_WALK_PHYS;
+
 	scatterwalk_start(&walk->in, req->src);
 	scatterwalk_start(&walk->out, req->dst);
 

-- 
2.7.4

The new skcipher walk interface does not take into account whether we
are encrypting or decrypting. In the latter case, the walk should
disregard the MAC. Fix this in the arm64 CE driver.

Signed-off-by: Ard Biesheuvel <redacted>
---
 arch/arm64/crypto/aes-ce-ccm-glue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index d4f35685363b..1a011d658387 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c

@@ -204,10 +204,10 @@ static int ccm_decrypt(struct aead_request *req)
 	struct skcipher_walk walk;
 	u8 __aligned(8) mac[AES_BLOCK_SIZE];
 	u8 buf[AES_BLOCK_SIZE];
-	u32 len = req->cryptlen - authsize;
 	int err;
 
-	err = ccm_init_mac(req, mac, len);
+	req->cryptlen -= authsize;
+	err = ccm_init_mac(req, mac, req->cryptlen);
 	if (err)
 		return err;
 

@@ -242,8 +242,7 @@ static int ccm_decrypt(struct aead_request *req)
 		return err;
 
 	/* compare calculated auth tag with the stored one */
-	scatterwalk_map_and_copy(buf, req->src,
-				 req->assoclen + req->cryptlen - authsize,
+	scatterwalk_map_and_copy(buf, req->src, req->assoclen + req->cryptlen,
 				 authsize, 0);
 
 	if (crypto_memneq(mac, buf, authsize))

-- 
2.7.4

Fix a missing statement that got lost in the skcipher conversion of
the CTR transform.

Signed-off-by: Ard Biesheuvel <redacted>
---
 arch/arm64/crypto/aes-glue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/crypto/aes-glue.c b/arch/arm64/crypto/aes-glue.c
index 5c43b92b3714..4e3f8adb1793 100644
--- a/arch/arm64/crypto/aes-glue.c
+++ b/arch/arm64/crypto/aes-glue.c

@@ -206,6 +206,7 @@ static int ctr_encrypt(struct skcipher_request *req)
 				(u8 *)ctx->key_enc, rounds, blocks, walk.iv,
 				first);
 		err = skcipher_walk_done(&walk, walk.nbytes % AES_BLOCK_SIZE);
+		first = 0;
 	}
 	if (walk.nbytes) {
 		u8 __aligned(8) tail[AES_BLOCK_SIZE];

-- 
2.7.4

On Tue, Nov 29, 2016 at 01:05:32PM +0000, Ard Biesheuvel wrote:

The new skcipher walk interface does not take into account whether we
are encrypting or decrypting. In the latter case, the walk should
disregard the MAC. Fix this in the arm64 CE driver.

Signed-off-by: Ard Biesheuvel <redacted>

Thanks for the patch.  I'm going to build this into the AEAD walker
instead, by providing separate entry points for encryption and
decryption.  Like this,

---8<---
Subject: crypto: skcipher - Add separate walker for AEAD decryption

The AEAD decrypt interface includes the authentication tag in
req->cryptlen.  Therefore we need to exlucde that when doing
a walk over it.

This patch adds separate walker functions for AEAD encryption
and decryption.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 5367f81..aca07c6 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c

@@ -500,8 +500,8 @@ int skcipher_walk_async(struct skcipher_walk *walk,
 }
 EXPORT_SYMBOL_GPL(skcipher_walk_async);
 
-int skcipher_walk_aead(struct skcipher_walk *walk, struct aead_request *req,
-		       bool atomic)
+static int skcipher_walk_aead_common(struct skcipher_walk *walk,
+				     struct aead_request *req, bool atomic)
 {
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	int err;

@@ -514,7 +514,6 @@ int skcipher_walk_aead(struct skcipher_walk *walk, struct aead_request *req,
 	scatterwalk_copychunks(NULL, &walk->in, req->assoclen, 2);
 	scatterwalk_copychunks(NULL, &walk->out, req->assoclen, 2);
 
-	walk->total = req->cryptlen;
 	walk->iv = req->iv;
 	walk->oiv = req->iv;
 

@@ -535,8 +534,36 @@ int skcipher_walk_aead(struct skcipher_walk *walk, struct aead_request *req,
 
 	return err;
 }
+
+int skcipher_walk_aead(struct skcipher_walk *walk, struct aead_request *req,
+		       bool atomic)
+{
+	walk->total = req->cryptlen;
+
+	return skcipher_walk_aead_common(walk, req, atomic);
+}
 EXPORT_SYMBOL_GPL(skcipher_walk_aead);
 
+int skcipher_walk_aead_encrypt(struct skcipher_walk *walk,
+			       struct aead_request *req, bool atomic)
+{
+	walk->total = req->cryptlen;
+
+	return skcipher_walk_aead_common(walk, req, atomic);
+}
+EXPORT_SYMBOL_GPL(skcipher_walk_aead_encrypt);
+
+int skcipher_walk_aead_decrypt(struct skcipher_walk *walk,
+			       struct aead_request *req, bool atomic)
+{
+	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
+
+	walk->total = req->cryptlen - crypto_aead_authsize(tfm);
+
+	return skcipher_walk_aead_common(walk, req, atomic);
+}
+EXPORT_SYMBOL_GPL(skcipher_walk_aead_decrypt);
+
 static unsigned int crypto_skcipher_extsize(struct crypto_alg *alg)
 {
 	if (alg->cra_type == &crypto_blkcipher_type)

diff --git a/include/crypto/internal/skcipher.h b/include/crypto/internal/skcipher.h
index d55041f..8735979 100644
--- a/include/crypto/internal/skcipher.h
+++ b/include/crypto/internal/skcipher.h

@@ -149,6 +149,10 @@ int skcipher_walk_async(struct skcipher_walk *walk,
 			struct skcipher_request *req);
 int skcipher_walk_aead(struct skcipher_walk *walk, struct aead_request *req,
 		       bool atomic);
+int skcipher_walk_aead_encrypt(struct skcipher_walk *walk,
+			       struct aead_request *req, bool atomic);
+int skcipher_walk_aead_decrypt(struct skcipher_walk *walk,
+			       struct aead_request *req, bool atomic);
 void skcipher_walk_complete(struct skcipher_walk *walk, int err);
 
 static inline void ablkcipher_request_complete(struct ablkcipher_request *req,

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

On Wed, Nov 30, 2016 at 09:14:07PM +0800, Herbert Xu wrote:

On Tue, Nov 29, 2016 at 01:05:32PM +0000, Ard Biesheuvel wrote:

quoted

The new skcipher walk interface does not take into account whether we
are encrypting or decrypting. In the latter case, the walk should
disregard the MAC. Fix this in the arm64 CE driver.

Signed-off-by: Ard Biesheuvel <redacted>

Thanks for the patch.  I'm going to build this into the AEAD walker
instead, by providing separate entry points for encryption and
decryption.  Like this,

Subject: crypto: skcipher - Add separate walker for AEAD decryption

---8<---
Subject: crypto: arm64/aes-ce-ccm - Fix AEAD decryption length

This patch fixes the ARM64 CE CCM implementation decryption by
using skcipher_walk_aead_decrypt instead of skcipher_walk_aead,
which ensures the correct length is used when doing the walk.

Fixes: cf2c0fe74084 ("crypto: aes-ce-ccm - Use skcipher walk interface")
Reported-by: Ard Biesheuvel <redacted>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index d4f3568..cc5515d 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c

@@ -167,7 +167,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(&walk, req, true);
+	err = skcipher_walk_aead_encrypt(&walk, req, true);
 
 	while (walk.nbytes) {
 		u32 tail = walk.nbytes % AES_BLOCK_SIZE;

@@ -219,7 +219,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(&walk, req, true);
+	err = skcipher_walk_aead_decrypt(&walk, req, true);
 
 	while (walk.nbytes) {
 		u32 tail = walk.nbytes % AES_BLOCK_SIZE;

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

On Tue, Nov 29, 2016 at 01:05:30PM +0000, Ard Biesheuvel wrote:

When building the arm64 kernel with both CONFIG_CRYPTO_AES_ARM64_CE_BLK=y
and CONFIG_CRYPTO_AES_ARM64_NEON_BLK=y configured, the build breaks with
the following error:

arch/arm64/crypto/aes-neon-blk.o:(.bss+0x0): multiple definition of `aes_simd_algs'
arch/arm64/crypto/aes-ce-blk.o:(.bss+0x0): first defined here

Fix this by making aes_simd_algs 'static'.

Signed-off-by: Ard Biesheuvel <redacted>

Patches 1,2 and 4 applied.  Thanks.
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On 30 November 2016 at 13:14, Herbert Xu [off-list ref] wrote:

On Tue, Nov 29, 2016 at 01:05:32PM +0000, Ard Biesheuvel wrote:

quoted

The new skcipher walk interface does not take into account whether we
are encrypting or decrypting. In the latter case, the walk should
disregard the MAC. Fix this in the arm64 CE driver.

Signed-off-by: Ard Biesheuvel <redacted>

Thanks for the patch.  I'm going to build this into the AEAD walker
instead, by providing separate entry points for encryption and
decryption.  Like this,

Yes, that's better, thanks

Reviewed-by: Ard Biesheuvel <redacted>