[PATCH 0/1] crypto: Undefined behaviour in crypto_aes_expand_key

[PATCH 0/1] crypto: Undefined behaviour in crypto_aes_expand_key

[Phil Carmody]

The following patch applies to the current head of torvalds/linux-2.6.git.
However, due to the relatively stable nature of the only file patched, it
should apply anywhere. Apologies if there are any mail mangling issues, 
they aren't unheard of, alas, and I will resend from home if need be. 

The nitty gritty details of the undefined behaviour can be found in the C
standards documents sections:
n869.txt (C89) - 3.3.2.1 Array subscripting, and 3.3.6 Additive operators
n1256.pdf (C99) - 6.5.2.1 Array subscripting, and 6.5.6 Additive operators

I trust that the nature of the patch should be self-evident.

[PATCH 1/1] crypto: Undefined behaviour in crypto_aes_expand_key

[Phil Carmody]

It's undefined behaviour in C to write outside the bounds of an array.
The key expansion routine takes a shortcut of creating 8 words at a
time, but this creates 4 additional words which don't fit in the array.

As everyone is hopefully now aware, GCC is at liberty to make any
assumptions and optimisations it likes in situations where it can
detect that UB has occured, up to and including nasal demons, and
as the indices being accessed in the array are trivially calculable,
it's rash to invite gcc to do take any liberties at all.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 crypto/aes_generic.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
index b8b66ec..e78b7ee 100644
--- a/crypto/aes_generic.c
+++ b/crypto/aes_generic.c
@@ -1174,7 +1174,7 @@ EXPORT_SYMBOL_GPL(crypto_il_tab);
 	ctx->key_enc[6 * i + 11] = t;		\
 } while (0)
 
-#define loop8(i)	do {			\
+#define loop8tophalf(i)	do {			\
 	t = ror32(t, 8);			\
 	t = ls_box(t) ^ rco_tab[i];		\
 	t ^= ctx->key_enc[8 * i];			\
@@ -1185,6 +1185,10 @@ EXPORT_SYMBOL_GPL(crypto_il_tab);
 	ctx->key_enc[8 * i + 10] = t;			\
 	t ^= ctx->key_enc[8 * i + 3];			\
 	ctx->key_enc[8 * i + 11] = t;			\
+} while (0)
+
+#define loop8(i)	do {				\
+	loop8tophalf(i);				\
 	t  = ctx->key_enc[8 * i + 4] ^ ls_box(t);	\
 	ctx->key_enc[8 * i + 12] = t;			\
 	t ^= ctx->key_enc[8 * i + 5];			\
@@ -1245,8 +1249,9 @@ int crypto_aes_expand_key(struct crypto_aes_ctx *ctx, const u8 *in_key,
 		ctx->key_enc[5] = le32_to_cpu(key[5]);
 		ctx->key_enc[6] = le32_to_cpu(key[6]);
 		t = ctx->key_enc[7] = le32_to_cpu(key[7]);
-		for (i = 0; i < 7; ++i)
+		for (i = 0; i < 6; ++i)
 			loop8(i);
+		loop8tophalf(i);
 		break;
 	}
 
-- 
1.5.4.3

Re: [PATCH 1/1] crypto: Undefined behaviour in crypto_aes_expand_key

[Herbert Xu]

On Wed, Jul 22, 2009 at 05:57:03PM +0300, Phil Carmody wrote:
> It's undefined behaviour in C to write outside the bounds of an array.
> The key expansion routine takes a shortcut of creating 8 words at a
> time, but this creates 4 additional words which don't fit in the array.
> 
> As everyone is hopefully now aware, GCC is at liberty to make any
> assumptions and optimisations it likes in situations where it can
> detect that UB has occured, up to and including nasal demons, and
> as the indices being accessed in the array are trivially calculable,
> it's rash to invite gcc to do take any liberties at all.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Applied to cryptodev.  Thanks!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt