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

[RFC PATCH 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. This requires a preparatory
patch so that we can remove the open coded implementation, which it shares
with the fils aead driver. That driver could receive the same treatment, in
which case we could replace patch #1 with one that carries it over first.

Note that this is an RFC. I have no idea how I would go about testing this
code, but I am on a mission to remove as many dependencies on the generic
AES cipher as I can.

Ard Biesheuvel (2):
  mac80211: fils_aead: clone shared CMAC functions into private version
  mac80211: aes-cmac: switch to shash CMAC driver

 net/mac80211/Kconfig     |   1 +
 net/mac80211/aes_cmac.c  | 137 +++++---------------
 net/mac80211/aes_cmac.h  |  15 +--
 net/mac80211/fils_aead.c |  68 ++++++++++
 net/mac80211/key.h       |   2 +-
 5 files changed, 110 insertions(+), 113 deletions(-)

-- 
2.7.4

[RFC PATCH 1/2] mac80211: fils_aead: clone shared CMAC functions into private version

[Ard Biesheuvel]

Before reworking the AES CMAC mac80211 code, clone the routines that it
shares with the FILS AEAD driver into its own source file, and remove the
external declaration from aes_cmac.h. This will allow us to carry over one
user at a time from the open coded CMAC code to the crypto API.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 net/mac80211/aes_cmac.h  |  4 --
 net/mac80211/fils_aead.c | 68 ++++++++++++++++++++
 2 files changed, 68 insertions(+), 4 deletions(-)

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 ecfdd97758a3..ec493e68957c 100644
--- a/net/mac80211/fils_aead.c
+++ b/net/mac80211/fils_aead.c
@@ -15,6 +15,74 @@
 #include "aes_cmac.h"
 #include "fils_aead.h"
 
+static 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;
+}
+
+static 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);
+}
+
 static int aes_s2v(struct crypto_cipher *tfm,
 		   size_t num_elem, const u8 *addr[], size_t len[], u8 *v)
 {
-- 
2.7.4

[RFC PATCH 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 considerable setup time.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 net/mac80211/Kconfig    |   1 +
 net/mac80211/aes_cmac.c | 137 +++++---------------
 net/mac80211/aes_cmac.h |  11 +-
 net/mac80211/key.h      |   2 +-
 4 files changed, 42 insertions(+), 109 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.c b/net/mac80211/aes_cmac.c
index d0bd5fff5f0a..fc004720e77c 100644
--- a/net/mac80211/aes_cmac.c
+++ b/net/mac80211/aes_cmac.c
@@ -22,126 +22,57 @@
 #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)
+void ieee80211_aes_cmac(struct crypto_shash *tfm, const u8 *aad,
+			const u8 *data, size_t data_len, u8 *mic)
 {
-	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;
-}
+	struct shash_desc *desc;
+	u8 buf[sizeof(*desc) + crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
+	u8 out[crypto_shash_digestsize(tfm)];
 
-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);
-}
+	desc = (struct shash_desc *)buf;
+	desc->tfm = tfm;
 
+	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);
 
-void ieee80211_aes_cmac(struct crypto_cipher *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];
-
-	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;
-
-	aes_cmac_vector(tfm, 3, addr, len, mic, CMAC_TLEN);
+	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];
-
-	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;
-
-	aes_cmac_vector(tfm, 3, addr, len, mic, CMAC_TLEN_256);
+	struct shash_desc *desc;
+	u8 buf[sizeof(*desc) + crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
+	u8 out[crypto_shash_digestsize(tfm)];
+
+	desc = (struct shash_desc *)buf;
+	desc->tfm = tfm;
+
+	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, out);
+
+	memcpy(mic, out, CMAC_TLEN_256);
 }
 
-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: [RFC PATCH 0/2] mac80211: use crypto shash for AES cmac

[Malinen, Jouni]

On Fri, Feb 03, 2017 at 07:25:53PM +0000, Ard Biesheuvel wrote:
> 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. This requires a preparatory
> patch so that we can remove the open coded implementation, which it shares
> with the fils aead driver. That driver could receive the same treatment, in
> which case we could replace patch #1 with one that carries it over first.
> 
> Note that this is an RFC. I have no idea how I would go about testing this
> code, but I am on a mission to remove as many dependencies on the generic
> AES cipher as I can.

