[PATCH v3 0/2] mac80211: use crypto shash for AES cmac

[PATCH v3 0/2] mac80211: use crypto shash for AES cmac

[Ard Biesheuvel]

This is something I spotted while working on AES in various modes for
ARM and arm64.

The mac80211 aes_cmac code reimplements the CMAC algorithm based on the
core AES cipher, which is rather restrictive in how platforms can satisfy
the dependency on this algorithm. For instance, SIMD implementations may
have a considerable setup time, which cannot be amortized over the entire
input when calling into the crypto API one block at a time. Also, it prevents
the use of more secure fixed time implementations, since not all AES drivers
expose the cipher interface.

So switch aes_cmac to use a cmac(aes) shash. Before updating the aes_cmac code
in patch #2, the FILS AEAD code is moved to using a cmac(aes) shash supplied by
the crypto API so that we can remove the open coded version entirely in the
second patch.

v3: - use more idiomatic SHASH_DESC_ON_STACK to allocate the shash descriptors
    - replace compound literal zero vectors with explicitly defined ones
    - drop a redundant memcpy() in #2

Ard Biesheuvel (2):
  mac80211: fils_aead: Use crypto api CMAC shash rather than bare cipher
  mac80211: aes-cmac: switch to shash CMAC driver

 net/mac80211/Kconfig     |   1 +
 net/mac80211/aes_cmac.c  | 126 ++++----------------
 net/mac80211/aes_cmac.h  |  15 +--
 net/mac80211/fils_aead.c |  74 +++++-------
 net/mac80211/key.h       |   2 +-
 5 files changed, 66 insertions(+), 152 deletions(-)

-- 
2.7.4

[PATCH v3 1/2] mac80211: fils_aead: Use crypto api CMAC shash rather than bare cipher

[Ard Biesheuvel]

Switch the FILS AEAD code to use a cmac(aes) shash instantiated by the
crypto API rather than reusing the open coded implementation in
aes_cmac_vector(). This makes the code more understandable, and allows
platforms to implement cmac(aes) in a more secure (*) and efficient way
than is typically possible when using the AES cipher directly.

So replace the crypto_cipher by a crypto_shash, and update the aes_s2v()
routine to call the shash interface directly.

* In particular, the generic table based AES implementation is sensitive
  to known-plaintext timing attacks on the key, to which AES based MAC
  algorithms are especially vulnerable, given that their plaintext is not
  usually secret. Time invariant alternatives are available (e.g., based
  on SIMD algorithms), but may incur a setup cost that is prohibitive when
  operating on a single block at a time, which is why they don't usually
  expose the cipher API.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 net/mac80211/Kconfig     |  1 +
 net/mac80211/aes_cmac.h  |  4 --
 net/mac80211/fils_aead.c | 74 +++++++++-----------
 3 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/net/mac80211/Kconfig b/net/mac80211/Kconfig
index 3891cbd2adea..76e30f4797fb 100644
--- a/net/mac80211/Kconfig
+++ b/net/mac80211/Kconfig
@@ -6,6 +6,7 @@ config MAC80211
 	select CRYPTO_AES
 	select CRYPTO_CCM
 	select CRYPTO_GCM
+	select CRYPTO_CMAC
 	select CRC32
 	---help---
 	  This option enables the hardware independent IEEE 802.11
diff --git a/net/mac80211/aes_cmac.h b/net/mac80211/aes_cmac.h
index c827e1d5de8b..3702041f44fd 100644
--- a/net/mac80211/aes_cmac.h
+++ b/net/mac80211/aes_cmac.h
@@ -11,10 +11,6 @@
 
 #include <linux/crypto.h>
 
-void gf_mulx(u8 *pad);
-void aes_cmac_vector(struct crypto_cipher *tfm, size_t num_elem,
-		     const u8 *addr[], const size_t *len, u8 *mac,
-		     size_t mac_len);
 struct crypto_cipher *ieee80211_aes_cmac_key_setup(const u8 key[],
 						   size_t key_len);
 void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
diff --git a/net/mac80211/fils_aead.c b/net/mac80211/fils_aead.c
index 5c3af5eb4052..3cfb1e2ab7ac 100644
--- a/net/mac80211/fils_aead.c
+++ b/net/mac80211/fils_aead.c
@@ -9,66 +9,58 @@
 
 #include <crypto/aes.h>
 #include <crypto/algapi.h>
+#include <crypto/hash.h>
 #include <crypto/skcipher.h>
 
 #include "ieee80211_i.h"
 #include "aes_cmac.h"
 #include "fils_aead.h"
 
