[PATCH] wireless: airo: switch to skcipher interface

[PATCH] wireless: airo: switch to skcipher interface

[Ard Biesheuvel]

The AIRO driver applies a ctr(aes) on a buffer of considerable size
(2400 bytes), and instead of invoking the crypto API to handle this
in its entirety, it open codes the counter manipulation and invokes
the AES block cipher directly.

Let's fix this, by switching to the sync skcipher API instead.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
NOTE: build tested only, since I don't have the hardware

 drivers/net/wireless/cisco/airo.c | 57 ++++++++++----------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
index 3f5a14112c6b..2d29ad10505b 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -49,6 +49,9 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 
+#include <crypto/aes.h>
+#include <crypto/skcipher.h>
+
 #include <net/cfg80211.h>
 #include <net/iw_handler.h>
 
@@ -951,7 +954,7 @@ typedef struct {
 } mic_statistics;
 
 typedef struct {
-	u32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
+	__be32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
 	u64 accum;	// accumulated mic, reduced to u32 in final()
 	int position;	// current position (byte offset) in message
 	union {
@@ -1216,7 +1219,7 @@ struct airo_info {
 	struct iw_spy_data	spy_data;
 	struct iw_public_data	wireless_data;
 	/* MIC stuff */
-	struct crypto_cipher	*tfm;
+	struct crypto_sync_skcipher	*tfm;
 	mic_module		mod[2];
 	mic_statistics		micstats;
 	HostRxDesc rxfids[MPI_MAX_FIDS]; // rx/tx/config MPI350 descriptors
@@ -1291,14 +1294,14 @@ static int flashrestart(struct airo_info *ai,struct net_device *dev);
 static int RxSeqValid (struct airo_info *ai,miccntx *context,int mcast,u32 micSeq);
 static void MoveWindow(miccntx *context, u32 micSeq);
 static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
-			   struct crypto_cipher *tfm);
+			   struct crypto_sync_skcipher *tfm);
 static void emmh32_init(emmh32_context *context);
 static void emmh32_update(emmh32_context *context, u8 *pOctets, int len);
 static void emmh32_final(emmh32_context *context, u8 digest[4]);
 static int flashpchar(struct airo_info *ai,int byte,int dwelltime);
 
 static void age_mic_context(miccntx *cur, miccntx *old, u8 *key, int key_len,
-			    struct crypto_cipher *tfm)
+			    struct crypto_sync_skcipher *tfm)
 {
 	/* If the current MIC context is valid and its key is the same as
 	 * the MIC register, there's nothing to do.
@@ -1359,7 +1362,7 @@ static int micsetup(struct airo_info *ai) {
 	int i;
 
 	if (ai->tfm == NULL)
-		ai->tfm = crypto_alloc_cipher("aes", 0, 0);
+		ai->tfm = crypto_alloc_sync_skcipher("ctr(aes)", 0, 0);
 
         if (IS_ERR(ai->tfm)) {
                 airo_print_err(ai->dev->name, "failed to load transform for AES");
@@ -1624,37 +1627,31 @@ static void MoveWindow(miccntx *context, u32 micSeq)
 
 /* mic accumulate */
 #define MIC_ACCUM(val)	\
-	context->accum += (u64)(val) * context->coeff[coeff_position++];
-
-static unsigned char aes_counter[16];
+	context->accum += (u64)(val) * be32_to_cpu(context->coeff[coeff_position++]);
 
 /* expand the key to fill the MMH coefficient array */
 static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
-			   struct crypto_cipher *tfm)
+			   struct crypto_sync_skcipher *tfm)
 {
   /* take the keying material, expand if necessary, truncate at 16-bytes */
   /* run through AES counter mode to generate context->coeff[] */
   
-	int i,j;
-	u32 counter;
-	u8 *cipher, plain[16];
-
-	crypto_cipher_setkey(tfm, pkey, 16);
-	counter = 0;
-	for (i = 0; i < ARRAY_SIZE(context->coeff); ) {
-		aes_counter[15] = (u8)(counter >> 0);
-		aes_counter[14] = (u8)(counter >> 8);
-		aes_counter[13] = (u8)(counter >> 16);
-		aes_counter[12] = (u8)(counter >> 24);
-		counter++;
-		memcpy (plain, aes_counter, 16);
-		crypto_cipher_encrypt_one(tfm, plain, plain);
-		cipher = plain;
-		for (j = 0; (j < 16) && (i < ARRAY_SIZE(context->coeff)); ) {
-			context->coeff[i++] = ntohl(*(__be32 *)&cipher[j]);
-			j += 4;
-		}
-	}
+	SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
+	struct scatterlist dst, src;
+	u8 iv[AES_BLOCK_SIZE] = {};
+	int ret;
+
+	crypto_sync_skcipher_setkey(tfm, pkey, 16);
+
+	sg_init_one(&dst, context->coeff, sizeof(context->coeff));
+	sg_init_one(&src, page_address(ZERO_PAGE(0)), sizeof(context->coeff));
+
+	skcipher_request_set_sync_tfm(req, tfm);
+	skcipher_request_set_callback(req, 0, NULL, NULL);
+	skcipher_request_set_crypt(req, &src, &dst, sizeof(context->coeff), iv);
+
+	ret = crypto_skcipher_encrypt(req);
+	WARN_ON_ONCE(ret);
 }
 
 /* prepare for calculation of a new mic */
