[PATCH v2] Set ssid when authenticating

[PATCH v2] Set ssid when authenticating

[Marc Bornand]

changes since v1:
- add some informations
- test it on wireless-2023-01-18 tag
- no real code change

When a connexion was established without going through
NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
Now we set it during when an NL80211_CMD_AUTHENTICATE is issued.

It may be needed to test this on some additional hardware (tested with
iwlwifi and a AX201, and iwd on the userspace side), I could not test
things like roaming and p2p.

alternatives:
1. Do the same but during association and not authentication.
2. use ieee80211_bss_get_elem in nl80211_send_iface, this would report
   the right ssid to userspace, but this would not fix the root cause,
   this alos wa the behavior prior to 7b0a0e3c3a882 when the bug was
   introduced.

This applies to v6.2-rc8 or wireless-2023-01-18,

The last linux version known to be unafected is 5.19 and the bug was
backported to the 5.19.y releases

Reported-by: Yohan Prod'homme <kernel@zoddo.fr>
Fixes: 7b0a0e3c3a88260b6fcb017e49f198463aa62ed1
Cc: stable@vger.kernel.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216711
Signed-off-by: Marc Bornand <dev.mbornand@systemb.ch>
---
 net/wireless/nl80211.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 33a82ecab9d5..f1627ea542b9 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -10552,6 +10552,10 @@ static int nl80211_authenticate(struct sk_buff *skb, struct genl_info *info)
 		return -ENOENT;

 	wdev_lock(dev->ieee80211_ptr);
+
+	memcpy(dev->ieee80211_ptr->u.client.ssid, ssid, ssid_len);
+	dev->ieee80211_ptr->u.client.ssid_len = ssid_len;
+
 	err = cfg80211_mlme_auth(rdev, dev, &req);
 	wdev_unlock(dev->ieee80211_ptr);

@@ -11025,6 +11029,11 @@ static int nl80211_deauthenticate(struct sk_buff *skb, struct genl_info *info)
 	local_state_change = !!info->attrs[NL80211_ATTR_LOCAL_STATE_CHANGE];

 	wdev_lock(dev->ieee80211_ptr);
+
+	if (reason_code == WLAN_REASON_DEAUTH_LEAVING) {
+		dev->ieee80211_ptr->u.client.ssid_len = 0;
+	}
+
 	err = cfg80211_mlme_deauth(rdev, dev, bssid, ie, ie_len, reason_code,
 				   local_state_change);
 	wdev_unlock(dev->ieee80211_ptr);
--
2.39.1

Re: [PATCH v2] Set ssid when authenticating

[Johannes Berg]

On Mon, 2023-02-13 at 10:55 +0000, Marc Bornand wrote:
> changes since v1:
> - add some informations
> - test it on wireless-2023-01-18 tag
> - no real code change
> 
> When a connexion was established without going through
> NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> Now we set it during when an NL80211_CMD_AUTHENTICATE is issued.

This is incorrect, doing an authentication doesn't require doing an
association afterwards, and doesn't necessarily imply any state change
in the kernel.

> alternatives:
> 1. Do the same but during association and not authentication.

Which should probably be done _after_ successful authentication, even in
the CONNECT command case, which currently does it in cfg80211_connect()
but I guess that should move to __cfg80211_connect_result().

> 2. use ieee80211_bss_get_elem in nl80211_send_iface, this would report
>    the right ssid to userspace, but this would not fix the root cause,
>    this alos wa the behavior prior to 7b0a0e3c3a882 when the bug was
>    introduced.