Neither the BIP nor FILS cases have any real speed requirements taken
into account how rarely they end up being used in practice (there is
really no use case for BIP today and FILS is used only once per
association). That said, there should be no issues with moving these to
a more generic mechanism assuming one is available now (I don't think
that was the case when I was working on BIP and I was too lazy to figure
out how to convert it or the newer FILS implementation)..

mac80211_hwsim show allow some of the testing to be done with wlantest
confirming the results in user space (*). I think that would cover all
of BIP (net/mac80211/aes_cmac.c), but not FILS. For FILS, we do not
currently have a convenient mechanism for running two different
instances of kernel or even just mac80211 in the setup, so that would
likely need testing with real WLAN hardware. I don't currently have a
good setup for testing this (was using Backports-based solution in the
past instead of full kernel build and Backports is a bit behind the
current state..), but I guess I'll need to build something functional
for this eventually.. Once that's in working condition on two devices,
it would be straightforward to run a test (snapshot of hostap.git build
to enable FILS functionality and go through one FILS authentication
round)..

Another alternative would be to extend wlantest to decrypt/validate FIPS
AEAD use case based on keys exposed from hostapd or wpa_supplicant.
There has not been sufficient use case for that so far and I have not
bothered working on it yet.


By the way, FILS AEAD uses SIV mode and I'm not sure it is supported in
the current crypto code, so that would be one additional piece to take
care of when considering net/mac80211/fils_aead.c conversion.


(*)
http://buildbot.w1.fi/hwsim/
http://w1.fi/cgit/hostap/tree/tests/hwsim/vm/example-vm-setup.txt

-- 
Jouni Malinen                                            PGP id EFC895FA

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

[Ard Biesheuvel]

On 3 February 2017 at 21:47, Malinen, Jouni <jouni@qca.qualcomm.com> wrote:
> On Fri, Feb 03, 2017 at 07:25:53PM +0000, Ard Biesheuvel wrote:
>> 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. This requires a preparatory
>> patch so that we can remove the open coded implementation, which it shares
>> with the fils aead driver. That driver could receive the same treatment, in
>> which case we could replace patch #1 with one that carries it over first.
>>
>> Note that this is an RFC. I have no idea how I would go about testing this
>> code, but I am on a mission to remove as many dependencies on the generic
>> AES cipher as I can.
>
> Neither the BIP nor FILS cases have any real speed requirements taken
> into account how rarely they end up being used in practice (there is
> really no use case for BIP today and FILS is used only once per
> association). That said, there should be no issues with moving these to
> a more generic mechanism assuming one is available now (I don't think
> that was the case when I was working on BIP and I was too lazy to figure
> out how to convert it or the newer FILS implementation)..
>
> mac80211_hwsim show allow some of the testing to be done with wlantest
> confirming the results in user space (*). I think that would cover all
> of BIP (net/mac80211/aes_cmac.c), but not FILS.

OK, that looks like something I could figure out how to use. But are
you saying the CMAC code is never called in practice?

> For FILS, we do not
> currently have a convenient mechanism for running two different
> instances of kernel or even just mac80211 in the setup, so that would
> likely need testing with real WLAN hardware. I don't currently have a
> good setup for testing this (was using Backports-based solution in the
> past instead of full kernel build and Backports is a bit behind the
> current state..), but I guess I'll need to build something functional
> for this eventually.. Once that's in working condition on two devices,
> it would be straightforward to run a test (snapshot of hostap.git build
> to enable FILS functionality and go through one FILS authentication
> round)..
>
> Another alternative would be to extend wlantest to decrypt/validate FIPS
> AEAD use case based on keys exposed from hostapd or wpa_supplicant.
> There has not been sufficient use case for that so far and I have not
> bothered working on it yet.
>
>
> By the way, FILS AEAD uses SIV mode and I'm not sure it is supported in
> the current crypto code, so that would be one additional piece to take
> care of when considering net/mac80211/fils_aead.c conversion.
>

I did spot something peculiar when looking at the code: if I am
reading the following sequence correctly (from
fils_encrypt_assoc_req())