@@ -2415,7 +2412,7 @@ void stop_airo_card( struct net_device *dev, int freeres )
 				ai->shared, ai->shared_dma);
 		}
         }
-	crypto_free_cipher(ai->tfm);
+	crypto_free_sync_skcipher(ai->tfm);
 	del_airo_dev(ai);
 	free_netdev( dev );
 }
-- 
2.20.1

Re: [PATCH] wireless: airo: switch to skcipher interface

[Eric Biggers]

On Fri, Jun 14, 2019 at 11:36:03AM +0200, Ard Biesheuvel wrote:
> The AIRO driver applies a ctr(aes) on a buffer of considerable size
> (2400 bytes), and instead of invoking the crypto API to handle this
> in its entirety, it open codes the counter manipulation and invokes
> the AES block cipher directly.
> 
> Let's fix this, by switching to the sync skcipher API instead.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> NOTE: build tested only, since I don't have the hardware
> 
>  drivers/net/wireless/cisco/airo.c | 57 ++++++++++----------
>  1 file changed, 27 insertions(+), 30 deletions(-)
> 

Need to also select CRYPTO_CTR in drivers/net/wireless/cisco/Kconfig under
AIRO_CS, and I think also CRYPTO_BLKCIPHER under AIRO.

> diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
> index 3f5a14112c6b..2d29ad10505b 100644
> --- a/drivers/net/wireless/cisco/airo.c
> +++ b/drivers/net/wireless/cisco/airo.c
> @@ -49,6 +49,9 @@
>  #include <linux/kthread.h>
>  #include <linux/freezer.h>
>  
> +#include <crypto/aes.h>
> +#include <crypto/skcipher.h>
> +
>  #include <net/cfg80211.h>
>  #include <net/iw_handler.h>
>  
> @@ -951,7 +954,7 @@ typedef struct {
>  } mic_statistics;
>  
>  typedef struct {
> -	u32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
> +	__be32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
>  	u64 accum;	// accumulated mic, reduced to u32 in final()
>  	int position;	// current position (byte offset) in message
>  	union {
> @@ -1216,7 +1219,7 @@ struct airo_info {
>  	struct iw_spy_data	spy_data;
>  	struct iw_public_data	wireless_data;
>  	/* MIC stuff */
> -	struct crypto_cipher	*tfm;
> +	struct crypto_sync_skcipher	*tfm;
>  	mic_module		mod[2];
>  	mic_statistics		micstats;
>  	HostRxDesc rxfids[MPI_MAX_FIDS]; // rx/tx/config MPI350 descriptors
> @@ -1291,14 +1294,14 @@ static int flashrestart(struct airo_info *ai,struct net_device *dev);
>  static int RxSeqValid (struct airo_info *ai,miccntx *context,int mcast,u32 micSeq);
>  static void MoveWindow(miccntx *context, u32 micSeq);
>  static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
> -			   struct crypto_cipher *tfm);
> +			   struct crypto_sync_skcipher *tfm);
>  static void emmh32_init(emmh32_context *context);
>  static void emmh32_update(emmh32_context *context, u8 *pOctets, int len);
>  static void emmh32_final(emmh32_context *context, u8 digest[4]);
>  static int flashpchar(struct airo_info *ai,int byte,int dwelltime);
>  
>  static void age_mic_context(miccntx *cur, miccntx *old, u8 *key, int key_len,
> -			    struct crypto_cipher *tfm)
> +			    struct crypto_sync_skcipher *tfm)
>  {
>  	/* If the current MIC context is valid and its key is the same as
>  	 * the MIC register, there's nothing to do.
> @@ -1359,7 +1362,7 @@ static int micsetup(struct airo_info *ai) {
>  	int i;
>  
>  	if (ai->tfm == NULL)
> -		ai->tfm = crypto_alloc_cipher("aes", 0, 0);
> +		ai->tfm = crypto_alloc_sync_skcipher("ctr(aes)", 0, 0);
>  
>          if (IS_ERR(ai->tfm)) {
>                  airo_print_err(ai->dev->name, "failed to load transform for AES");
> @@ -1624,37 +1627,31 @@ static void MoveWindow(miccntx *context, u32 micSeq)
>  
>  /* mic accumulate */
>  #define MIC_ACCUM(val)	\
> -	context->accum += (u64)(val) * context->coeff[coeff_position++];
> -
> -static unsigned char aes_counter[16];
> +	context->accum += (u64)(val) * be32_to_cpu(context->coeff[coeff_position++]);

You could alternatively call be32_to_cpu_array() after the AES encryption.
But this works too.

>  
>  /* expand the key to fill the MMH coefficient array */
>  static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
> -			   struct crypto_cipher *tfm)
> +			   struct crypto_sync_skcipher *tfm)
>  {
>    /* take the keying material, expand if necessary, truncate at 16-bytes */
>    /* run through AES counter mode to generate context->coeff[] */
>    
> -	int i,j;
> -	u32 counter;
> -	u8 *cipher, plain[16];
> -
> -	crypto_cipher_setkey(tfm, pkey, 16);
> -	counter = 0;
> -	for (i = 0; i < ARRAY_SIZE(context->coeff); ) {
> -		aes_counter[15] = (u8)(counter >> 0);
> -		aes_counter[14] = (u8)(counter >> 8);
> -		aes_counter[13] = (u8)(counter >> 16);
> -		aes_counter[12] = (u8)(counter >> 24);
> -		counter++;
> -		memcpy (plain, aes_counter, 16);
> -		crypto_cipher_encrypt_one(tfm, plain, plain);
> -		cipher = plain;
> -		for (j = 0; (j < 16) && (i < ARRAY_SIZE(context->coeff)); ) {
> -			context->coeff[i++] = ntohl(*(__be32 *)&cipher[j]);
> -			j += 4;
> -		}
> -	}
> +	SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
> +	struct scatterlist dst, src;
> +	u8 iv[AES_BLOCK_SIZE] = {};
> +	int ret;
> +
> +	crypto_sync_skcipher_setkey(tfm, pkey, 16);
> +
> +	sg_init_one(&dst, context->coeff, sizeof(context->coeff));
> +	sg_init_one(&src, page_address(ZERO_PAGE(0)), sizeof(context->coeff));

Should add:

	BUILD_BUG_ON(sizeof(context->coeff) > PAGE_SIZE);

Or alternatively, instead of using ZERO_PAGE, just memset() the buffer to zero
and encrypt it in-place.  That would be less fragile.

> +
> +	skcipher_request_set_sync_tfm(req, tfm);
> +	skcipher_request_set_callback(req, 0, NULL, NULL);
> +	skcipher_request_set_crypt(req, &src, &dst, sizeof(context->coeff), iv);
> +
> +	ret = crypto_skcipher_encrypt(req);
> +	WARN_ON_ONCE(ret);
>  }
>  
>  /* prepare for calculation of a new mic */
> @@ -2415,7 +2412,7 @@ void stop_airo_card( struct net_device *dev, int freeres )
>  				ai->shared, ai->shared_dma);
>  		}
>          }
> -	crypto_free_cipher(ai->tfm);
> +	crypto_free_sync_skcipher(ai->tfm);
>  	del_airo_dev(ai);
>  	free_netdev( dev );
>  }
> -- 
> 2.20.1
> 

