From: Avraham Stern <[email protected]>
Not all public action frames have a protected variant. When MFP is
enabled drop only public action frames that have a dual protected
variant.
Fixes: 76a3059cf124 ("wifi: mac80211: drop some unprotected action frames")
Signed-off-by: Avraham Stern <[email protected]>
Signed-off-by: Gregory Greenman <[email protected]>
---
include/linux/ieee80211.h | 29 +++++++++++++++++++++++++++++
net/mac80211/rx.c | 3 +--
2 files changed, 30 insertions(+), 2 deletions(-)
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;
}
--
2.38.1
On Mon, Oct 16, 2023 at 02:52:48PM +0300, [email protected] 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.
--
Jouni Malinen PGP id EFC895FA
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