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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG_xHGO1GvSy0pC=xNfBGyAin9b58k+b78+7gWv1YoOuQ9OAHQ@mail.gmail.com>
Date:   Mon, 29 Aug 2022 15:43:16 +1200
From:   Gilad Itzkovitch <gilad.itzkovitch@...semicro.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] wifi: ieee80211: Fix for fragmented action frames

Hi Johannes,

On Thu, Aug 25, 2022 at 8:08 PM Johannes Berg <johannes@...solutions.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@...semicro.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