Otherwise this patch looks correct to me.

The actual crypto in this driver, on the other hand, looks very outdated and
broken.  Apparently it's implementing some Cisco proprietary extension to WEP
that uses a universal hashing based MAC, where the hash key is generated from
AES-CTR.  But the MAC is only 32 bits, and the universal hash (MMH) is
implemented incorrectly: there's an off-by-one error in emmh32_final() in the
code that is supposed to be an optimized version of 'sum % ((1ULL << 32) + 15)'.

Do we know whether anyone is actually using this, or is this just another old
driver that's sitting around unused?

- Eric

Re: [PATCH] wireless: airo: switch to skcipher interface

[Ard Biesheuvel]

On Sun, 16 Jun 2019 at 09:12, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Jun 14, 2019 at 11:36:03AM +0200, Ard Biesheuvel wrote:
> > The AIRO driver applies a ctr(aes) on a buffer of considerable size
> > (2400 bytes), and instead of invoking the crypto API to handle this
> > in its entirety, it open codes the counter manipulation and invokes
> > the AES block cipher directly.
> >
> > Let's fix this, by switching to the sync skcipher API instead.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > NOTE: build tested only, since I don't have the hardware
> >
> >  drivers/net/wireless/cisco/airo.c | 57 ++++++++++----------
> >  1 file changed, 27 insertions(+), 30 deletions(-)
> >
>
> Need to also select CRYPTO_CTR in drivers/net/wireless/cisco/Kconfig under
> AIRO_CS, and I think also CRYPTO_BLKCIPHER under AIRO.
>

OK

> > diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
> > index 3f5a14112c6b..2d29ad10505b 100644
> > --- a/drivers/net/wireless/cisco/airo.c
> > +++ b/drivers/net/wireless/cisco/airo.c
> > @@ -49,6 +49,9 @@
> >  #include <linux/kthread.h>
> >  #include <linux/freezer.h>
> >
> > +#include <crypto/aes.h>
> > +#include <crypto/skcipher.h>
> > +
> >  #include <net/cfg80211.h>
> >  #include <net/iw_handler.h>
> >
> > @@ -951,7 +954,7 @@ typedef struct {
> >  } mic_statistics;
> >
> >  typedef struct {
> > -     u32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
> > +     __be32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
> >       u64 accum;      // accumulated mic, reduced to u32 in final()
> >       int position;   // current position (byte offset) in message
> >       union {
> > @@ -1216,7 +1219,7 @@ struct airo_info {
> >       struct iw_spy_data      spy_data;
> >       struct iw_public_data   wireless_data;
> >       /* MIC stuff */
> > -     struct crypto_cipher    *tfm;
> > +     struct crypto_sync_skcipher     *tfm;
> >       mic_module              mod[2];
> >       mic_statistics          micstats;
> >       HostRxDesc rxfids[MPI_MAX_FIDS]; // rx/tx/config MPI350 descriptors
> > @@ -1291,14 +1294,14 @@ static int flashrestart(struct airo_info *ai,struct net_device *dev);
> >  static int RxSeqValid (struct airo_info *ai,miccntx *context,int mcast,u32 micSeq);
> >  static void MoveWindow(miccntx *context, u32 micSeq);
> >  static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
> > -                        struct crypto_cipher *tfm);
> > +                        struct crypto_sync_skcipher *tfm);
> >  static void emmh32_init(emmh32_context *context);
> >  static void emmh32_update(emmh32_context *context, u8 *pOctets, int len);
> >  static void emmh32_final(emmh32_context *context, u8 digest[4]);
> >  static int flashpchar(struct airo_info *ai,int byte,int dwelltime);
> >
> >  static void age_mic_context(miccntx *cur, miccntx *old, u8 *key, int key_len,
> > -                         struct crypto_cipher *tfm)
> > +                         struct crypto_sync_skcipher *tfm)
> >  {
> >       /* If the current MIC context is valid and its key is the same as
> >        * the MIC register, there's nothing to do.
> > @@ -1359,7 +1362,7 @@ static int micsetup(struct airo_info *ai) {
> >       int i;
> >
> >       if (ai->tfm == NULL)
> > -             ai->tfm = crypto_alloc_cipher("aes", 0, 0);
> > +             ai->tfm = crypto_alloc_sync_skcipher("ctr(aes)", 0, 0);
> >
> >          if (IS_ERR(ai->tfm)) {
> >                  airo_print_err(ai->dev->name, "failed to load transform for AES");
> > @@ -1624,37 +1627,31 @@ static void MoveWindow(miccntx *context, u32 micSeq)
> >
> >  /* mic accumulate */
> >  #define MIC_ACCUM(val)       \
> > -     context->accum += (u64)(val) * context->coeff[coeff_position++];
> > -
> > -static unsigned char aes_counter[16];
> > +     context->accum += (u64)(val) * be32_to_cpu(context->coeff[coeff_position++]);
>
> You could alternatively call be32_to_cpu_array() after the AES encryption.
> But this works too.
>
> >
> >  /* expand the key to fill the MMH coefficient array */
> >  static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
> > -                        struct crypto_cipher *tfm)
> > +                        struct crypto_sync_skcipher *tfm)
> >  {
> >    /* take the keying material, expand if necessary, truncate at 16-bytes */
> >    /* run through AES counter mode to generate context->coeff[] */
> >
> > -     int i,j;
> > -     u32 counter;
> > -     u8 *cipher, plain[16];
> > -
> > -     crypto_cipher_setkey(tfm, pkey, 16);
> > -     counter = 0;
> > -     for (i = 0; i < ARRAY_SIZE(context->coeff); ) {
> > -             aes_counter[15] = (u8)(counter >> 0);
> > -             aes_counter[14] = (u8)(counter >> 8);
> > -             aes_counter[13] = (u8)(counter >> 16);
> > -             aes_counter[12] = (u8)(counter >> 24);
> > -             counter++;
> > -             memcpy (plain, aes_counter, 16);
> > -             crypto_cipher_encrypt_one(tfm, plain, plain);
> > -             cipher = plain;
> > -             for (j = 0; (j < 16) && (i < ARRAY_SIZE(context->coeff)); ) {
> > -                     context->coeff[i++] = ntohl(*(__be32 *)&cipher[j]);
> > -                     j += 4;
> > -             }
> > -     }
> > +     SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
> > +     struct scatterlist dst, src;
> > +     u8 iv[AES_BLOCK_SIZE] = {};
> > +     int ret;
> > +
> > +     crypto_sync_skcipher_setkey(tfm, pkey, 16);
> > +
> > +     sg_init_one(&dst, context->coeff, sizeof(context->coeff));
> > +     sg_init_one(&src, page_address(ZERO_PAGE(0)), sizeof(context->coeff));
>
> Should add:
>
>         BUILD_BUG_ON(sizeof(context->coeff) > PAGE_SIZE);
>
> Or alternatively, instead of using ZERO_PAGE, just memset() the buffer to zero
> and encrypt it in-place.  That would be less fragile.
>

I agree, I will change that.

> > +
> > +     skcipher_request_set_sync_tfm(req, tfm);
> > +     skcipher_request_set_callback(req, 0, NULL, NULL);
> > +     skcipher_request_set_crypt(req, &src, &dst, sizeof(context->coeff), iv);
> > +
> > +     ret = crypto_skcipher_encrypt(req);
> > +     WARN_ON_ONCE(ret);
> >  }
> >
> >  /* prepare for calculation of a new mic */
> > @@ -2415,7 +2412,7 @@ void stop_airo_card( struct net_device *dev, int freeres )
> >                               ai->shared, ai->shared_dma);
> >               }
> >          }
> > -     crypto_free_cipher(ai->tfm);
> > +     crypto_free_sync_skcipher(ai->tfm);
> >       del_airo_dev(ai);
> >       free_netdev( dev );
> >  }
> > --
> > 2.20.1
> >
>
> Otherwise this patch looks correct to me.
>
> The actual crypto in this driver, on the other hand, looks very outdated and
> broken.  Apparently it's implementing some Cisco proprietary extension to WEP
> that uses a universal hashing based MAC, where the hash key is generated from
> AES-CTR.  But the MAC is only 32 bits, and the universal hash (MMH) is
> implemented incorrectly: there's an off-by-one error in emmh32_final() in the
> code that is supposed to be an optimized version of 'sum % ((1ULL << 32) + 15)'.
>

