Re: [Patch v9 05/16] Introduce and add key_flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 06.01.20 um 10:17 schrieb Jouni Malinen:
On Sat, Jan 04, 2020 at 11:10:04PM +0100, Alexander Wetzel wrote:
+enum key_flag {
+	KEY_FLAG_MODIFY			= BIT(0),
+	KEY_FLAG_DEFAULT		= BIT(1),
+	KEY_FLAG_RX			= BIT(2),
+	KEY_FLAG_TX			= BIT(3),
+	KEY_FLAG_GROUP			= BIT(4),
+	KEY_FLAG_PAIRWISE		= BIT(5),
+	KEY_FLAG_PMK			= BIT(6),

How much of this is really needed for new functionality vs. cleaning up?
I'm not necessarily against these flags, but I'll note that the addr
parameter is already documented to indicate broadcast vs. default vs.
unicast keys.


I believe they are. And also that the documentation of addr should be changed then, too:-)

The MODIFY Flag is not needed till we merge Extended Key ID (nobody is changing a TX status so far). The PMK flag could also be called something like "OTHER" or we also could just use "0" for them. Just looking at the addr to figure out if we have a default key is missing the (existing) special case of IBSS GTK keys. (Search for "RSN IBSS RX GTK" in the driver) and look at supp_set_key() in ibss_rsn.c which has this code block:
        if (is_broadcast_ether_addr(addr))
                addr = peer->addr;
        return wpa_drv_set_key(peer->ibss_rsn->wpa_s, alg, addr, key_idx,
                               set_tx, seq, seq_len, key, key_len);

There also is the issue of the dual-usage of keyid 1 with Extended Key ID when we delete a key. We have to figure out if the pairwise or the group key must be deleted. (But just making sure pairwise keys are flagged accordingly would sove that, it just would be a bit less obvious.)

That said we could achieve something similar with a slight update to the key_flag API proposed in V8. But I really liked how the API proposed here did clean up the special case handling in nl80211 and making that much simpler.

diff --git a/src/drivers/driver.h b/src/drivers/driver.h
@@ -2359,7 +2400,7 @@ struct wpa_driver_ops {
  	int (*set_key)(const char *ifname, void *priv, enum wpa_alg alg,
  		       const u8 *addr, int key_idx, int set_tx,
  		       const u8 *seq, size_t seq_len,
-		       const u8 *key, size_t key_len);
+		       const u8 *key, size_t key_len, enum key_flag key_flag);

Instead of doing these set_key() prototype changes in driver.h and all
driver_*.c in this patch 5/16 and then repeat that in 9/16, maybe it
would make more sense to have a new patch before 5/16 that converts
set_key() to take in a struct of parameter values similarly to
associate(). In other words, only take in void *priv and struct
wpa_driver_key_params *params. This change would only touch
src/drivers/* and the set_key() wrapper functions in
wpa_supplicant/driver_i.h and src/ap/ap_drv_ops.c. Addition of a new
key_flag into the struct could then be done without having to touch any
driver_*.c and similarly, removal of set_tx would be trivial once there
are no remaining users for it.

Ok, V10 of the series (or the three replacement series for it) will make that change. (Will probably at least some days till I get to that.)

Alexander

_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux