[PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[Vitaliy Shevtsov]

It is possible to set both MONITOR_FLAG_COOK_FRAMES and MONITOR_FLAG_ACTIVE
flags simultaneously on the same monitor interface from the userspace. This
causes a sub-interface to be created with no IEEE80211_SDATA_IN_DRIVER bit
set because the monitor interface is in the cooked state and it takes
precedence over all other states. When the interface is then being deleted
the kernel calls WARN_ONCE() from check_sdata_in_driver() because of missing
that bit.

Fix this by overriding all flags if MONITOR_FLAG_COOK_FRAMES is set because
this flag is incompatible with other monitor flags being set.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Fixes: 66f7ac50ed7c (nl80211: Add monitor interface configuration flags)
Reported-by: syzbot+2e5c1e55b9e5c28a3da7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2e5c1e55b9e5c28a3da7
Signed-off-by: Vitaliy Shevtsov <v.shevtsov@mt-integration.ru>
---
 net/wireless/nl80211.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d7d3da0f6833..1338b0a06f8d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4220,6 +4220,10 @@ static int parse_monitor_flags(struct nlattr *nla, u32 *mntrflags)
 		if (flags[flag])
 			*mntrflags |= (1<<flag);
 
+	/* MONITOR_FLAG_COOK_FRAMES must override all other flags */
+	if (*mntrflags & MONITOR_FLAG_COOK_FRAMES)
+		*mntrflags = MONITOR_FLAG_COOK_FRAMES;
+
 	*mntrflags |= MONITOR_FLAG_CHANGED;
 
 	return 0;
-- 
2.48.1

Re: [PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[Johannes Berg]

On Thu, 2025-01-30 at 17:34 +0500, Vitaliy Shevtsov wrote:
> It is possible to set both MONITOR_FLAG_COOK_FRAMES and MONITOR_FLAG_ACTIVE
> flags simultaneously on the same monitor interface from the userspace. This
> causes a sub-interface to be created with no IEEE80211_SDATA_IN_DRIVER bit
> set because the monitor interface is in the cooked state and it takes
> precedence over all other states. When the interface is then being deleted
> the kernel calls WARN_ONCE() from check_sdata_in_driver() because of missing
> that bit.
> 
> Fix this by overriding all flags if MONITOR_FLAG_COOK_FRAMES is set because
> this flag is incompatible with other monitor flags being set.
> 

That seems wrong, reject it instead. Anyway we should probably remove
cooked mode.

johannes

Re: [lvc-project] [PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[Fedor Pchelkin]

On Thu, 30. Jan 13:36, Johannes Berg wrote:
> On Thu, 2025-01-30 at 17:34 +0500, Vitaliy Shevtsov wrote:
> > It is possible to set both MONITOR_FLAG_COOK_FRAMES and MONITOR_FLAG_ACTIVE
> > flags simultaneously on the same monitor interface from the userspace. This
> > causes a sub-interface to be created with no IEEE80211_SDATA_IN_DRIVER bit
> > set because the monitor interface is in the cooked state and it takes
> > precedence over all other states. When the interface is then being deleted
> > the kernel calls WARN_ONCE() from check_sdata_in_driver() because of missing
> > that bit.
> > 
> > Fix this by overriding all flags if MONITOR_FLAG_COOK_FRAMES is set because
> > this flag is incompatible with other monitor flags being set.
> > 
> 
> That seems wrong, reject it instead. Anyway we should probably remove
> cooked mode.

Do you suggest rejecting to set MONITOR_FLAG_COOK_FRAMES overall?
Wouldn't it break existing userspace, especially in context of systems
running old stable kernels where the patch is also needed?

There is still some usage of this flag in hostap [1].

Or your suggestion is to explicitly reject setting MONITOR_FLAG_COOK_FRAMES
only when it is passed combined with some other flags which it is
incompatible with?

Btw, the fragment [2] says the cooked flag overrides the other ones.
But it was written a long time ago so many things have changed I guess.

  /**
   * enum nl80211_mntr_flags - monitor configuration flags
   *
   * Monitor configuration flags.
  
  ...
  
   * @NL80211_MNTR_FLAG_COOK_FRAMES: report frames after processing.
   *	overrides all other flags.

[1]: https://w1.fi/cgit/hostap/tree/src/drivers/driver_nl80211.c#n6209
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/nl80211.h#n4731

--
Thanks,
Fedor

Re: [lvc-project] [PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[Johannes Berg]

On Thu, 2025-01-30 at 22:23 +0300, Fedor Pchelkin wrote:
> 
> Do you suggest rejecting to set MONITOR_FLAG_COOK_FRAMES overall?

No and yes :)

No for the context of this patch. Yes in general.

> Wouldn't it break existing userspace, especially in context of systems
> running old stable kernels where the patch is also needed?
> 
> There is still some usage of this flag in hostap [1].

Theoretically, but I just commented on that here:

https://lore.kernel.org/r/a49e58998553c45953a30243ad1957c06ce6db8c.camel@sipsolutions.net

tl;dr: only ancient hostapd versions will actually _use_ it, and they
have to fall into a relatively narrow range (April 2009 - Dec 2011.)

> Or your suggestion is to explicitly reject setting MONITOR_FLAG_COOK_FRAMES
> only when it is passed combined with some other flags which it is
> incompatible with?

Yes.

> Btw, the fragment [2] says the cooked flag overrides the other ones.
> But it was written a long time ago so many things have changed I guess.

Indeed, that doesn't really make sense (certainly not any longer.)

> [1]: https://w1.fi/cgit/hostap/tree/src/drivers/driver_nl80211.c#n6209

And that was almost certainly the only user that ever existed, and can
only set the flag by itself.

johannes

Re: [lvc-project] [PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[Jouni Malinen]

On Thu, Jan 30, 2025 at 10:13:14PM +0100, Johannes Berg wrote:
> On Thu, 2025-01-30 at 22:23 +0300, Fedor Pchelkin wrote:
> > Wouldn't it break existing userspace, especially in context of systems
> > running old stable kernels where the patch is also needed?
> > 
> > There is still some usage of this flag in hostap [1].
> 
> Theoretically, but I just commented on that here:
> 
> https://lore.kernel.org/r/a49e58998553c45953a30243ad1957c06ce6db8c.camel@sipsolutions.net
> 
> tl;dr: only ancient hostapd versions will actually _use_ it, and they
> have to fall into a relatively narrow range (April 2009 - Dec 2011.)

How did you determine that commit a11241fa1149 ("nl80211: Use nl80211
for mgmt TX/RX in AP mode") ends this use? Support for monitor mode
interface is still in hostap.git and it is even tested as part of the
hwsim test cases.. Both hostapd and wpa_supplicant can still be
configured to use the monitor interface.

It would be another question to ask whether there is any good reason to
use this anymore now that a better approach has been available for 13
years and the answer to that is likely "no". Anyway, this is a kernel
interface that has a user even in the current snapshot of user space
programs. If we are about to break that use, it would make sense to
first remove such users. I don't think I would really care about this
anymore and it would be nice to get rid of all that unlikely to be used
much, if at all, code.

> > Or your suggestion is to explicitly reject setting MONITOR_FLAG_COOK_FRAMES
> > only when it is passed combined with some other flags which it is
> > incompatible with?
> 
> Yes.

Though, this seems to imply that the case used by hostapd/wpa_supplicant
would not be broken.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [lvc-project] [PATCH] wifi: nl80211: override all other flags if MONITOR_FLAG_COOK_FRAMES is set

[Johannes Berg]

On Mon, 2025-02-03 at 12:40 +0200, Jouni Malinen wrote:
> On Thu, Jan 30, 2025 at 10:13:14PM +0100, Johannes Berg wrote:
> > On Thu, 2025-01-30 at 22:23 +0300, Fedor Pchelkin wrote:
> > > Wouldn't it break existing userspace, especially in context of systems
> > > running old stable kernels where the patch is also needed?
> > > 
> > > There is still some usage of this flag in hostap [1].
> > 
> > Theoretically, but I just commented on that here:
> > 
> > https://lore.kernel.org/r/a49e58998553c45953a30243ad1957c06ce6db8c.camel@sipsolutions.net
> > 
> > tl;dr: only ancient hostapd versions will actually _use_ it, and they
> > have to fall into a relatively narrow range (April 2009 - Dec 2011.)
> 
> How did you determine that commit a11241fa1149 ("nl80211: Use nl80211
> for mgmt TX/RX in AP mode") ends this use? Support for monitor mode
> interface is still in hostap.git and it is even tested as part of the
> hwsim test cases.. Both hostapd and wpa_supplicant can still be
> configured to use the monitor interface.

Well, fair, there are perhaps certain explicit configurations where you
get monitor support, but by default as of that commit it should be
selecting the nl80211 path.

Though looking further, I suppose that in practice commit 73a3c6ffca0c
("nl80211: Use the monitor interface if socket tx status is not
supported") moves the date out a bit, albeit not related to upstream
kernels but only wifi backports to earlier upstream kernels.

> It would be another question to ask whether there is any good reason to
> use this anymore now that a better approach has been available for 13
> years and the answer to that is likely "no". Anyway, this is a kernel
> interface that has a user even in the current snapshot of user space
> programs. If we are about to break that use, it would make sense to
> first remove such users. I don't think I would really care about this
> anymore and it would be nice to get rid of all that unlikely to be used
> much, if at all, code.

Right.

> Though, this seems to imply that the case used by hostapd/wpa_supplicant
> would not be broken.

Not for this patch, no, but I did float the idea of removing it all
entirely.

johannes