-static int aes_s2v(struct crypto_cipher *tfm,
+static void gf_mulx(u8 *pad)
+{
+	u64 a = get_unaligned_be64(pad);
+	u64 b = get_unaligned_be64(pad + 8);
+
+	put_unaligned_be64((a << 1) | (b >> 63), pad);
+	put_unaligned_be64((b << 1) ^ ((a >> 63) ? 0x87 : 0), pad + 8);
+}
+
+static int aes_s2v(struct crypto_shash *tfm,
 		   size_t num_elem, const u8 *addr[], size_t len[], u8 *v)
 {
-	u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
+	u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE] = {};
+	SHASH_DESC_ON_STACK(desc, tfm);
 	size_t i;
-	const u8 *data[2];
-	size_t data_len[2], data_elems;
+
+	desc->tfm = tfm;
 
 	/* D = AES-CMAC(K, <zero>) */
-	memset(tmp, 0, AES_BLOCK_SIZE);
-	data[0] = tmp;
-	data_len[0] = AES_BLOCK_SIZE;
-	aes_cmac_vector(tfm, 1, data, data_len, d, AES_BLOCK_SIZE);
+	crypto_shash_digest(desc, tmp, AES_BLOCK_SIZE, d);
 
 	for (i = 0; i < num_elem - 1; i++) {
 		/* D = dbl(D) xor AES_CMAC(K, Si) */
 		gf_mulx(d); /* dbl */
-		aes_cmac_vector(tfm, 1, &addr[i], &len[i], tmp,
-				AES_BLOCK_SIZE);
+		crypto_shash_digest(desc, addr[i], len[i], tmp);
 		crypto_xor(d, tmp, AES_BLOCK_SIZE);
 	}
 
+	crypto_shash_init(desc);
+
 	if (len[i] >= AES_BLOCK_SIZE) {
 		/* len(Sn) >= 128 */
-		size_t j;
-		const u8 *pos;
-
 		/* T = Sn xorend D */
-
-		/* Use a temporary buffer to perform xorend on Sn (addr[i]) to
-		 * avoid modifying the const input argument.
-		 */
-		data[0] = addr[i];
-		data_len[0] = len[i] - AES_BLOCK_SIZE;
-		pos = addr[i] + data_len[0];
-		for (j = 0; j < AES_BLOCK_SIZE; j++)
-			tmp[j] = pos[j] ^ d[j];
-		data[1] = tmp;
-		data_len[1] = AES_BLOCK_SIZE;
-		data_elems = 2;
+		crypto_shash_update(desc, addr[i], len[i] - AES_BLOCK_SIZE);
+		crypto_xor(d, addr[i] + len[i] - AES_BLOCK_SIZE,
+			   AES_BLOCK_SIZE);
 	} else {
 		/* len(Sn) < 128 */
 		/* T = dbl(D) xor pad(Sn) */
 		gf_mulx(d); /* dbl */
-		memset(tmp, 0, AES_BLOCK_SIZE);
-		memcpy(tmp, addr[i], len[i]);
-		tmp[len[i]] = 0x80;
-		crypto_xor(d, tmp, AES_BLOCK_SIZE);
-		data[0] = d;
-		data_len[0] = sizeof(d);
-		data_elems = 1;
+		crypto_xor(d, addr[i], len[i]);
+		d[len[i]] ^= 0x80;
 	}
 	/* V = AES-CMAC(K, T) */
-	aes_cmac_vector(tfm, data_elems, data, data_len, v, AES_BLOCK_SIZE);
+	crypto_shash_finup(desc, d, AES_BLOCK_SIZE, v);
 
 	return 0;
 }
