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

[Ard Biesheuvel]

On 6 February 2017 at 08:47, Johannes Berg  wrote:
>
>>  {
>>   u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
>> + struct shash_desc *desc;
>> + u8 buf[sizeof(*desc) + crypto_shash_descsize(tfm)]
>> CRYPTO_MINALIGN_ATTR;

I realised we have a more idiomatic SHASH_DESC_ON_STACK for this.

>>   size_t i;
>> - const u8 *data[2];
>> - size_t data_len[2], data_elems;
>> +
>> + desc = (struct shash_desc *)buf;
>> + desc->tfm = tfm;
>>
>> + crypto_shash_digest(desc, (u8[AES_BLOCK_SIZE]){},
>> AES_BLOCK_SIZE, d);
>
> That's an interesting expression in there. Can we name it into a real
> variable? :)
>

Sure, if you prefer.

> I'm also slightly worried about stack usage now - do we know none of
> this goes into an sg list eventually?
>

Shashes do not usually use scatterlists: the shash API does not use
them, but uses u8[] arrays and lengths everywhere, and shashes are
explicitly synchronous, which means they are unsuitable for being
exposed on top of a high latency peripheral that uses DMA.

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

[Johannes Berg]

>  {
>   u8 d[AES_BLOCK_SIZE], tmp[AES_BLOCK_SIZE];
> + struct shash_desc *desc;
> + u8 buf[sizeof(*desc) + crypto_shash_descsize(tfm)]
> CRYPTO_MINALIGN_ATTR;
>   size_t i;
> - const u8 *data[2];
> - size_t data_len[2], data_elems;
> +
> + desc = (struct shash_desc *)buf;
> + desc->tfm = tfm;
> 
> + crypto_shash_digest(desc, (u8[AES_BLOCK_SIZE]){},
> AES_BLOCK_SIZE, d);

That's an interesting expression in there. Can we name it into a real
variable? :)

I'm also slightly worried about stack usage now - do we know none of
this goes into an sg list eventually?

johannes

[PATCH v2 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 
---
 net/mac80211/Kconfig |  1 +
 net/mac80211/aes_cmac.h  |  4 --
 net/mac80211/fils_aead.c | 74 +---
 3 files changed, 35 insertions(+), 44 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 
 
-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..a294a57e856d 100644
--- a/net/mac80211/fils_aead.c
+++ b/net/mac80211/fils_aead.c
@@ -9,66 +9,60 @@
 
 #include 
 #include 
+#include 
 #include 
 
 #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];
+   struct shash_desc *desc;
+   u8 buf[sizeof(*desc) + crypto_shash_descsize(tfm)] CRYPTO_MINALIGN_ATTR;
size_t i;
-   const u8 *data[2];
-   size_t data_len[2], data_elems;
+
+   desc = (struct shash_desc *)buf;
+   desc->tfm = tfm;
 
/* D = AES-CMAC(K, ) */
-   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, (u8[AES_BLOCK_SIZE]){}, 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, [i], [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]);
-