On 1/30/23 4:58 PM, Herbert Xu wrote:
An often overlooked aspect of the skcipher walker API is that an
error is not just indicated by a non-zero return value, but by the
fact that walk->nbytes is zero.
Thus it is an error to call skcipher_walk_done after getting back
walk->nbytes == 0 from the previous interaction with the walker.
This is because when walk->nbytes is zero the walker is left in
an undefined state and any further calls to it may try to free
uninitialised stack memory.
The arm64 ccm code has to deal with zero-length messages, and
it needs to process data even when walk->nbytes == 0 is returned.
It doesn't have this bug because there is an explicit check for
walk->nbytes != 0 prior to the skcipher_walk_done call.
However, the loop is still sufficiently different from the usual
layout and it appears to have been copied into other code which
then ended up with this bug. This patch rewrites it to follow the
usual convention of checking walk->nbytes.
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index c4f14415f5f0..25cd3808ecbe 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
@@ -161,43 +161,39 @@ static int ccm_encrypt(struct aead_request *req)
memcpy(buf, req->iv, AES_BLOCK_SIZE);
err = skcipher_walk_aead_encrypt(&walk, req, false);
- if (unlikely(err))
- return err;
kernel_neon_begin();
if (req->assoclen)
ccm_calculate_auth_mac(req, mac);
- do {
+ while (walk.nbytes) {
u32 tail = walk.nbytes % AES_BLOCK_SIZE;
+ bool final = walk.nbytes == walk.total;
- if (walk.nbytes == walk.total)
+ if (final)
tail = 0;
ce_aes_ccm_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
walk.nbytes - tail, ctx->key_enc,
num_rounds(ctx), mac, walk.iv);
- if (walk.nbytes == walk.total)
- ce_aes_ccm_final(mac, buf, ctx->key_enc, num_rounds(ctx));
+ if (!final)
+ kernel_neon_end();
+ err = skcipher_walk_done(&walk, tail);
+ if (!final)
+ kernel_neon_begin();
+ }
- kernel_neon_end();
+ ce_aes_ccm_final(mac, buf, ctx->key_enc, num_rounds(ctx));
- if (walk.nbytes) {
- err = skcipher_walk_done(&walk, tail);
- if (unlikely(err))
- return err;
- if (unlikely(walk.nbytes))
- kernel_neon_begin();
- }
- } while (walk.nbytes);
+ kernel_neon_end();
/* copy authtag to end of dst */
scatterwalk_map_and_copy(mac, req->dst, req->assoclen + req->cryptlen,
crypto_aead_authsize(aead), 1);
- return 0;
+ return err;
}
I think the following is a more cleaner rewriting form of the loop,
which handles the last chunk separately, and both gcm and ccm can be
handled similarly.
while (walk.nbytes != walk.total) {
u32 tail = walk.nbytes % AES_BLOCK_SIZE;
ce_aes_ccm_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
walk.nbytes - tail, ctx->key_enc,
num_rounds(ctx), mac, walk.iv);
kernel_neon_end();
err = skcipher_walk_done(&walk, tail);
kernel_neon_begin();
}
if (walk.nbytes) {
ce_aes_ccm_encrypt(walk.dst.virt.addr, walk.src.virt.addr,
walk.nbytes, ctx->key_enc,
num_rounds(ctx), mac, walk.iv);
err = skcipher_walk_done(&walk, 0);
}
ce_aes_ccm_final(mac, buf, ctx->key_enc, num_rounds(ctx));
kernel_neon_end();
I have tested it initially. What are your opinions？
static int ccm_decrypt(struct aead_request *req)
@@ -219,37 +215,36 @@ static int ccm_decrypt(struct aead_request *req)
memcpy(buf, req->iv, AES_BLOCK_SIZE);
err = skcipher_walk_aead_decrypt(&walk, req, false);
- if (unlikely(err))
- return err;
kernel_neon_begin();
if (req->assoclen)
ccm_calculate_auth_mac(req, mac);
- do {
+ while (walk.nbytes) {
u32 tail = walk.nbytes % AES_BLOCK_SIZE;
+ bool final = walk.nbytes == walk.total;
- if (walk.nbytes == walk.total)
+ if (final)
tail = 0;
ce_aes_ccm_decrypt(walk.dst.virt.addr, walk.src.virt.addr,
walk.nbytes - tail, ctx->key_enc,
num_rounds(ctx), mac, walk.iv);
- if (walk.nbytes == walk.total)
- ce_aes_ccm_final(mac, buf, ctx->key_enc, num_rounds(ctx));
+ if (!final)
+ kernel_neon_end();
+ err = skcipher_walk_done(&walk, tail);
+ if (!final)
+ kernel_neon_begin();
+ }
- kernel_neon_end();
+ ce_aes_ccm_final(mac, buf, ctx->key_enc, num_rounds(ctx));
- if (walk.nbytes) {
- err = skcipher_walk_done(&walk, tail);
- if (unlikely(err))
- return err;
- if (unlikely(walk.nbytes))
- kernel_neon_begin();
- }
- } while (walk.nbytes);
+ kernel_neon_end();
+
+ if (unlikely(err))
+ return err;
/* compare calculated auth tag with the stored one */
scatterwalk_map_and_copy(buf, req->src,
