Received-SPF: pass (google.com: domain of linux-wireless+bounces-326-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 2 Dec 2023 13:28:49 +0200
From: Jouni Malinen <j@w1.fi>
To: gregory.greenman@intel.com
Cc: johannes@sipsolutions.net, linux-wireless@vger.kernel.org,
	Avraham Stern <avraham.stern@intel.com>
Subject: Re: [PATCH] wifi: mac80211: don't drop all unprotected public action
 frames
Message-ID: <ZWsU8T2c9GHQUsE9@w1.fi>
References: <20231016145213.2973e3c8d3bb.I6198b8d3b04cf4a97b06660d346caec3032f232a@changeid>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231016145213.2973e3c8d3bb.I6198b8d3b04cf4a97b06660d346caec3032f232a@changeid>

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.

-- 
Jouni Malinen                                            PGP id EFC895FA