I'm working on IEEE 802.11w support for mac80211 and my current code is
in a state that allows both unicast and multicast/broadcast management
frames to be protected (with CCMP and BIP/AES-CMAC, respectively). I'll
clean up the patches a bit and send the first version for commenting in
the near future. However, even before the patches are available, there
is one area that could benefit from comments.
hostapd is currently using a monitor interface to inject management
frames. This seems to work fine for unencrypted frames, but at least BIP
protection of broadcast/multicast management frames does not work
without some hacks.. The problem here is that the default TX key is
configured for the main data interface (wlan0) while the injected frame
is coming from the monitor interface (mon.wlan0).
ieee80211_tx_h_select_key() does not find the default key for management
frames and consequently, the frame ends up going out without BIP
processing.
My current workaround is to configure IGTK for both wlan0 and mon.wlan0
in hostapd. This works, but is somewhat undesirable.. Would there be a
better way for configuring the default keys (this could apply also for
data frames, but at least for IGTK and management frames for the time
being), so that they would apply both to frames generated by MLME code
in mac80211 (if any; currently there is no broadcast/multicast
management frames used and actually, no management frames generated in
mac80211 MLME code in AP mode) and monitor interfaces (or something that
would replace them for packet injection)?
PS.
I'm not sure whether this is a bug somewhere in my changes or whether
the configuration of default keys for monitor interfaces triggered this,
but mac80211 is now leaving behind almost empty debugfs ieee80211/phy#
directories. The only thing remaining in that directory is an empty
netdev:mon.wlan0 subdirectory, so it looks like something fails to
remove said subdirectory and then the phy# directory.
--
Jouni Malinen PGP id EFC895FA
> > Yes, that sounds likely since the changes I did for debugfs were very
> > trivial copies from CCMP/data-default-key processing. I'll debug this
> > more and try to figure if there is need to re-order something or make
> > the debugfs entry removal able to handle such a case.
>
> Ok. I don't know right now, and it does seem to work correctly here, but
> maybe it doesn't when the application doesn't explicitly remove the key
> or something, I'll take a look.
It's definitely done, from ieee80211_free_keys() calling
ieee80211_debugfs_key_remove_default(), maybe you missed something
there?
johannes
> I did.. I added another set of functions for default management key and
> did not remember to call the removal function from
> ieee80211_free_keys(). However, adding that call did not change
> anything.
Ok.
> It looks like we end up trying to remove the netdev directory in debugfs
> before removing the default key symlinks. Consequently, debugfs_remove()
> fails since there is still a file in the directory. This is what happens
> when removing the monitor interface:
>
> cfg.c: ieee80211_del_iface() ->
> iface.c: ieee80211_if_remove() ->
> iface.c: __ieee80211_if_del() ->
> debugfs_netdev.c: ieee80211_debugfs_remove_netdev()
> [too early; symlink still there]
> unregister_netdevice(dev) -> [dev->uninit]
> iface.c: ieee80211_if_reinit() ->
> key.c: ieee80211_free_keys() ->
> debugfs_key.c: ieee80211_debugfs_key_remove_default()
>
>
> Any idea how to fix this?
Hmm, no, no idea at all. To be honest, I don't fully understand
interface lifetime rules and I think many of those functions are
probably misnamed. I.e. why is the _reinit function assigned to the
->uninit callback? Much of that either comes from the original
Devicescape code or what Jiri did to it, and I never bothered to clean
it up.
> Why is ieee80211_debugfs_remove_netdev() call
> in __ieee80211_if_del()?
I don't really know. Probably because that's where Jiri had the sysfs
code I converted to debugfs ;)
> Could it be moved into ieee80211_if_reinit(),
> so that it would happen only after the ieee80211_free_keys() call?
That's well possible, I guess you'd have to try.
> Since
> ieee80211_if_reinit() is called from other places, too, it might be
> cleaner to define a new dev->uninit function that is a wrapper for call
> to ieee80211_if_reinit() followed by call to
> ieee80211_debugfs_remove_netdev().. However, since ieee80211_if_reinit()
> calls ieee80211_if_sdata_deinit(), it might be necessary to call
> ieee80211_debugfs_remove_netdev() before this call (or from it?); I did
> not yet look into details of what would be the required order for these.
I really don't know either. If you feel motivated to clean up the
interface handling I'd appreciate it.
johannes
On Mon, Jun 16, 2008 at 06:47:48PM +0200, Johannes Berg wrote:
> > > Yes, that sounds likely since the changes I did for debugfs were very
> > > trivial copies from CCMP/data-default-key processing. I'll debug this
> > > more and try to figure if there is need to re-order something or make
> > > the debugfs entry removal able to handle such a case.
> >
> > Ok. I don't know right now, and it does seem to work correctly here, but
> > maybe it doesn't when the application doesn't explicitly remove the key
> > or something, I'll take a look.
>
> It's definitely done, from ieee80211_free_keys() calling
> ieee80211_debugfs_key_remove_default(), maybe you missed something
> there?
I did.. I added another set of functions for default management key and
did not remember to call the removal function from
ieee80211_free_keys(). However, adding that call did not change
anything.
It looks like we end up trying to remove the netdev directory in debugfs
before removing the default key symlinks. Consequently, debugfs_remove()
fails since there is still a file in the directory. This is what happens
when removing the monitor interface:
cfg.c: ieee80211_del_iface() ->
iface.c: ieee80211_if_remove() ->
iface.c: __ieee80211_if_del() ->
debugfs_netdev.c: ieee80211_debugfs_remove_netdev()
[too early; symlink still there]
unregister_netdevice(dev) -> [dev->uninit]
iface.c: ieee80211_if_reinit() ->
key.c: ieee80211_free_keys() ->
debugfs_key.c: ieee80211_debugfs_key_remove_default()
Any idea how to fix this? Why is ieee80211_debugfs_remove_netdev() call
in __ieee80211_if_del()? Could it be moved into ieee80211_if_reinit(),
so that it would happen only after the ieee80211_free_keys() call? Since
ieee80211_if_reinit() is called from other places, too, it might be
cleaner to define a new dev->uninit function that is a wrapper for call
to ieee80211_if_reinit() followed by call to
ieee80211_debugfs_remove_netdev().. However, since ieee80211_if_reinit()
calls ieee80211_if_sdata_deinit(), it might be necessary to call
ieee80211_debugfs_remove_netdev() before this call (or from it?); I did
not yet look into details of what would be the required order for these.
--
Jouni Malinen PGP id EFC895FA
> Yes, as long as the STA entry is found (and it should, in this case),
> the pairwise CCMP key is used both for data and management frames. I
> haven't done full testing yet, but I would expect this to work even when
> the unicast management frames comes from hostapd.
Yeah, it should.
> Broadcast/multicast management frames are protected with BIP using IGTK.
> This is similar to how GTK is used with TKIP/CCMK for broadcast data
> frames, i.e., there is a default TX key index that the AP uses for the
> frames. Indexes 1..3 are used for data frames and 802.11w is using 4..5
> for BIP (even though the key index space is actually completely separate
> from the one used with data frames).
Ok, thanks for the explanation.
> > Yeah that sounds like a hack. I guess it should work just like when we
> > submit a unicast frame via monitor with the radiotap flag to indicate
> > that we want encryption, only we should add logic to look up the default
> > key to use if there is no peer/unicast key by the outgoing MAC address?
>
> Yes, we already look for STA entry for unicast and that should work as
> long as the STA table is not per netdev, i.e., both data and monitor
> interface end using the same STA table. If no STA entry is found
> (multicast/broadcast), we look for a default TX key, but this is only
> done for the netdev that was used to send the frame (which is different
> between normal data interface and packet injection via monitor
> interface).
Good point about the STA tables there. I guess we really do need to bind
the frame to a certain outgoing device to use that sdata state struct.
> If the monitor interface were to look for the default key (or well,
> keys, since there are now two; one for data, one for mgmt) from the data
> interface (somehow bound to the monitor iface?), that should work here.
>
> > OTOH, that would break for multi-SSID/single-BSSID scenarios I guess, so
> > we probably need a way to indicate "this frame belongs to interface
> > index N"?
>
> Yes, either the frame or maybe more easily the monitor interface would
> need to be bound to the interface that is used for key configuration.
Yeah, either would work, for per-frame we'd have to extend radiotap but
it would probably be better as it would allow hostapd to use the same
mon.wlan0 interface for all BSSes.
> > I don't see any such problems, but if I were to venture a guess it's
> > because of configuring keys on a monitor interface. It sounds like
> > something sticks around within the netdev:mon.wlan0 subdirectory, then
> > the code tries to delete the directory and only afterwards is the entry
> > removed, leaving the directory (and the parent, of course) hanging
> > there.
>
> Yes, that sounds likely since the changes I did for debugfs were very
> trivial copies from CCMP/data-default-key processing. I'll debug this
> more and try to figure if there is need to re-order something or make
> the debugfs entry removal able to handle such a case.
Ok. I don't know right now, and it does seem to work correctly here, but
maybe it doesn't when the application doesn't explicitly remove the key
or something, I'll take a look.
johannes
On Mon, Jun 16, 2008 at 04:53:11PM +0200, Johannes Berg wrote:
> So just for me, BIP processing is done with the same IGTK? Unicast
> frames are handled by the per-station key, and as such work correctly,
> I'd guess?
Yes, as long as the STA entry is found (and it should, in this case),
the pairwise CCMP key is used both for data and management frames. I
haven't done full testing yet, but I would expect this to work even when
the unicast management frames comes from hostapd.
Broadcast/multicast management frames are protected with BIP using IGTK.
This is similar to how GTK is used with TKIP/CCMK for broadcast data
frames, i.e., there is a default TX key index that the AP uses for the
frames. Indexes 1..3 are used for data frames and 802.11w is using 4..5
for BIP (even though the key index space is actually completely separate
from the one used with data frames).
> Yeah that sounds like a hack. I guess it should work just like when we
> submit a unicast frame via monitor with the radiotap flag to indicate
> that we want encryption, only we should add logic to look up the default
> key to use if there is no peer/unicast key by the outgoing MAC address?
Yes, we already look for STA entry for unicast and that should work as
long as the STA table is not per netdev, i.e., both data and monitor
interface end using the same STA table. If no STA entry is found
(multicast/broadcast), we look for a default TX key, but this is only
done for the netdev that was used to send the frame (which is different
between normal data interface and packet injection via monitor
interface).
If the monitor interface were to look for the default key (or well,
keys, since there are now two; one for data, one for mgmt) from the data
interface (somehow bound to the monitor iface?), that should work here.
> OTOH, that would break for multi-SSID/single-BSSID scenarios I guess, so
> we probably need a way to indicate "this frame belongs to interface
> index N"?
Yes, either the frame or maybe more easily the monitor interface would
need to be bound to the interface that is used for key configuration.
> I don't see any such problems, but if I were to venture a guess it's
> because of configuring keys on a monitor interface. It sounds like
> something sticks around within the netdev:mon.wlan0 subdirectory, then
> the code tries to delete the directory and only afterwards is the entry
> removed, leaving the directory (and the parent, of course) hanging
> there.
Yes, that sounds likely since the changes I did for debugfs were very
trivial copies from CCMP/data-default-key processing. I'll debug this
more and try to figure if there is need to re-order something or make
the debugfs entry removal able to handle such a case.
--
Jouni Malinen PGP id EFC895FA
On Mon, 2008-06-16 at 17:33 +0300, Jouni Malinen wrote:
> I'm working on IEEE 802.11w support for mac80211 and my current code is
> in a state that allows both unicast and multicast/broadcast management
> frames to be protected (with CCMP and BIP/AES-CMAC, respectively). I'll
> clean up the patches a bit and send the first version for commenting in
> the near future. However, even before the patches are available, there
> is one area that could benefit from comments.
Neat.
> hostapd is currently using a monitor interface to inject management
> frames. This seems to work fine for unencrypted frames, but at least BIP
> protection of broadcast/multicast management frames does not work
> without some hacks.. The problem here is that the default TX key is
> configured for the main data interface (wlan0) while the injected frame
> is coming from the monitor interface (mon.wlan0).
> ieee80211_tx_h_select_key() does not find the default key for management
> frames and consequently, the frame ends up going out without BIP
> processing.
So just for me, BIP processing is done with the same IGTK? Unicast
frames are handled by the per-station key, and as such work correctly,
I'd guess?
> My current workaround is to configure IGTK for both wlan0 and mon.wlan0
> in hostapd. This works, but is somewhat undesirable.. Would there be a
> better way for configuring the default keys (this could apply also for
> data frames, but at least for IGTK and management frames for the time
> being), so that they would apply both to frames generated by MLME code
> in mac80211 (if any; currently there is no broadcast/multicast
> management frames used and actually, no management frames generated in
> mac80211 MLME code in AP mode) and monitor interfaces (or something that
> would replace them for packet injection)?
Yeah that sounds like a hack. I guess it should work just like when we
submit a unicast frame via monitor with the radiotap flag to indicate
that we want encryption, only we should add logic to look up the default
key to use if there is no peer/unicast key by the outgoing MAC address?
OTOH, that would break for multi-SSID/single-BSSID scenarios I guess, so
we probably need a way to indicate "this frame belongs to interface
index N"?
> I'm not sure whether this is a bug somewhere in my changes or whether
> the configuration of default keys for monitor interfaces triggered this,
> but mac80211 is now leaving behind almost empty debugfs ieee80211/phy#
> directories. The only thing remaining in that directory is an empty
> netdev:mon.wlan0 subdirectory, so it looks like something fails to
> remove said subdirectory and then the phy# directory.
I don't see any such problems, but if I were to venture a guess it's
because of configuring keys on a monitor interface. It sounds like
something sticks around within the netdev:mon.wlan0 subdirectory, then
the code tries to delete the directory and only afterwards is the entry
removed, leaving the directory (and the parent, of course) hanging
there.
johannes