I stared at that code for a bit, and I don't see the problem.

> Do we know whether anyone is actually using this, or is this just another old
> driver that's sitting around unused?
>

Excellent question. I take it this is pre-802.11b hardware, and so
even the OpenWRT people are unlikely to still be using this.

Re: [PATCH] wireless: airo: switch to skcipher interface

[Eric Biggers]

On Sun, Jun 16, 2019 at 09:03:58PM +0200, Ard Biesheuvel wrote:
> >
> > Otherwise this patch looks correct to me.
> >
> > The actual crypto in this driver, on the other hand, looks very outdated and
> > broken.  Apparently it's implementing some Cisco proprietary extension to WEP
> > that uses a universal hashing based MAC, where the hash key is generated from
> > AES-CTR.  But the MAC is only 32 bits, and the universal hash (MMH) is
> > implemented incorrectly: there's an off-by-one error in emmh32_final() in the
> > code that is supposed to be an optimized version of 'sum % ((1ULL << 32) + 15)'.
> >
> 
> I stared at that code for a bit, and I don't see the problem.
> 

I'm fairly certain that the line:

	if (utmp > 0x10000000fLL)

is supposed to be:

	if (utmp >= 0x10000000fLL)

Since it's doing mod 0x10000000f.  It's supposed to be an optimized
implementation of 'val = (u32)(context->accum % 0x10000000f)' where 0x10000000f
is the prime number 2^32 + 15.  It's meant to be the MMH algorithm: Section 3.2
of https://link.springer.com/content/pdf/10.1007/BFb0052345.pdf.  But there are
values of 'accum' where it gives the wrong result, e.g. 14137323879880455377.

Possibly this is a bug in the Cisco MIC protocol itself so can't be fixed.

> > Do we know whether anyone is actually using this, or is this just another old
> > driver that's sitting around unused?
> >
> 
> Excellent question. I take it this is pre-802.11b hardware, and so
> even the OpenWRT people are unlikely to still be using this.

- Eric

Re: [PATCH] wireless: airo: switch to skcipher interface

[Johannes Berg]

On Sun, 2019-06-16 at 00:12 -0700, Eric Biggers wrote:
> 
> The actual crypto in this driver, on the other hand, looks very outdated and
> broken.  Apparently it's implementing some Cisco proprietary extension to WEP
> that uses a universal hashing based MAC, where the hash key is generated from
> AES-CTR.  But the MAC is only 32 bits, and the universal hash (MMH) is
> implemented incorrectly: there's an off-by-one error in emmh32_final() in the
> code that is supposed to be an optimized version of 'sum % ((1ULL << 32) + 15)'.
> 
> Do we know whether anyone is actually using this, or is this just another old
> driver that's sitting around unused?

I'd guess the latter, though as recent as 2015 Ondrej (CC'ed now)
actually changed something in the driver that wasn't just cleanups ...

Remove at least this weird Cisco WEP-with-AES-key-generation mode? That
must pre-date even TKIP?

johannes

Re: [PATCH] wireless: airo: switch to skcipher interface

[Ard Biesheuvel]

