Re: [PATCH 1/2] wifi: mac80211: use aesgcm library

[Johannes Berg <johannes@sipsolutions.net>]

Hi Eric,

> I really appreciate the enthusiasm for the crypto library!

:)

> And it isn't
> surprising, since it's clearly the way to go.

I was kinda just playing with it, having been reminded that some code
was already ported.

> But I do think these two
> patches are jumping the gun a bit, since we haven't yet migrated all the
> optimized AES-GCM code into the library, or added an improved AES-GCM
> API that provides enough functionality to fulfill all the in-kernel use
> cases (for example, incremental computation of AES-GMAC).

> So as-is these two patches could regress performance in some cases
> (despite the library having less overhead).

Fair. I don't think the performance matters all that much (though I
shouldn't lie about it in the commit log) since this is mostly used for
testing - I don't think there are many users of software crypto beyond
that. Some, for sure, but I believe those are all old drivers that will
get you a maximum of ~25 Mbps throughput (both directions combined) if
you're lucky :)

Anyway, I don't really disagree either, none of this is urgent or
important at this point.

Also, there's a separate conversation to be had here - I was looking and
we also instantiate ccm(aes) and ctr(aes) in mac80211, and I didn't find
equivalent library calls for those.

> And also the AES-GCM API is
> likely to change a bit.  In particular I don't think code outside the
> crypto subsystem should be constructing its own AES-GMAC by combining
> the GHASH functions with the AES functions, as your second patch does.
> Instead they should invoke an AES-GMAC API (or AES-GCM, of which
> AES-GMAC is a special case) provided by lib/crypto/.

I _was_ thinking that could be better ... and forgot that GMAC is just a
GCM special case, despite obviously constructing it by hand. Oops.

I actually thought about exporting aesgcm_mac(), but of course that'd
basically be equivalent to just using aesgcm_encrypt() without data.

However, both of them can only use a single buffer for the associated
data, so they can't be used here. The crypto API used sg tables which
aren't great either, but definitely more flexible than the current
function. Note that in this case I actually need to use three or four
AAD buffers:

 - the pseudo-header constructed outside the frame buffer specifically
   for WiFi, representing the frame header but not exactly the same
 - for beacons an 8-byte zero buffer representing the Timestamp
 - the frame payload without the MIC
   (and without the Timestamp for beacons)
 - a 16-byte zero buffer representing the MIC

This would require a more specific GMAC API like the CMAC API, or,
equivalently but more flexible, an init/aad_update/data_update/final GCM
API. Could even have

  aes_gcm_init()
  aes_gcm_update_aad()
  aes_gcm_update_data()
  aes_gcm_final()

and
  #define aes_gmac_init aes_gcm_init
  #define aes_gmac_update aes_gmac_update_aad
  #define aes_gmac_final aes_gcm_final

or something like that, I guess.

> So I'd ask that we wait just a bit until I can finish getting the
> AES-GCM library APIs into a good state.  I got a lot of the prerequisite
> work in for 7.0 and 7.1, and I'll see if I can finish it in 7.2.  I've
> just been a bit busy with other things in the past few weeks.

Sure, no hurry, was mostly playing with how that'd look like. Maybe it
even helps figure out the right APIs ;-)

Thanks,
johannes