addr[0] = mgmt->sa;
len[0] = ETH_ALEN;
/* The AP's BSSID */
addr[1] = mgmt->da;
len[1] = ETH_ALEN;
/* The STA's nonce */
addr[2] = assoc_data->fils_nonces;
len[2] = FILS_NONCE_LEN;
/* The AP's nonce */
addr[3] = &assoc_data->fils_nonces[FILS_NONCE_LEN];
len[3] = FILS_NONCE_LEN;
/* The (Re)Association Request frame from the Capability Information
* field to the FILS Session element (both inclusive).
*/
addr[4] = capab;
len[4] = encr - capab;

crypt_len = skb->data + skb->len - encr;
skb_put(skb, AES_BLOCK_SIZE);
return aes_siv_encrypt(assoc_data->fils_kek, assoc_data->fils_kek_len,
      encr, crypt_len, 1, addr, len, encr);

the addr[]/len[] arrays are populated with 5 (addr, len) pairs, but
only one is actually passed into aes_siv_encrypt()? This is actually
the main reason I stopped looking into whether I could convert it to
CMAC, because I couldn't figure it out.

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

[Malinen, Jouni]

On Fri, Feb 03, 2017 at 09:55:36PM +0000, Ard Biesheuvel wrote:
> OK, that looks like something I could figure out how to use. But are
> you saying the CMAC code is never called in practice?

It will get called if there is a frame that were to need to use BIP.
There are some APs that do send broadcast addressed Deauthentication and
Disassociation frames when terminating the network and those would get
here. In theory, someone could come up with a new Action frame use case
as well with a group addressed Robust Action frame, but no such thing is
defined today. Anyway, this will be needed for certification purposes
and to block DoS with broadcast addressed Deauthentication and
Disassociation frames even if there is not really a common use case
using BIP frames today.

> I did spot something peculiar when looking at the code: if I am
> reading the following sequence correctly (from
> fils_encrypt_assoc_req())

> addr[0] = mgmt->sa;
...
> addr[4] = capab;
> len[4] = encr - capab;
> 
> crypt_len = skb->data + skb->len - encr;
> skb_put(skb, AES_BLOCK_SIZE);
> return aes_siv_encrypt(assoc_data->fils_kek, assoc_data->fils_kek_len,
>       encr, crypt_len, 1, addr, len, encr);
> 
> the addr[]/len[] arrays are populated with 5 (addr, len) pairs, but
> only one is actually passed into aes_siv_encrypt()? This is actually
> the main reason I stopped looking into whether I could convert it to
> CMAC, because I couldn't figure it out.

Argh.. Thanks for finding this!

That is indeed supposed to have num_elems == 5. This "works" since the
other end of the exchange is actually in user space (hostapd) and a
similar error is there..

This comes from the development history of FILS support.. The draft
standard changed this AES-SIV AAD design from P802.11ai/D7.0 to D8.0
from a single AAD vector with concatenated fields to separate vectors.
Clearly I added the vectors here in addr[] and len[] arrays, but forgot
to update the num_elems parameter in this direction (STA->AP); the other
direction for Association Response frame is actually using correct
number of AAD vectors.

I'll fix this in hostapd and mac80211.


I did not actually remember that the AP side was still in hostapd for
FILS AEAD in case of mac80211, but with that in mind, the answer to your
earlier question about testing these becomes much simpler.. All you need
to do is this with hostap.git hwsim tests:

hostap/tests/hwsim/vm$ ./vm-run.sh ap_cipher_bip fils_sk_erp
Starting test run in a virtual machine
./run-all.sh: passing the following args to run-tests.py: ap_cipher_bip fils_sk_erp 
START ap_cipher_bip 1/2
PASS ap_cipher_bip 0.703048 2017-02-04 11:21:35.341174
START fils_sk_erp 2/2
PASS fils_sk_erp 0.667927 2017-02-04 11:21:36.029205
passed all 2 test case(s)
ALL-PASSED


ap_cipher_bip uses wlantest to verify the BIP (AES-128-CMAC) protection
in the frames and fils_sk_erp goes through FILS AEAD exchange with one
side of the operation in mac80211 and the other one in hostapd. This
should be able to discover if something is broken in the AES related
changes in crypto.