On Sun, 16 Jun 2019 at 21:43, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Jun 16, 2019 at 09:03:58PM +0200, Ard Biesheuvel wrote:
> > >
> > > Otherwise this patch looks correct to me.
> > >
> > > The actual crypto in this driver, on the other hand, looks very outdated and
> > > broken.  Apparently it's implementing some Cisco proprietary extension to WEP
> > > that uses a universal hashing based MAC, where the hash key is generated from
> > > AES-CTR.  But the MAC is only 32 bits, and the universal hash (MMH) is
> > > implemented incorrectly: there's an off-by-one error in emmh32_final() in the
> > > code that is supposed to be an optimized version of 'sum % ((1ULL << 32) + 15)'.
> > >
> >
> > I stared at that code for a bit, and I don't see the problem.
> >
>
> I'm fairly certain that the line:
>
>         if (utmp > 0x10000000fLL)
>
> is supposed to be:
>
>         if (utmp >= 0x10000000fLL)
>

Ah yes, I see what you mean. 0x10000000f % 0x10000000f == 0 not 15
(after truncation). So yes, that is definitely a bug.

> Since it's doing mod 0x10000000f.  It's supposed to be an optimized
> implementation of 'val = (u32)(context->accum % 0x10000000f)' where 0x10000000f
> is the prime number 2^32 + 15.  It's meant to be the MMH algorithm: Section 3.2
> of https://link.springer.com/content/pdf/10.1007/BFb0052345.pdf.  But there are
> values of 'accum' where it gives the wrong result, e.g. 14137323879880455377.
>
> Possibly this is a bug in the Cisco MIC protocol itself so can't be fixed.
>

Highly unlikely. But also highly irrelevant :-)

> > > Do we know whether anyone is actually using this, or is this just another old
> > > driver that's sitting around unused?
> > >
> >
> > Excellent question. I take it this is pre-802.11b hardware, and so
> > even the OpenWRT people are unlikely to still be using this.
>

I'd be fine with dropping this driver. I am just trying to get rid of
(ab)use of the crypto cipher interface, so either we change it or we
drop it.

Re: [PATCH] wireless: airo: switch to skcipher interface

[Ard Biesheuvel]

On Fri, 14 Jun 2019 at 11:42, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> The AIRO driver applies a ctr(aes) on a buffer of considerable size
> (2400 bytes), and instead of invoking the crypto API to handle this
> in its entirety, it open codes the counter manipulation and invokes
> the AES block cipher directly.
>
> Let's fix this, by switching to the sync skcipher API instead.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> NOTE: build tested only, since I don't have the hardware
>


