[PATCH] wifi: ieee80211: Fix for fragmented action frames

[PATCH] wifi: ieee80211: Fix for fragmented action frames

[Gilad Itzkovitch]

The robust management frame check ensures a station exists for
the frame before proceeding, but there are some action frame
categories which don't require an existing station, and so the
_ieee80211_is_robust_mgmt_frame function peeks into the
action frame's payload to identify the category and filter them out.

In some scenarios, e.g. DPP at S1G data rates, action frames
can get fragmented. This commit adds an extra check to ensure
we don't peek into the payload of fragmented frames beyond the
first fragment.

Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
---
 include/linux/ieee80211.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 55e6f4ad0ca6..5da9608fdce3 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -4124,6 +4124,7 @@ static inline bool _ieee80211_is_robust_mgmt_frame(struct ieee80211_hdr *hdr)
 
 	if (ieee80211_is_action(hdr->frame_control)) {
 		u8 *category;
+		u16 sc;
 
 		/*
 		 * Action frames, excluding Public Action frames, are Robust
@@ -4134,6 +4135,17 @@ static inline bool _ieee80211_is_robust_mgmt_frame(struct ieee80211_hdr *hdr)
 		 */
 		if (ieee80211_has_protected(hdr->frame_control))
 			return true;
+
+		/*
+		 * Some action frames do not have a STA associated with them,
+		 * so we rule them out from the robust management frame check.
+		 * The category is within the payload, so we only proceed if
+		 * we're checking the first fragment.
+		 */
+		sc = le16_to_cpu(hdr->seq_ctrl);
+		if (sc & IEEE80211_SCTL_FRAG)
+			return false;
+
 		category = ((u8 *) hdr) + 24;
 		return *category != WLAN_CATEGORY_PUBLIC &&
 			*category != WLAN_CATEGORY_HT &&
-- 
2.25.1

Re: [PATCH] wifi: ieee80211: Fix for fragmented action frames

[Johannes Berg]

On Thu, 2022-08-11 at 10:48 +1200, Gilad Itzkovitch wrote:
> The robust management frame check ensures a station exists for
> the frame before proceeding, but there are some action frame
> categories which don't require an existing station, and so the
> _ieee80211_is_robust_mgmt_frame function peeks into the
> action frame's payload to identify the category and filter them out.
> 
> In some scenarios, e.g. DPP at S1G data rates, action frames
> can get fragmented. This commit adds an extra check to ensure
> we don't peek into the payload of fragmented frames beyond the
> first fragment.
> 
> Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
> ---
>  include/linux/ieee80211.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
> index 55e6f4ad0ca6..5da9608fdce3 100644
> --- a/include/linux/ieee80211.h
> +++ b/include/linux/ieee80211.h
> @@ -4124,6 +4124,7 @@ static inline bool _ieee80211_is_robust_mgmt_frame(struct ieee80211_hdr *hdr)
>  
>  	if (ieee80211_is_action(hdr->frame_control)) {
>  		u8 *category;
> +		u16 sc;
>  
>  		/*
>  		 * Action frames, excluding Public Action frames, are Robust
> @@ -4134,6 +4135,17 @@ static inline bool _ieee80211_is_robust_mgmt_frame(struct ieee80211_hdr *hdr)
>  		 */
>  		if (ieee80211_has_protected(hdr->frame_control))
>  			return true;
> +
> +		/*
> +		 * Some action frames do not have a STA associated with them,
> +		 * so we rule them out from the robust management frame check.
> +		 * The category is within the payload, so we only proceed if
> +		 * we're checking the first fragment.
> +		 */
> +		sc = le16_to_cpu(hdr->seq_ctrl);
> +		if (sc & IEEE80211_SCTL_FRAG)
> +			return false;
> 


This doesn't make much sense to me - why would it be allowed or
necessary to call this function on a frame that wasn't yet defragmented?

johannes

Re: [PATCH] wifi: ieee80211: Fix for fragmented action frames

[Gilad Itzkovitch]

Hi Johannes,

On Thu, Aug 25, 2022 at 8:08 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Thu, 2022-08-11 at 10:48 +1200, Gilad Itzkovitch wrote:
> > The robust management frame check ensures a station exists for
> > the frame before proceeding, but there are some action frame
> > categories which don't require an existing station, and so the
> > _ieee80211_is_robust_mgmt_frame function peeks into the
> > action frame's payload to identify the category and filter them out.
> >
> > In some scenarios, e.g. DPP at S1G data rates, action frames
> > can get fragmented. This commit adds an extra check to ensure
> > we don't peek into the payload of fragmented frames beyond the
> > first fragment.
> >
> > Signed-off-by: Gilad Itzkovitch <gilad.itzkovitch@morsemicro.com>
> > ---
> >  include/linux/ieee80211.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
> > index 55e6f4ad0ca6..5da9608fdce3 100644
> > --- a/include/linux/ieee80211.h
> > +++ b/include/linux/ieee80211.h
> > @@ -4124,6 +4124,7 @@ static inline bool _ieee80211_is_robust_mgmt_frame(struct ieee80211_hdr *hdr)
> >
> >       if (ieee80211_is_action(hdr->frame_control)) {
> >               u8 *category;
> > +             u16 sc;
> >
> >               /*
> >                * Action frames, excluding Public Action frames, are Robust
> > @@ -4134,6 +4135,17 @@ static inline bool _ieee80211_is_robust_mgmt_frame(struct ieee80211_hdr *hdr)
> >                */
> >               if (ieee80211_has_protected(hdr->frame_control))
> >                       return true;
> > +
> > +             /*
> > +              * Some action frames do not have a STA associated with them,
> > +              * so we rule them out from the robust management frame check.
> > +              * The category is within the payload, so we only proceed if
> > +              * we're checking the first fragment.
> > +              */
> > +             sc = le16_to_cpu(hdr->seq_ctrl);
> > +             if (sc & IEEE80211_SCTL_FRAG)
> > +                     return false;
> >
>
>
> This doesn't make much sense to me - why would it be allowed or
> necessary to call this function on a frame that wasn't yet defragmented?
>
> johannes

That was partially our understanding. But, the fragmented action frame is
being dropped by this function as it is part of the provisioning DPP process
(fragmented due to S1G low rates).
Trying to avoid a big change here for this specific action category.
As defragmentation will occur later on in the process there should be a
safe way to avoid dropping the frame beforehand.

Gilad

Re: [PATCH] wifi: ieee80211: Fix for fragmented action frames

[Johannes Berg]

Hi,

On Mon, 2022-08-29 at 15:43 +1200, Gilad Itzkovitch wrote:
> 
> > This doesn't make much sense to me - why would it be allowed or
> > necessary to call this function on a frame that wasn't yet defragmented?
> 
> That was partially our understanding. But, the fragmented action frame is
> being dropped by this function
> 

Well, this function doesn't drop anything :) It just answers a question
"is this a robust management frame", but if the question is nonsense
(calling it on a fragment other than the first) then the answer will
also be nonsense, right?

> as it is part of the provisioning DPP process
> (fragmented due to S1G low rates).

Right.

> Trying to avoid a big change here for this specific action category.

It's not really related to any specific category, is it?

> As defragmentation will occur later on in the process there should be a
> safe way to avoid dropping the frame beforehand.

Sure! But this isn't really a good way, nor would I argue it is safe ...

Perhaps ieee80211_rx_h_decrypt() needs to be more careful? I'm not even
sure where the frame really is being dropped.

Anyway, I really don't think this patch makes any sense, I think you
need to back up a bit and look at the higher layer(s) to see where and
why it's being dropped, and skip that if it's fragmented?


johannes