Re: [PATCH] lib80211: use crypto API ccm(aes) transform for CCMP processing

[Eric Biggers <ebiggers@kernel.org>]

On Sun, Jun 16, 2019 at 09:07:28PM +0200, Ard Biesheuvel wrote:
> On Sun, 16 Jun 2019 at 21:01, Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > Hi Ard,
> >
> > On Fri, Jun 14, 2019 at 11:29:22AM +0200, Ard Biesheuvel wrote:
> > > -static void ccmp_init_blocks(struct crypto_cipher *tfm,
> > > -                          struct ieee80211_hdr *hdr,
> > > -                          u8 * pn, size_t dlen, u8 * b0, u8 * auth, u8 * s0)
> > > +static void ccmp_init_blocks(struct ieee80211_hdr *hdr,
> > > +                          u8 * pn, size_t dlen, u8 * b0, u8 * aad)
> > >  {
> > >       u8 *pos, qc = 0;
> > >       size_t aad_len;
> > >       int a4_included, qc_included;
> > > -     u8 aad[2 * AES_BLOCK_LEN];
> > >
> > >       a4_included = ieee80211_has_a4(hdr->frame_control);
> > >       qc_included = ieee80211_is_data_qos(hdr->frame_control);
> > > @@ -131,17 +123,19 @@ static void ccmp_init_blocks(struct crypto_cipher *tfm,
> > >               aad_len += 2;
> > >       }
> > >
> > > -     /* CCM Initial Block:
> > > -      * Flag (Include authentication header, M=3 (8-octet MIC),
> > > -      *       L=1 (2-octet Dlen))
> > > -      * Nonce: 0x00 | A2 | PN
> > > -      * Dlen */
> > > -     b0[0] = 0x59;
> > > +     /* In CCM, the initial vectors (IV) used for CTR mode encryption and CBC
> > > +      * mode authentication are not allowed to collide, yet both are derived
> > > +      * from this vector b0. We only set L := 1 here to indicate that the
> > > +      * data size can be represented in (L+1) bytes. The CCM layer will take
> > > +      * care of storing the data length in the top (L+1) bytes and setting
> > > +      * and clearing the other bits as is required to derive the two IVs.
> > > +      */
> > > +     b0[0] = 0x1;
> > > +
> > > +     /* Nonce: QC | A2 | PN */
> > >       b0[1] = qc;
> > >       memcpy(b0 + 2, hdr->addr2, ETH_ALEN);
> > >       memcpy(b0 + 8, pn, CCMP_PN_LEN);
> > > -     b0[14] = (dlen >> 8) & 0xff;
> > > -     b0[15] = dlen & 0xff;
> > >
> > >       /* AAD:
> > >        * FC with bits 4..6 and 11..13 masked to zero; 14 is always one
> > > @@ -166,16 +160,6 @@ static void ccmp_init_blocks(struct crypto_cipher *tfm,
> > >               aad[a4_included ? 30 : 24] = qc;
> > >               /* rest of QC masked */
> > >       }
> > > -
> > > -     /* Start with the first block and AAD */
> > > -     lib80211_ccmp_aes_encrypt(tfm, b0, auth);
> > > -     xor_block(auth, aad, AES_BLOCK_LEN);
> > > -     lib80211_ccmp_aes_encrypt(tfm, auth, auth);
> > > -     xor_block(auth, &aad[AES_BLOCK_LEN], AES_BLOCK_LEN);
> > > -     lib80211_ccmp_aes_encrypt(tfm, auth, auth);
> > > -     b0[0] &= 0x07;
> > > -     b0[14] = b0[15] = 0;
> > > -     lib80211_ccmp_aes_encrypt(tfm, b0, s0);
> > >  }
> >
> > How about shifting the contents of aad over by 2 bytes and returning the AAD
> > length from this function instead?  It's confusing to still manually format the
> > AAD length for CCM mode, when actually it's ignored now.
> >
> > Also I suggest fixing up the naming:
> >
> >         ccmp_init_blocks() => ccmp_init_iv_and_aad()
> >         b0 => iv
> >
> > >
> > >  static int lib80211_ccmp_hdr(struct sk_buff *skb, int hdr_len,
> > > @@ -218,13 +202,13 @@ static int lib80211_ccmp_hdr(struct sk_buff *skb, int hdr_len,
> > >  static int lib80211_ccmp_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
> > >  {
> > >       struct lib80211_ccmp_data *key = priv;
> > > -     int data_len, i, blocks, last, len;
> > > -     u8 *pos, *mic;
> > >       struct ieee80211_hdr *hdr;
> > > -     u8 *b0 = key->tx_b0;
> > > -     u8 *b = key->tx_b;
> > > -     u8 *e = key->tx_e;
> > > -     u8 *s0 = key->tx_s0;
> > > +     struct aead_request *req;
> > > +     struct scatterlist sg[2];
> > > +     u8 *aad = key->tx_aad;
> > > +     u8 b0[AES_BLOCK_LEN];
> > > +     int len, data_len;
> > > +     int ret;
> > >
> > >       if (skb_tailroom(skb) < CCMP_MIC_LEN || skb->len < hdr_len)
> > >               return -1;
> > > @@ -234,31 +218,29 @@ static int lib80211_ccmp_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
> > >       if (len < 0)
> > >               return -1;
> > >
> > > -     pos = skb->data + hdr_len + CCMP_HDR_LEN;
> > > +     req = kzalloc(sizeof(*req) + crypto_aead_reqsize(key->tfm), GFP_ATOMIC);
> > > +     if (!req)
> > > +             return -ENOMEM;
> > > +
> >
> > Why not kzalloc() and kzfree() instead of aead_request_alloc() and
> > aead_request_free()?  Same in lib80211_ccmp_decrypt().
> >
> 
> Why kzalloc, right? (i.e., without the 'not'). Good question, I'll change that.
> 

Yes ignore the "not", sorry.

- Eric