On Sun, 16 Jun 2019 at 09:12, Eric Biggers <ebiggers@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