That would be OK too but the reason I changed it there (missing the fact
that it wasn't set) is that we have multiple BSSes with MLO. So it's
hard to get one to do this with.

johannes

Re: [PATCH v2] Set ssid when authenticating

[Marc Bornand]

On Monday, February 13th, 2023 at 12:01, Johannes Berg <johannes@sipsolutions.net> wrote:
> 
> 
> On Mon, 2023-02-13 at 10:55 +0000, Marc Bornand wrote:
> 
> > changes since v1:
> > - add some informations
> > - test it on wireless-2023-01-18 tag
> > - no real code change
> > 
> > When a connexion was established without going through
> > NL80211_CMD_CONNECT, the ssid was never set in the wireless_dev struct.
> > Now we set it during when an NL80211_CMD_AUTHENTICATE is issued.
> 
> 
> This is incorrect, doing an authentication doesn't require doing an
> association afterwards, and doesn't necessarily imply any state change
> in the kernel.

So is it intended behavior that the ssid in wireless_dev is not set
or is there a place were this state change should happen?

> 
> > alternatives:
> > 1. Do the same but during association and not authentication.
> 
> 
> Which should probably be done after successful authentication, even in
> the CONNECT command case, which currently does it in cfg80211_connect()
> but I guess that should move to __cfg80211_connect_result().

Is there an existing way to get the ssid in __cfg80211_connect_result()?


> 
> > 2. use ieee80211_bss_get_elem in nl80211_send_iface, this would report
> > the right ssid to userspace, but this would not fix the root cause,
> > this alos wa the behavior prior to 7b0a0e3c3a882 when the bug was
> > introduced.
> 
> 
> That would be OK too but the reason I changed it there (missing the fact
> that it wasn't set) is that we have multiple BSSes with MLO. So it's
> hard to get one to do this with.
> 
> johannes

Just a side question do the BSSes all have the same SSID?

Marc

Re: [PATCH v2] Set ssid when authenticating

[Johannes Berg]

Hi,

As an aside - there's little point in encrypting the mail sent to me
when you send it also to a public list :) Just makes it more annoying to
use.

> > This is incorrect, doing an authentication doesn't require doing an
> > association afterwards, and doesn't necessarily imply any state change
> > in the kernel.
> 
> So is it intended behavior that the ssid in wireless_dev is not set
> or is there a place were this state change should happen?

It's incorrect in that this is the wrong place to set it.

I don't have a strong feeling about whether it _should_ be set, but I
clearly assumed that it is indeed set ...

> > > alternatives:
> > > 1. Do the same but during association and not authentication.
> > 
> > 
> > Which should probably be done after successful authentication, even in
> > the CONNECT command case, which currently does it in cfg80211_connect()
> > but I guess that should move to __cfg80211_connect_result().
> 
> Is there an existing way to get the ssid in __cfg80211_connect_result()?

There's the BSS, or multiple pointers for multi-link.

> Just a side question do the BSSes all have the same SSID?
> 

In multi-link? Yes, I don't think we actively enforce that wpa_s does
that, but we'd probably fail to connect to the AP if that weren't the
case. So yeah. Maybe we should check it in assoc.

Here I think you can safely just pick any of the BSSes and look at the
SSID. Really we could even do the same in the nl80211 code, but it's
probably easier to fill in the ssid when we already have it anyway.

In the connect case it might be needed to fill it in earlier for use by
the SME state machine, not sure.

johannes

Re: [PATCH v2] Set ssid when authenticating

[Marc Bornand]

On Monday, February 13th, 2023 at 18:37, Johannes Berg <johannes@sipsolutions.net> wrote:


>
>
> Hi,
>
> As an aside - there's little point in encrypting the mail sent to me
> when you send it also to a public list :) Just makes it more annoying to
> use.
Really Sorry, The mail service I am using is currently not letting me deactivate
encryption for recipients with a wkd, I think I will try to contact support
and ask there.

>
> > > This is incorrect, doing an authentication doesn't require doing an
> > > association afterwards, and doesn't necessarily imply any state change
> > > in the kernel.
> >
> > So is it intended behavior that the ssid in wireless_dev is not set
> > or is there a place were this state change should happen?
>
>
> It's incorrect in that this is the wrong place to set it.
>
> I don't have a strong feeling about whether it should be set, but I
> clearly assumed that it is indeed set ...

The upstream behavior for know is that the SSID is not set in
wireless_dev.u.client.ssid, when cfg80211_connect is not used.
This is the case at least one some setup with iwd and wpa3.

>
> > > > alternatives:
> > > > 1. Do the same but during association and not authentication.
> > >
> > > Which should probably be done after successful authentication, even in
> > > the CONNECT command case, which currently does it in cfg80211_connect()
> > > but I guess that should move to __cfg80211_connect_result().
> >
> > Is there an existing way to get the ssid in __cfg80211_connect_result()?
>
>
> There's the BSS, or multiple pointers for multi-link.
>
> > Just a side question do the BSSes all have the same SSID?
>
>
> In multi-link? Yes, I don't think we actively enforce that wpa_s does
> that, but we'd probably fail to connect to the AP if that weren't the
> case. So yeah. Maybe we should check it in assoc.
>
> Here I think you can safely just pick any of the BSSes and look at the
> SSID. Really we could even do the same in the nl80211 code, but it's
> probably easier to fill in the ssid when we already have it anyway.
>
> In the connect case it might be needed to fill it in earlier for use by
> the SME state machine, not sure.
>
> johannes


Marc

Re: [PATCH v2] Set ssid when authenticating

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On Mon, Feb 13, 2023 at 07:04:22PM +0000, Marc Bornand wrote:
> On Monday, February 13th, 2023 at 18:37, Johannes Berg <johannes@sipsolutions.net> wrote:
> > As an aside - there's little point in encrypting the mail sent to me
> > when you send it also to a public list :) Just makes it more annoying to
> > use.

> Really Sorry, The mail service I am using is currently not letting me deactivate
> encryption for recipients with a wkd, I think I will try to contact support
> and ask there.

It's proton isn't it?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/process/email-clients.rst#n354

Good luck with their support, I'm curious how you get on!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]