| Message ID | 20231016145213.2973e3c8d3bb.I6198b8d3b04cf4a97b06660d346caec3032f232a@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: don't drop all unprotected public action frames | expand |
On Mon, Oct 16, 2023 at 02:52:48PM +0300, gregory.greenman@intel.com wrote: > Not all public action frames have a protected variant. When MFP is > enabled drop only public action frames that have a dual protected > variant. That description sounds accurate.. > diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h > +/** > + * ieee80211_is_protected_dual_of_public_action - check if skb contains a > + * protected dual of public action management frame > + * @skb: the skb containing the frame, length will be checked > + * > + * Return: true if the skb contains a protected dual of public action > + * management frame, false otherwise. > + */ But this comment and the function name feel quite misleading. This does not return true if the skb contains a Protected Dual of Public Action frame; this returns true if the skb contains a Public Action frame for which a Protected Dual of Public Action frame is defined. Or well, that is what this function should do for the mac80211 change to work correctly, but it does not really do that.. > +static inline bool > +ieee80211_is_protected_dual_of_public_action(struct sk_buff *skb) > +{ > + u8 action; > + > + if (!ieee80211_is_public_action((void *)skb->data, skb->len) || > + skb->len < IEEE80211_MIN_ACTION_SIZE + 1) > + return false; > + > + action = *(u8 *)(skb->data + IEEE80211_MIN_ACTION_SIZE); > + > + return action != WLAN_PUB_ACTION_20_40_BSS_COEX && > + action != WLAN_PUB_ACTION_DSE_REG_LOC_ANN && > + action != WLAN_PUB_ACTION_MSMT_PILOT && > + action != WLAN_PUB_ACTION_TDLS_DISCOVER_RES && > + action != WLAN_PUB_ACTION_LOC_TRACK_NOTI && > + action != WLAN_PUB_ACTION_FTM_REQUEST && > + action != WLAN_PUB_ACTION_FTM_RESPONSE && > + action != WLAN_PUB_ACTION_FILS_DISCOVERY; > +} What is this list of Public Action frames based on? The "Reserved" rows of the Protected Dual of Public Action frames from some snapshot of the IEEE 802.11 standard? That is neither robust nor correct way of doing this. It would be more robust (in a sense of not breaking things in future) to make this match against cases for which there is a known protected variant instead of assuming that there is a protected variant for everything that is known to not have one yet defined. Furthermore, this is completely wrong for Vendor Specific Public Action frames. There is a Protected Vendor Specific value for Protected Dual of Public Action frame, but that value is used on case by case basis for each different type of vendor specific frame. In other words, this part would need to look at the OUI:subtype combination to search which vendor specific cases have a protected variant. I'd expect there to be a very limited, if any, such cases, i.e., more or less all vendor specific Public Action frames should be allowed to be processed in mac80211 even when MFP has been negotiated for an association. In practice, this patch (well, a commit in wireless-next.git now) leaves Vendor Specific Public Action frame cases broken. For example, DPP does not work correctly with this. hostap.git test case dpp_conn_status_success_hostapd_configurator can demonstrate that issue. In addition, this would break more recently added Public Action frames with Action field values larger than 34 broken. There are quite a few such frames defined and none of them seem to have a matching protected dual.
On Sat, 2023-12-02 at 13:28 +0200, Jouni Malinen wrote: > > But this comment and the function name feel quite misleading. This does > not return true if the skb contains a Protected Dual of Public Action > frame; this returns true if the skb contains a Public Action frame for > which a Protected Dual of Public Action frame is defined. Or well, that > is what this function should do for the mac80211 change to work > correctly, but it does not really do that.. > > > +static inline bool > > +ieee80211_is_protected_dual_of_public_action(struct sk_buff *skb) > > +{ > > + u8 action; > > + > > + if (!ieee80211_is_public_action((void *)skb->data, skb->len) || > > + skb->len < IEEE80211_MIN_ACTION_SIZE + 1) > > + return false; > > + > > + action = *(u8 *)(skb->data + IEEE80211_MIN_ACTION_SIZE); > > + > > + return action != WLAN_PUB_ACTION_20_40_BSS_COEX && > > + action != WLAN_PUB_ACTION_DSE_REG_LOC_ANN && > > + action != WLAN_PUB_ACTION_MSMT_PILOT && > > + action != WLAN_PUB_ACTION_TDLS_DISCOVER_RES && > > + action != WLAN_PUB_ACTION_LOC_TRACK_NOTI && > > + action != WLAN_PUB_ACTION_FTM_REQUEST && > > + action != WLAN_PUB_ACTION_FTM_RESPONSE && > > + action != WLAN_PUB_ACTION_FILS_DISCOVERY; > > +} > > What is this list of Public Action frames based on? The "Reserved" rows > of the Protected Dual of Public Action frames from some snapshot of the > IEEE 802.11 standard? I guess? We actually went back and forth on this a bit, Avi wrote it to be similar to _ieee80211_is_robust_mgmt_frame() which has a list of "known not to be robust". > That is neither robust nor correct way of doing this. > It would be more robust (in a sense of not breaking things in > future) to make this match against cases for which there is a known > protected variant instead of assuming that there is a protected variant > for everything that is known to not have one yet defined. Right, it would work both ways. But then the argument was that we've basically done the same for _ieee80211_is_robust_mgmt_frame() and decided that we'd rather treat them as robust if we don't know? Maybe this case is more likely to define something new that's not, since it's about public action frames? OTOH, it's OK anyway if they're not exchanged within an existing connection. > Furthermore, this is completely wrong for Vendor Specific Public Action > frames. There is a Protected Vendor Specific value for Protected Dual of > Public Action frame, but that value is used on case by case basis for > each different type of vendor specific frame. In other words, this part > would need to look at the OUI:subtype combination to search which vendor > specific cases have a protected variant. I'd expect there to be a very > limited, if any, such cases, i.e., more or less all vendor specific > Public Action frames should be allowed to be processed in mac80211 even > when MFP has been negotiated for an association. Agree, that'd be up to the implementation to figure out whether it wants that or not. The kernel doesn't process them anyway, and should hopefully give you enough information out to userspace to see if they were protected or not. So certainly adding WLAN_PUB_ACTION_VENDOR_SPECIFIC here makes sense. > In practice, this patch (well, a commit in wireless-next.git now) leaves > Vendor Specific Public Action frame cases broken. For example, DPP does > not work correctly with this. hostap.git test case > dpp_conn_status_success_hostapd_configurator can demonstrate that issue. Which would fix that. > In addition, this would break more recently added Public Action frames > with Action field values larger than 34 broken. There are quite a few > such frames defined and none of them seem to have a matching protected > dual. DCS is for (E?)DMG which ... we don't support well, let's say? The next ones also don't seem like we'd support them (don't even know where 1.08 GHz channels are used.) Agree that On-channel Tunnel request should be included here. But this similarly applies to _ieee80211_is_robust_mgmt_frame(), and we haven't even added HE to that list yet. So it's always going to be a case where the list is necessarily wrong in the face of future enhancements. Perhaps the kernel should not care about it for frames that it doesn't know about (and will therefore not process), but then arguably both of the functions should list out the frames that *are* robust and have protected dual respectively, rather than that are not robust and don't have protected dual, I'd think? johannes
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h index 62b4469c6866..d31876236c0f 100644 --- a/include/linux/ieee80211.h +++ b/include/linux/ieee80211.h @@ -4419,6 +4419,35 @@ static inline bool ieee80211_is_public_action(struct ieee80211_hdr *hdr, return mgmt->u.action.category == WLAN_CATEGORY_PUBLIC; } +/** + * ieee80211_is_protected_dual_of_public_action - check if skb contains a + * protected dual of public action management frame + * @skb: the skb containing the frame, length will be checked + * + * Return: true if the skb contains a protected dual of public action + * management frame, false otherwise. + */ +static inline bool +ieee80211_is_protected_dual_of_public_action(struct sk_buff *skb) +{ + u8 action; + + if (!ieee80211_is_public_action((void *)skb->data, skb->len) || + skb->len < IEEE80211_MIN_ACTION_SIZE + 1) + return false; + + action = *(u8 *)(skb->data + IEEE80211_MIN_ACTION_SIZE); + + return action != WLAN_PUB_ACTION_20_40_BSS_COEX && + action != WLAN_PUB_ACTION_DSE_REG_LOC_ANN && + action != WLAN_PUB_ACTION_MSMT_PILOT && + action != WLAN_PUB_ACTION_TDLS_DISCOVER_RES && + action != WLAN_PUB_ACTION_LOC_TRACK_NOTI && + action != WLAN_PUB_ACTION_FTM_REQUEST && + action != WLAN_PUB_ACTION_FTM_RESPONSE && + action != WLAN_PUB_ACTION_FILS_DISCOVERY; +} + /** * _ieee80211_is_group_privacy_action - check if frame is a group addressed * privacy action frame diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index f2ea15d31c31..69db83433ba1 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -2469,8 +2469,7 @@ ieee80211_drop_unencrypted_mgmt(struct ieee80211_rx_data *rx) /* drop unicast public action frames when using MPF */ if (is_unicast_ether_addr(mgmt->da) && - ieee80211_is_public_action((void *)rx->skb->data, - rx->skb->len)) + ieee80211_is_protected_dual_of_public_action(rx->skb)) return RX_DROP_U_UNPROT_UNICAST_PUB_ACTION; }