And this particular example run here was with your two patches applied,
to the proposed net/mac80211/aes_cmac.c change seems to work fine.

-- 
Jouni Malinen                                            PGP id EFC895FA

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

[Ard Biesheuvel]

On 4 February 2017 at 11:35, Malinen, Jouni <jouni@qca.qualcomm.com> wrote:
> On Fri, Feb 03, 2017 at 09:55:36PM +0000, Ard Biesheuvel wrote:
>> OK, that looks like something I could figure out how to use. But are
>> you saying the CMAC code is never called in practice?
>
> It will get called if there is a frame that were to need to use BIP.
> There are some APs that do send broadcast addressed Deauthentication and
> Disassociation frames when terminating the network and those would get
> here. In theory, someone could come up with a new Action frame use case
> as well with a group addressed Robust Action frame, but no such thing is
> defined today. Anyway, this will be needed for certification purposes
> and to block DoS with broadcast addressed Deauthentication and
> Disassociation frames even if there is not really a common use case
> using BIP frames today.
>
>> I did spot something peculiar when looking at the code: if I am
>> reading the following sequence correctly (from
>> fils_encrypt_assoc_req())
>
>> addr[0] = mgmt->sa;
> ...
>> addr[4] = capab;
>> len[4] = encr - capab;
>>
>> crypt_len = skb->data + skb->len - encr;
>> skb_put(skb, AES_BLOCK_SIZE);
>> return aes_siv_encrypt(assoc_data->fils_kek, assoc_data->fils_kek_len,
>>       encr, crypt_len, 1, addr, len, encr);
>>
>> the addr[]/len[] arrays are populated with 5 (addr, len) pairs, but
>> only one is actually passed into aes_siv_encrypt()? This is actually
>> the main reason I stopped looking into whether I could convert it to
>> CMAC, because I couldn't figure it out.
>
> Argh.. Thanks for finding this!
>
> That is indeed supposed to have num_elems == 5. This "works" since the
> other end of the exchange is actually in user space (hostapd) and a
> similar error is there..
>

Interesting.

> This comes from the development history of FILS support.. The draft
> standard changed this AES-SIV AAD design from P802.11ai/D7.0 to D8.0
> from a single AAD vector with concatenated fields to separate vectors.
> Clearly I added the vectors here in addr[] and len[] arrays, but forgot
> to update the num_elems parameter in this direction (STA->AP); the other
> direction for Association Response frame is actually using correct
> number of AAD vectors.
>
> I'll fix this in hostapd and mac80211.
>

There is another issue I spotted: the skcipher you allocate may be of
the async variant, which may return from skcipher_encrypt() with
-EINPROGRESS as soon as it has queued the request. Since you don't
deal with that result, you should allocate a sync cipher explicitly:

diff --git a/net/mac80211/fils_aead.c b/net/mac80211/fils_aead.c
index ecfdd97758a3..a31be5262283 100644
--- a/net/mac80211/fils_aead.c
+++ b/net/mac80211/fils_aead.c
@@ -124,7 +124,7 @@ static int aes_siv_encrypt(const u8 *key, size_t key_len,

        /* CTR */

-       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, 0);
+       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
        if (IS_ERR(tfm2)) {
                kfree(tmp);
                return PTR_ERR(tfm2);
@@ -183,7 +183,7 @@ static int aes_siv_decrypt(const u8 *key, size_t key_len,

        /* CTR */

-       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, 0);
+       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
        if (IS_ERR(tfm2))
                return PTR_ERR(tfm2);
        /* K2 for CTR */

> I did not actually remember that the AP side was still in hostapd for
> FILS AEAD in case of mac80211, but with that in mind, the answer to your
> earlier question about testing these becomes much simpler.. All you need
> to do is this with hostap.git hwsim tests:
>
> hostap/tests/hwsim/vm$ ./vm-run.sh ap_cipher_bip fils_sk_erp
> Starting test run in a virtual machine
> ./run-all.sh: passing the following args to run-tests.py: ap_cipher_bip fils_sk_erp
> START ap_cipher_bip 1/2
> PASS ap_cipher_bip 0.703048 2017-02-04 11:21:35.341174
> START fils_sk_erp 2/2
> PASS fils_sk_erp 0.667927 2017-02-04 11:21:36.029205
> passed all 2 test case(s)
> ALL-PASSED
>
>
> ap_cipher_bip uses wlantest to verify the BIP (AES-128-CMAC) protection
> in the frames and fils_sk_erp goes through FILS AEAD exchange with one
> side of the operation in mac80211 and the other one in hostapd. This
> should be able to discover if something is broken in the AES related
> changes in crypto.
>
> And this particular example run here was with your two patches applied,
> to the proposed net/mac80211/aes_cmac.c change seems to work fine.

Thanks for the instructions and thanks for testing. If I manage to run
this locally, I will follow up with an alternative patch #1 that
switches FILS to use cmac(aes) as well (which looks reasonably
feasible now that I understand the code)

Regards,
Ard.

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

[Malinen, Jouni]

On Sat, Feb 04, 2017 at 02:24:36PM +0000, Ard Biesheuvel wrote:
> There is another issue I spotted: the skcipher you allocate may be of
> the async variant, which may return from skcipher_encrypt() with
> -EINPROGRESS as soon as it has queued the request. Since you don't
> deal with that result, you should allocate a sync cipher explicitly:

> diff --git a/net/mac80211/fils_aead.c b/net/mac80211/fils_aead.c
> -       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, 0);
> +       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);

> -       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, 0);
> +       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);

Thanks! Can you send this as a full contribution or do you want me to
do that? I did run this through all the FILS test cases with
mac80211_hwsim.

> Thanks for the instructions and thanks for testing. If I manage to run
> this locally, I will follow up with an alternative patch #1 that
> switches FILS to use cmac(aes) as well (which looks reasonably
> feasible now that I understand the code)

If you have any issues in getting the hwsim test setup working, please
let me know. I'm trying to make it easy for anyone to run those tests in
hopes of improving quality of Linux WLAN contributions and what gets
applied into cfg80211 or mac80211 (or hostap.git for that matter). In
particular the latest step-by-step guide I added for the VM version (*)
was hoping to show how that can be done in 15 minutes from scratch..


(*) http://w1.fi/cgit/hostap/plain/tests/hwsim/vm/example-vm-setup.txt

-- 
Jouni Malinen                                            PGP id EFC895FA

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

[Ard Biesheuvel]

On 4 February 2017 at 14:39, Malinen, Jouni <jouni@qca.qualcomm.com> wrote:
> On Sat, Feb 04, 2017 at 02:24:36PM +0000, Ard Biesheuvel wrote:
>> There is another issue I spotted: the skcipher you allocate may be of
>> the async variant, which may return from skcipher_encrypt() with
>> -EINPROGRESS as soon as it has queued the request. Since you don't
>> deal with that result, you should allocate a sync cipher explicitly:
>
>> diff --git a/net/mac80211/fils_aead.c b/net/mac80211/fils_aead.c
>> -       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, 0);
>> +       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
>
>> -       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, 0);
>> +       tfm2 = crypto_alloc_skcipher("ctr(aes)", 0, CRYPTO_ALG_ASYNC);
>
> Thanks! Can you send this as a full contribution or do you want me to
> do that?

Please go ahead if you don't mind doing it

> I did run this through all the FILS test cases with
> mac80211_hwsim.
>

Well, even async ciphers can act synchronously: the SIMD based async
ciphers will only enqueue the request for deferred processing when
called in interrupt context (on most architectures) but if you happen
to run on a platform that has a h/w accelerator for ctr(aes), you are
quite likely to see failures here.

>> Thanks for the instructions and thanks for testing. If I manage to run
>> this locally, I will follow up with an alternative patch #1 tha here
>> switches FILS to use cmac(aes) as well (which looks reasonably
>> feasible now that I understand the code)
>
> If you have any issues in getting the hwsim test setup working, please
> let me know. I'm trying to make it easy for anyone to run those tests in
> hopes of improving quality of Linux WLAN contributions and what gets
> applied into cfg80211 or mac80211 (or hostap.git for that matter). In
> particular the latest step-by-step guide I added for the VM version (*)
> was hoping to show how that can be done in 15 minutes from scratch..
>
>
> (*) http://w1.fi/cgit/hostap/plain/tests/hwsim/vm/example-vm-setup.txt
>

I will take a look on Monday