crypto: arm64/neonbs - fix out-of-bounds access on short input
Subsystems (3)
PATCH 1/1 crypto: arm64/neonbs - fix out-of-bounds access on short input
From: Ard Biesheuvel <[email protected]>
The bit-sliced implementation of AES-CTR operates on blocks of 128 bytes, and will fall back to the plain NEON version for tail blocks or inputs that are shorter than 128 bytes to begin with.
It will call straight into the plain NEON asm helper, which performs all memory accesses in granules of 16 bytes (the size of a NEON register). For this reason, the associated plain NEON glue code will copy inputs shorter than 16 bytes into a temporary buffer, given that this is a rare occurrence and it is not worth the effort to work around this in the asm code.
The fallback from the bit-sliced NEON version fails to take this into account, potentially resulting in out-of-bounds accesses. So clone the same workaround, and use a temp buffer for short in/outputs.
--- arch/arm64/crypto/aes-neonbs-glue.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
arch/arm64/crypto/aes-neonbs-glue.c +11 -0
@@ -227,8 +227,19 @@ static int ctr_encrypt(struct skcipher_request *req) src += blocks *AES_BLOCK_SIZE ; }if (nbytes && walk.nbytes == walk.total) { +u8 buf[AES_BLOCK_SIZE ]; +u8 *d = dst; + +if (unlikely (nbytes <AES_BLOCK_SIZE )) + src = dst =memcpy (buf +sizeof (buf) - nbytes, + src, nbytes); +neon_aes_ctr_encrypt (dst, src, ctx->enc, ctx->key.rounds, nbytes, walk.iv); + +if (unlikely (nbytes <AES_BLOCK_SIZE )) +memcpy (d, buf +sizeof (buf) - nbytes, nbytes); + nbytes =0 ; }kernel_neon_end ();
Signature · 1 line
2.44.0.rc0.258.g7320e95886-goog
Quoted 14 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The bit-sliced implementation of AES-CTR operates on blocks of 128 bytes, and will fall back to the plain NEON version for tail blocks or inputs that are shorter than 128 bytes to begin with.
It will call straight into the plain NEON asm helper, which performs all memory accesses in granules of 16 bytes (the size of a NEON register). For this reason, the associated plain NEON glue code will copy inputs shorter than 16 bytes into a temporary buffer, given that this is a rare occurrence and it is not worth the effort to work around this in the asm code.
Quoted 8 lines
The fallback from the bit-sliced NEON version fails to take this into account, potentially resulting in out-of-bounds accesses. So clone the same workaround, and use a temp buffer for short in/outputs.
Cc <[email protected]>Reported-by [email protected]Tested-by [email protected]Signed-off-by Ard Biesheuvel <[email protected]>
Ping?
Quoted 31 lines
--- arch/arm64/crypto/aes-neonbs-glue.c | 11 +++++++++++ 1 file changed, 11 insertions(+)arch/arm64/crypto/aes-neonbs-glue.c +11
-1@@ -227,8 +227,19 @@ static int ctr_encrypt(struct skcipher_request *req) src += blocks *AES_BLOCK_SIZE ; }if (nbytes && walk.nbytes == walk.total) { +u8 buf[AES_BLOCK_SIZE ]; +u8 *d = dst; + +if (unlikely (nbytes <AES_BLOCK_SIZE )) + src = dst =memcpy (buf +sizeof (buf) - nbytes, + src, nbytes); +neon_aes_ctr_encrypt (dst, src, ctx->enc, ctx->key.rounds, nbytes, walk.iv); + +if (unlikely (nbytes <AES_BLOCK_SIZE )) +memcpy (d, buf +sizeof (buf) - nbytes, nbytes); + nbytes =0 ; }kernel_neon_end (); --2.44.0.rc0.258.g7320e95886-goog
Herbert Xu
maintainerQuoted 19 lines · Ard Biesheuvel
Quoted 18 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The bit-sliced implementation of AES-CTR operates on blocks of 128 bytes, and will fall back to the plain NEON version for tail blocks or inputs that are shorter than 128 bytes to begin with.
It will call straight into the plain NEON asm helper, which performs all memory accesses in granules of 16 bytes (the size of a NEON register). For this reason, the associated plain NEON glue code will copy inputs shorter than 16 bytes into a temporary buffer, given that this is a rare occurrence and it is not worth the effort to work around this in the asm code.
The fallback from the bit-sliced NEON version fails to take this into account, potentially resulting in out-of-bounds accesses. So clone the same workaround, and use a temp buffer for short in/outputs.
Quoted 6 lines
Quoted 4 lines
Cc <[email protected]>Reported-by [email protected]Tested-by [email protected]Signed-off-by Ard Biesheuvel <[email protected]>Ping?
It's in my queue. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 13 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The bit-sliced implementation of AES-CTR operates on blocks of 128 bytes, and will fall back to the plain NEON version for tail blocks or inputs that are shorter than 128 bytes to begin with.
It will call straight into the plain NEON asm helper, which performs all memory accesses in granules of 16 bytes (the size of a NEON register). For this reason, the associated plain NEON glue code will copy inputs shorter than 16 bytes into a temporary buffer, given that this is a rare occurrence and it is not worth the effort to work around this in the asm code.
Quoted 8 lines
The fallback from the bit-sliced NEON version fails to take this into account, potentially resulting in out-of-bounds accesses. So clone the same workaround, and use a temp buffer for short in/outputs.
Cc <[email protected]>Reported-by [email protected]Tested-by [email protected]Signed-off-by Ard Biesheuvel <[email protected]>
Looks like this could use:
arch/arm64/crypto/aes-neonbs-glue.c static int ctr_encrypt(struct skcipher_request *req) · line 241
Quoted 9 lines
arch/arm64/crypto/aes-neonbs-glue.c +7
-0+if (unlikely (nbytes <AES_BLOCK_SIZE )) + src = dst =memcpy (buf +sizeof (buf) - nbytes, + src, nbytes); +neon_aes_ctr_encrypt (dst, src, ctx->enc, ctx->key.rounds, nbytes, walk.iv); + +if (unlikely (nbytes <AES_BLOCK_SIZE )) +memcpy (d, buf +sizeof (buf) - nbytes, nbytes);
The second one could use 'dst' instead of 'buf + sizeof(buf) - nbytes', right?
Otherwise this looks good.
- Eric
Quoted 19 lines · Eric Biggers
Quoted 17 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The bit-sliced implementation of AES-CTR operates on blocks of 128 bytes, and will fall back to the plain NEON version for tail blocks or inputs that are shorter than 128 bytes to begin with.
It will call straight into the plain NEON asm helper, which performs all memory accesses in granules of 16 bytes (the size of a NEON register). For this reason, the associated plain NEON glue code will copy inputs shorter than 16 bytes into a temporary buffer, given that this is a rare occurrence and it is not worth the effort to work around this in the asm code.
The fallback from the bit-sliced NEON version fails to take this into account, potentially resulting in out-of-bounds accesses. So clone the same workaround, and use a temp buffer for short in/outputs.
Quoted 9 lines
Quoted 4 lines
Cc <[email protected]>Reported-by [email protected]Tested-by [email protected]Signed-off-by Ard Biesheuvel <[email protected]>Looks like this could use:
Fixes fc074e130051 ("crypto: arm64/aes-neonbs-ctr - fallback to plain NEON for final chunk")
Indeed.
Quoted 12 lines
arch/arm64/crypto/aes-neonbs-glue.c static int ctr_encrypt(struct skcipher_request *req) · line 241
Quoted 9 lines
arch/arm64/crypto/aes-neonbs-glue.c +7
-0+if (unlikely (nbytes <AES_BLOCK_SIZE )) + src = dst =memcpy (buf +sizeof (buf) - nbytes, + src, nbytes); +neon_aes_ctr_encrypt (dst, src, ctx->enc, ctx->key.rounds, nbytes, walk.iv); + +if (unlikely (nbytes <AES_BLOCK_SIZE )) +memcpy (d, buf +sizeof (buf) - nbytes, nbytes);The second one could use 'dst' instead of 'buf + sizeof(buf) - nbytes', right?
Correct.
Quoted 4 lines
Otherwise this looks good.
Reviewed-by Eric Biggers <[email protected]>
I'll respin with these changes. Thanks.