Greg, please disregard - I sent the wrong patch twice by accident (and
cc'ed you the second time)


>  drivers/net/wireless/cisco/airo.c | 57 ++++++++++----------
>  1 file changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
> index 3f5a14112c6b..2d29ad10505b 100644
> --- a/drivers/net/wireless/cisco/airo.c
> +++ b/drivers/net/wireless/cisco/airo.c
> @@ -49,6 +49,9 @@
>  #include <linux/kthread.h>
>  #include <linux/freezer.h>
>
> +#include <crypto/aes.h>
> +#include <crypto/skcipher.h>
> +
>  #include <net/cfg80211.h>
>  #include <net/iw_handler.h>
>
> @@ -951,7 +954,7 @@ typedef struct {
>  } mic_statistics;
>
>  typedef struct {
> -       u32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
> +       __be32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
>         u64 accum;      // accumulated mic, reduced to u32 in final()
>         int position;   // current position (byte offset) in message
>         union {
> @@ -1216,7 +1219,7 @@ struct airo_info {
>         struct iw_spy_data      spy_data;
>         struct iw_public_data   wireless_data;
>         /* MIC stuff */
> -       struct crypto_cipher    *tfm;
> +       struct crypto_sync_skcipher     *tfm;
>         mic_module              mod[2];
>         mic_statistics          micstats;
>         HostRxDesc rxfids[MPI_MAX_FIDS]; // rx/tx/config MPI350 descriptors
> @@ -1291,14 +1294,14 @@ static int flashrestart(struct airo_info *ai,struct net_device *dev);
>  static int RxSeqValid (struct airo_info *ai,miccntx *context,int mcast,u32 micSeq);
>  static void MoveWindow(miccntx *context, u32 micSeq);
>  static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
> -                          struct crypto_cipher *tfm);
> +                          struct crypto_sync_skcipher *tfm);
>  static void emmh32_init(emmh32_context *context);
>  static void emmh32_update(emmh32_context *context, u8 *pOctets, int len);
>  static void emmh32_final(emmh32_context *context, u8 digest[4]);
>  static int flashpchar(struct airo_info *ai,int byte,int dwelltime);
>
>  static void age_mic_context(miccntx *cur, miccntx *old, u8 *key, int key_len,
> -                           struct crypto_cipher *tfm)
> +                           struct crypto_sync_skcipher *tfm)
>  {
>         /* If the current MIC context is valid and its key is the same as
>          * the MIC register, there's nothing to do.
> @@ -1359,7 +1362,7 @@ static int micsetup(struct airo_info *ai) {
>         int i;
>
>         if (ai->tfm == NULL)
> -               ai->tfm = crypto_alloc_cipher("aes", 0, 0);
> +               ai->tfm = crypto_alloc_sync_skcipher("ctr(aes)", 0, 0);
>
>          if (IS_ERR(ai->tfm)) {
>                  airo_print_err(ai->dev->name, "failed to load transform for AES");
> @@ -1624,37 +1627,31 @@ static void MoveWindow(miccntx *context, u32 micSeq)
>
>  /* mic accumulate */
>  #define MIC_ACCUM(val) \
> -       context->accum += (u64)(val) * context->coeff[coeff_position++];
> -
> -static unsigned char aes_counter[16];
> +       context->accum += (u64)(val) * be32_to_cpu(context->coeff[coeff_position++]);
>
>  /* expand the key to fill the MMH coefficient array */
>  static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
> -                          struct crypto_cipher *tfm)
> +                          struct crypto_sync_skcipher *tfm)
>  {
>    /* take the keying material, expand if necessary, truncate at 16-bytes */
>    /* run through AES counter mode to generate context->coeff[] */
>
> -       int i,j;
> -       u32 counter;
> -       u8 *cipher, plain[16];
> -
> -       crypto_cipher_setkey(tfm, pkey, 16);
> -       counter = 0;
> -       for (i = 0; i < ARRAY_SIZE(context->coeff); ) {
> -               aes_counter[15] = (u8)(counter >> 0);
> -               aes_counter[14] = (u8)(counter >> 8);
> -               aes_counter[13] = (u8)(counter >> 16);
> -               aes_counter[12] = (u8)(counter >> 24);
> -               counter++;
> -               memcpy (plain, aes_counter, 16);
> -               crypto_cipher_encrypt_one(tfm, plain, plain);
> -               cipher = plain;
> -               for (j = 0; (j < 16) && (i < ARRAY_SIZE(context->coeff)); ) {
> -                       context->coeff[i++] = ntohl(*(__be32 *)&cipher[j]);
> -                       j += 4;
> -               }
> -       }
> +       SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
> +       struct scatterlist dst, src;
> +       u8 iv[AES_BLOCK_SIZE] = {};
> +       int ret;
> +
> +       crypto_sync_skcipher_setkey(tfm, pkey, 16);
> +
> +       sg_init_one(&dst, context->coeff, sizeof(context->coeff));
> +       sg_init_one(&src, page_address(ZERO_PAGE(0)), sizeof(context->coeff));
> +
> +       skcipher_request_set_sync_tfm(req, tfm);
> +       skcipher_request_set_callback(req, 0, NULL, NULL);
> +       skcipher_request_set_crypt(req, &src, &dst, sizeof(context->coeff), iv);
> +
> +       ret = crypto_skcipher_encrypt(req);
> +       WARN_ON_ONCE(ret);
>  }
>
>  /* prepare for calculation of a new mic */
> @@ -2415,7 +2412,7 @@ void stop_airo_card( struct net_device *dev, int freeres )
>                                 ai->shared, ai->shared_dma);
>                 }
>          }
> -       crypto_free_cipher(ai->tfm);
> +       crypto_free_sync_skcipher(ai->tfm);
>         del_airo_dev(ai);
>         free_netdev( dev );
>  }
> --
> 2.20.1
>

[PATCH] wireless: airo: switch to skcipher interface

[Ard Biesheuvel]

The AIRO driver applies a ctr(aes) on a buffer of considerable size
(2400 bytes), and instead of invoking the crypto API to handle this
in its entirety, it open codes the counter manipulation and invokes
the AES block cipher directly.

Let's fix this, by switching to the sync skcipher API instead.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
NOTE: build tested only, since I don't have the hardware

 drivers/net/wireless/cisco/airo.c | 57 ++++++++++----------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/net/wireless/cisco/airo.c b/drivers/net/wireless/cisco/airo.c
index 3f5a14112c6b..2d29ad10505b 100644
--- a/drivers/net/wireless/cisco/airo.c
+++ b/drivers/net/wireless/cisco/airo.c
@@ -49,6 +49,9 @@
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 
+#include <crypto/aes.h>
+#include <crypto/skcipher.h>
+
 #include <net/cfg80211.h>
 #include <net/iw_handler.h>
 
