Page MenuHomeFreeBSD

net80211: migrate the ioctl API to a 128 bit specific API + use key API
ClosedPublic

Authored by adrian on Tue, Jul 21, 7:57 PM.
Referenced Files
Unknown Object (File)
Mon, Aug 17, 6:06 AM
Unknown Object (File)
Sun, Aug 16, 4:19 PM
Unknown Object (File)
Sun, Aug 16, 11:22 AM
Unknown Object (File)
Sat, Aug 15, 5:25 PM
Unknown Object (File)
Sat, Aug 15, 1:22 PM
Unknown Object (File)
Sat, Aug 15, 10:07 AM
Unknown Object (File)
Fri, Aug 14, 9:48 PM
Unknown Object (File)
Thu, Aug 13, 3:48 PM

Details

Summary
  • Begin migrating the ioctl code to use the key management APIs. Not all of it has been migrated (notably the WEP API hasn't.)
  • Take special care to copy the TKIP MIC in and out correctly.
  • Note that some of the defines used as sizes are actually the ioctl sizes, they'll need to be fixed before I push this into a review.
  • Document this current API as a specific 128 bit key + 128 bit TKIP MIC API.

The goal here is to solidify this stuff as the 128 bit ioctl API
and not change it, even if net80211 will eventually grow 256 and
384 bit key support.

Notably the TKIP stuff - the driver_bsd.c code puts the TKIP after
the normal key contents, whereas the net80211 code puts the TKIP
stuff in the /end/ of the key buffer. They happen to be equivalent
when ioctl key buffer size == net80211 key buffer size, but as I learnt
the last couple times I tried this, they're not always going to be
equivalent.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 75617
Build 72500: arc lint + arc unit

Event Timeline

bz requested changes to this revision.Wed, Jul 22, 7:54 AM
bz added a subscriber: bz.
bz added inline comments.
sys/net80211/ieee80211_crypto.h
323 ↗(On Diff #182416)

len is unsigned; size_t?

341 ↗(On Diff #182416)

again

358 ↗(On Diff #182416)

and again

sys/net80211/ieee80211_ioctl.c
113

Please do.

118

Please do. In case of error make sure no key data is left behind in the buffer.

120

Same here.

852

Sorry but no sorry. Ignoring actual lengths the logic says: if my key is longer than my buffer than copy a partial key, which is an error.
Given this cannot happen for WEPKEY why not simply add an assert to document this rather than trying to pretend logic will handle it (but would so wrongly)?

866

Well, then do so.

1264–1265

key length is unsigned.

1317

indent?

1323

Which is also a dangerous thing. One should check somewhere up there (if not done), that ik_keylen is sane; doesn't help is someone supplies -1 == 255 (current max given uint8_t) but the buffer is only 128 long. Way worse once this gets expanded for larger key sizes and -1 will be 64k at least.

1346

We usually call these cases "error".

This revision now requires changes to proceed.Wed, Jul 22, 7:54 AM
adrian marked 3 inline comments as done.

another update

adrian added inline comments.
sys/net80211/ieee80211_ioctl.c
113

I'll fix it in a follow-up commit; the existing behaviour does the same mistakes.

852

I'll tackle it later; I don't want it to just be an assert as it'll then get ignored in non-debug kernels.

1346

it's not a separate code path for error though.

update the wep side size check, bz@

update to use the copy out routines

@bz ok this should implement the behaviours you wanted - bounds checking, buffer zero'ing, etc. Please let me know.

This currently throws errors in ifconfig if there's no key (as the key length is 0) - which means it'll error out unless four WEP keys are configured.
I'll need to go and handle the "no key configured, return a blank no key" case before this lands.

change to the new key copy API that returns true/false, not key copy length.

ok, i think this is correct. I'm bounds checking copy in and copy out of the keys / ioctl buffers; i'm handling 0 byte empty keys properly again so ifconfig and such doesn't error out. I've tested it with CCMP, TKIP and WEP (and I'm sure if CCMP works then GCMP will work, but I'll try to validate that soon.)

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Aug 19, 2:07 AM
This revision was automatically updated to reflect the committed changes.