@@ -80,7 +72,7 @@ static int aes_siv_encrypt(const u8 *key, size_t key_len,
 			   size_t len[], u8 *out)
 {
 	u8 v[AES_BLOCK_SIZE];
-	struct crypto_cipher *tfm;
+	struct crypto_shash *tfm;
 	struct crypto_skcipher *tfm2;
 	struct skcipher_request *req;
 	int res;
@@ -95,14 +87,14 @@ static int aes_siv_encrypt(const u8 *key, size_t key_len,
 
 	/* S2V */
 
-	tfm = crypto_alloc_cipher("aes", 0, 0);
+	tfm = crypto_alloc_shash("cmac(aes)", 0, 0);
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 	/* K1 for S2V */
-	res = crypto_cipher_setkey(tfm, key, key_len);
+	res = crypto_shash_setkey(tfm, key, key_len);
 	if (!res)
 		res = aes_s2v(tfm, num_elem, addr, len, v);
-	crypto_free_cipher(tfm);
+	crypto_free_shash(tfm);
 	if (res)
 		return res;
 
@@ -157,7 +149,7 @@ static int aes_siv_decrypt(const u8 *key, size_t key_len,
 			   size_t num_elem, const u8 *addr[], size_t len[],
 			   u8 *out)
 {
-	struct crypto_cipher *tfm;
+	struct crypto_shash *tfm;
 	struct crypto_skcipher *tfm2;
 	struct skcipher_request *req;
 	struct scatterlist src[1], dst[1];
@@ -210,14 +202,14 @@ static int aes_siv_decrypt(const u8 *key, size_t key_len,
 
 	/* S2V */
 
-	tfm = crypto_alloc_cipher("aes", 0, 0);
+	tfm = crypto_alloc_shash("cmac(aes)", 0, 0);
 	if (IS_ERR(tfm))
 		return PTR_ERR(tfm);
 	/* K1 for S2V */
-	res = crypto_cipher_setkey(tfm, key, key_len);
+	res = crypto_shash_setkey(tfm, key, key_len);
 	if (!res)
 		res = aes_s2v(tfm, num_elem, addr, len, check);
-	crypto_free_cipher(tfm);
+	crypto_free_shash(tfm);
 	if (res)
 		return res;
 	if (memcmp(check, frame_iv, AES_BLOCK_SIZE) != 0)
-- 
2.7.4

Re: [PATCH v3 1/2] mac80211: fils_aead: Use crypto api CMAC shash rather than bare cipher

[Johannes Berg]

This looks strange to me:

> +static int aes_s2v(struct crypto_shash *tfm,
>  		   size_t num_elem, const u8 *addr[], size_t len[],
> u8 *v)
>  {
> -	u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
> +	u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE] = {};
> +	SHASH_DESC_ON_STACK(desc, tfm);

desc declared

> 
> +	crypto_shash_digest(desc, tmp, AES_BLOCK_SIZE, d);

used here
 
> +	crypto_shash_init(desc);

but initialized now?

johannes

Re: [PATCH v3 1/2] mac80211: fils_aead: Use crypto api CMAC shash rather than bare cipher

[Ard Biesheuvel]

On 8 February 2017 at 07:00, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
> This looks strange to me:
>
>> +static int aes_s2v(struct crypto_shash *tfm,
>>                  size_t num_elem, const u8 *addr[], size_t len[],
>> u8 *v)
>>  {
>> -     u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
>> +     u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE] = {};
>> +     SHASH_DESC_ON_STACK(desc, tfm);
>
> desc declared
>
>>
>> +     crypto_shash_digest(desc, tmp, AES_BLOCK_SIZE, d);
>
> used here
>

Each digest() call combines a init()/update()/final() sequence

>> +     crypto_shash_init(desc);
>
> but initialized now?
>

... for the 6th time, or so. The final vector may require two
update()s, so we cannot use digest() here. But we can use finup() for
the last one, which combines update() and final().

Hence,

init()/finup()

or

init()/update()/finup()

depending on the length of the last vector.

Re: [PATCH v3 1/2] mac80211: fils_aead: Use crypto api CMAC shash rather than bare cipher

[Johannes Berg]

On Wed, 2017-02-08 at 07:45 +0000, Ard Biesheuvel wrote:
> On 8 February 2017 at 07:00, Johannes Berg <johannes@sipsolutions.net
> > wrote:
> > This looks strange to me:
> > 
> > > +static int aes_s2v(struct crypto_shash *tfm,
> > >                  size_t num_elem, const u8 *addr[], size_t len[],
> > > u8 *v)
> > >  {
> > > -     u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
> > > +     u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE] = {};
> > > +     SHASH_DESC_ON_STACK(desc, tfm);
> > 
> > desc declared
> > 
> > > 
> > > +     crypto_shash_digest(desc, tmp, AES_BLOCK_SIZE, d);
> > 
> > used here
> > 
> 
> Each digest() call combines a init()/update()/final() sequence
> 
> > > +     crypto_shash_init(desc);
> > 
> > but initialized now?
> > 
> 
> ... for the 6th time, or so. The final vector may require two
> update()s, so we cannot use digest() here. But we can use finup() for
> the last one, which combines update() and final().
> 
> Hence,
> 
> init()/finup()
> 
> or
> 
> init()/update()/finup()
> 
> depending on the length of the last vector.

Great, thanks for the explanation :)

johannes

[PATCH v3 2/2] mac80211: aes-cmac: switch to shash CMAC driver

[Ard Biesheuvel]

Instead of open coding the CMAC algorithm in the mac80211 driver using
byte wide xors and calls into the crypto layer for each block of data,
instantiate a cmac(aes) synchronous hash and pass all the data into it
directly. This does not only simplify the code, it also allows the use
of more efficient and more secure implementations, especially on
platforms where SIMD ciphers have a considerable setup cost.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 net/mac80211/aes_cmac.c | 126 ++++----------------
 net/mac80211/aes_cmac.h |  11 +-
 net/mac80211/key.h      |   2 +-
 3 files changed, 32 insertions(+), 107 deletions(-)

diff --git a/net/mac80211/aes_cmac.c b/net/mac80211/aes_cmac.c
index d0bd5fff5f0a..2fb65588490c 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -22,126 +22,50 @@
 #define CMAC_TLEN_256 16 /* CMAC TLen = 128 bits (16 octets) */
 #define AAD_LEN 20
 
+static const u8 zero[CMAC_TLEN_256];
 
-void gf_mulx(u8 *pad)
-{
-	int i, carry;
-
-	carry = pad[0] & 0x80;
-	for (i = 0; i < AES_BLOCK_SIZE - 1; i++)
-		pad[i] = (pad[i] << 1) | (pad[i + 1] >> 7);
-	pad[AES_BLOCK_SIZE - 1] <<= 1;
-	if (carry)
-		pad[AES_BLOCK_SIZE - 1] ^= 0x87;
-}
-
-void aes_cmac_vector(struct crypto_cipher *tfm, size_t num_elem,
-		     const u8 *addr[], const size_t *len, u8 *mac,
-		     size_t mac_len)
-{
-	u8 cbc[AES_BLOCK_SIZE], pad[AES_BLOCK_SIZE];
-	const u8 *pos, *end;
-	size_t i, e, left, total_len;
-
-	memset(cbc, 0, AES_BLOCK_SIZE);
-
-	total_len = 0;
-	for (e = 0; e < num_elem; e++)
-		total_len += len[e];
-	left = total_len;
-
-	e = 0;
-	pos = addr[0];
-	end = pos + len[0];
-
-	while (left >= AES_BLOCK_SIZE) {
-		for (i = 0; i < AES_BLOCK_SIZE; i++) {
-			cbc[i] ^= *pos++;
-			if (pos >= end) {
-				e++;
-				pos = addr[e];
-				end = pos + len[e];
-			}
-		}
-		if (left > AES_BLOCK_SIZE)
-			crypto_cipher_encrypt_one(tfm, cbc, cbc);
-		left -= AES_BLOCK_SIZE;
-	}
-
-	memset(pad, 0, AES_BLOCK_SIZE);
-	crypto_cipher_encrypt_one(tfm, pad, pad);
-	gf_mulx(pad);
-
-	if (left || total_len == 0) {
-		for (i = 0; i < left; i++) {
-			cbc[i] ^= *pos++;
-			if (pos >= end) {
-				e++;
-				pos = addr[e];
-				end = pos + len[e];
-			}
-		}
-		cbc[left] ^= 0x80;
-		gf_mulx(pad);
-	}
-
-	for (i = 0; i < AES_BLOCK_SIZE; i++)
-		pad[i] ^= cbc[i];
-	crypto_cipher_encrypt_one(tfm, pad, pad);
-	memcpy(mac, pad, mac_len);
-}
-
-
-void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
+void ieee80211_aes_cmac(struct crypto_shash *tfm, const u8 *aad,
 			const u8 *data, size_t data_len, u8 *mic)
 {
-	const u8 *addr[3];
-	size_t len[3];
-	u8 zero[CMAC_TLEN];
+	SHASH_DESC_ON_STACK(desc, tfm);
+	u8 out[AES_BLOCK_SIZE];
 
-	memset(zero, 0, CMAC_TLEN);
-	addr[0] = aad;
-	len[0] = AAD_LEN;
-	addr[1] = data;
-	len[1] = data_len - CMAC_TLEN;
-	addr[2] = zero;
-	len[2] = CMAC_TLEN;
+	desc->tfm = tfm;
 
-	aes_cmac_vector(tfm, 3, addr, len, mic, CMAC_TLEN);
+	crypto_shash_init(desc);
+	crypto_shash_update(desc, aad, AAD_LEN);
+	crypto_shash_update(desc, data, data_len - CMAC_TLEN);
+	crypto_shash_finup(desc, zero, CMAC_TLEN, out);
+
+	memcpy(mic, out, CMAC_TLEN);
 }
 
-void ieee80211_aes_cmac_256(struct crypto_cipher *tfm, const u8 *aad,
+void ieee80211_aes_cmac_256(struct crypto_shash *tfm, const u8 *aad,
 			    const u8 *data, size_t data_len, u8 *mic)
 {
-	const u8 *addr[3];
-	size_t len[3];
-	u8 zero[CMAC_TLEN_256];
+	SHASH_DESC_ON_STACK(desc, tfm);
 
-	memset(zero, 0, CMAC_TLEN_256);
-	addr[0] = aad;
-	len[0] = AAD_LEN;
-	addr[1] = data;
-	len[1] = data_len - CMAC_TLEN_256;
-	addr[2] = zero;
-	len[2] = CMAC_TLEN_256;
+	desc->tfm = tfm;
 
-	aes_cmac_vector(tfm, 3, addr, len, mic, CMAC_TLEN_256);
+	crypto_shash_init(desc);
+	crypto_shash_update(desc, aad, AAD_LEN);
+	crypto_shash_update(desc, data, data_len - CMAC_TLEN_256);
+	crypto_shash_finup(desc, zero, CMAC_TLEN_256, mic);
 }
 
-struct crypto_cipher *ieee80211_aes_cmac_key_setup(const u8 key[],
-						   size_t key_len)
+struct crypto_shash *ieee80211_aes_cmac_key_setup(const u8 key[],
+						  size_t key_len)
 {
-	struct crypto_cipher *tfm;
+	struct crypto_shash *tfm;
 
-	tfm = crypto_alloc_cipher("aes", 0, CRYPTO_ALG_ASYNC);
+	tfm = crypto_alloc_shash("cmac(aes)", 0, 0);
 	if (!IS_ERR(tfm))
-		crypto_cipher_setkey(tfm, key, key_len);
+		crypto_shash_setkey(tfm, key, key_len);
 
 	return tfm;
 }
 
-
-void ieee80211_aes_cmac_key_free(struct crypto_cipher *tfm)
+void ieee80211_aes_cmac_key_free(struct crypto_shash *tfm)
 {
-	crypto_free_cipher(tfm);
+	crypto_free_shash(tfm);
 }
diff --git a/net/mac80211/aes_cmac.h b/net/mac80211/aes_cmac.h
index 3702041f44fd..fef531f42003 100644
--- a/net/mac80211/aes_cmac.h
+++ b/net/mac80211/aes_cmac.h
@@ -10,13 +10,14 @@
 #define AES_CMAC_H
 
 #include <linux/crypto.h>
+#include <crypto/hash.h>
 
-struct crypto_cipher *ieee80211_aes_cmac_key_setup(const u8 key[],
-						   size_t key_len);
-void ieee80211_aes_cmac(struct crypto_cipher *tfm, const u8 *aad,
+struct crypto_shash *ieee80211_aes_cmac_key_setup(const u8 key[],
+						  size_t key_len);
+void ieee80211_aes_cmac(struct crypto_shash *tfm, const u8 *aad,
 			const u8 *data, size_t data_len, u8 *mic);
-void ieee80211_aes_cmac_256(struct crypto_cipher *tfm, const u8 *aad,
+void ieee80211_aes_cmac_256(struct crypto_shash *tfm, const u8 *aad,
 			    const u8 *data, size_t data_len, u8 *mic);
-void ieee80211_aes_cmac_key_free(struct crypto_cipher *tfm);
+void ieee80211_aes_cmac_key_free(struct crypto_shash *tfm);
 
 #endif /* AES_CMAC_H */
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index 4aa20cef0859..ebdb80b85dc3 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -93,7 +93,7 @@ struct ieee80211_key {
 		} ccmp;
 		struct {
 			u8 rx_pn[IEEE80211_CMAC_PN_LEN];
-			struct crypto_cipher *tfm;
+			struct crypto_shash *tfm;
 			u32 replays; /* dot11RSNAStatsCMACReplays */
 			u32 icverrors; /* dot11RSNAStatsCMACICVErrors */
 		} aes_cmac;
-- 
2.7.4

Re: [PATCH v3 0/2] mac80211: use crypto shash for AES cmac

[Johannes Berg]

Both applied, thanks.

johannes