@@ -951,7 +954,7 @@ typedef struct {
 } mic_statistics;
 
 typedef struct {
-	u32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
+	__be32 coeff[((EMMH32_MSGLEN_MAX)+3)>>2];
 	u64 accum;	// accumulated mic, reduced to u32 in final()
 	int position;	// current position (byte offset) in message
 	union {
@@ -1216,7 +1219,7 @@ struct airo_info {
 	struct iw_spy_data	spy_data;
 	struct iw_public_data	wireless_data;
 	/* MIC stuff */
-	struct crypto_cipher	*tfm;
+	struct crypto_sync_skcipher	*tfm;
 	mic_module		mod[2];
 	mic_statistics		micstats;
 	HostRxDesc rxfids[MPI_MAX_FIDS]; // rx/tx/config MPI350 descriptors
@@ -1291,14 +1294,14 @@ static int flashrestart(struct airo_info *ai,struct net_device *dev);
 static int RxSeqValid (struct airo_info *ai,miccntx *context,int mcast,u32 micSeq);
 static void MoveWindow(miccntx *context, u32 micSeq);
 static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
-			   struct crypto_cipher *tfm);
+			   struct crypto_sync_skcipher *tfm);
 static void emmh32_init(emmh32_context *context);
 static void emmh32_update(emmh32_context *context, u8 *pOctets, int len);
 static void emmh32_final(emmh32_context *context, u8 digest[4]);
 static int flashpchar(struct airo_info *ai,int byte,int dwelltime);
 
 static void age_mic_context(miccntx *cur, miccntx *old, u8 *key, int key_len,
-			    struct crypto_cipher *tfm)
+			    struct crypto_sync_skcipher *tfm)
 {
 	/* If the current MIC context is valid and its key is the same as
 	 * the MIC register, there's nothing to do.
@@ -1359,7 +1362,7 @@ static int micsetup(struct airo_info *ai) {
 	int i;
 
 	if (ai->tfm == NULL)
-		ai->tfm = crypto_alloc_cipher("aes", 0, 0);
+		ai->tfm = crypto_alloc_sync_skcipher("ctr(aes)", 0, 0);
 
         if (IS_ERR(ai->tfm)) {
                 airo_print_err(ai->dev->name, "failed to load transform for AES");
@@ -1624,37 +1627,31 @@ static void MoveWindow(miccntx *context, u32 micSeq)
 
 /* mic accumulate */
 #define MIC_ACCUM(val)	\
-	context->accum += (u64)(val) * context->coeff[coeff_position++];
-
-static unsigned char aes_counter[16];
+	context->accum += (u64)(val) * be32_to_cpu(context->coeff[coeff_position++]);
 
 /* expand the key to fill the MMH coefficient array */
 static void emmh32_setseed(emmh32_context *context, u8 *pkey, int keylen,
-			   struct crypto_cipher *tfm)
+			   struct crypto_sync_skcipher *tfm)
 {
   /* take the keying material, expand if necessary, truncate at 16-bytes */
   /* run through AES counter mode to generate context->coeff[] */
   
-	int i,j;
-	u32 counter;
-	u8 *cipher, plain[16];
-
-	crypto_cipher_setkey(tfm, pkey, 16);
-	counter = 0;
-	for (i = 0; i < ARRAY_SIZE(context->coeff); ) {
-		aes_counter[15] = (u8)(counter >> 0);
-		aes_counter[14] = (u8)(counter >> 8);
-		aes_counter[13] = (u8)(counter >> 16);
-		aes_counter[12] = (u8)(counter >> 24);
-		counter++;
-		memcpy (plain, aes_counter, 16);
-		crypto_cipher_encrypt_one(tfm, plain, plain);
-		cipher = plain;
-		for (j = 0; (j < 16) && (i < ARRAY_SIZE(context->coeff)); ) {
-			context->coeff[i++] = ntohl(*(__be32 *)&cipher[j]);
-			j += 4;
-		}
-	}
+	SYNC_SKCIPHER_REQUEST_ON_STACK(req, tfm);
+	struct scatterlist dst, src;
+	u8 iv[AES_BLOCK_SIZE] = {};
+	int ret;
+
+	crypto_sync_skcipher_setkey(tfm, pkey, 16);
+
+	sg_init_one(&dst, context->coeff, sizeof(context->coeff));
+	sg_init_one(&src, page_address(ZERO_PAGE(0)), sizeof(context->coeff));
+
+	skcipher_request_set_sync_tfm(req, tfm);
+	skcipher_request_set_callback(req, 0, NULL, NULL);
+	skcipher_request_set_crypt(req, &src, &dst, sizeof(context->coeff), iv);
+
+	ret = crypto_skcipher_encrypt(req);
+	WARN_ON_ONCE(ret);
 }
 
 /* prepare for calculation of a new mic */
@@ -2415,7 +2412,7 @@ void stop_airo_card( struct net_device *dev, int freeres )
 				ai->shared, ai->shared_dma);
 		}
         }
-	crypto_free_cipher(ai->tfm);
+	crypto_free_sync_skcipher(ai->tfm);
 	del_airo_dev(ai);
 	free_netdev( dev );
 }
-- 
2.20.1