| Message ID | 20230124141856.356646-1-alexander@wetzel-home.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wifi: cfg80211: Fix use after free for wext | expand |
Hi, This broke WPA auth entirely on brcmfmac (in offload mode) and probably others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please revert or fix. Notes below. Reported-by: Ilya <me@0upti.me> Reported-by: Janne Grunau <j@jannau.net> #regzbot introduced: 015b8cc5e7c4d7 #regzbot monitor: https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wetzel-home.de/ On 24/01/2023 23.18, Alexander Wetzel wrote: > Key information in wext.connect is not reset on (re)connect and can hold > data from a previous connection. > > Reset key data to avoid that drivers or mac80211 incorrectly detect a > WEP connection request and access the freed or already reused memory. > > Additionally optimize cfg80211_sme_connect() and avoid an useless > schedule of conn_work. > > Fixes: fffd0934b939 ("cfg80211: rework key operation") > Cc: stable@vger.kernel.org > Link: https://lore.kernel.org/r/c80f04d2-8159-a02a-9287-26e5ec838826@wetzel-home.de > Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de> > > --- > V2 changes: > - updated comment > - reset more key data > > --- > net/wireless/sme.c | 31 ++++++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/net/wireless/sme.c b/net/wireless/sme.c > index 123248b2c0be..0cc841c0c59b 100644 > --- a/net/wireless/sme.c > +++ b/net/wireless/sme.c [snip] > @@ -1464,6 +1476,15 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, This if branch only fires if the connection is WEP. > } else { > if (WARN_ON(connkeys)) > return -EINVAL; > + > + /* connect can point to wdev->wext.connect which > + * can hold key data from a previous connection > + */ > + connect->key = NULL; > + connect->key_len = 0; > + connect->key_idx = 0; And these are indeed only used by WEP. > + connect->crypto.cipher_group = 0; > + connect->crypto.n_ciphers_pairwise = 0; But here you're killing the info that is used for *other* auth modes too if !WEP, breaking WPA and everything else. > } > > wdev->connect_keys = connkeys; - Hector
Hi Hector, On 3/11/23 10:55, Hector Martin wrote: > Hi, > > This broke WPA auth entirely on brcmfmac (in offload mode) and probably > others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please > revert or fix. Notes below. > > Reported-by: Ilya <me@0upti.me> > Reported-by: Janne Grunau <j@jannau.net> > > #regzbot introduced: 015b8cc5e7c4d7 > #regzbot monitor: > https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wetzel-home.de/ I can confirm this bug, I was seeing broken wifi on brcmfmac with 6.3-rc1 and I was about to start a git bisect for this this morning when I saw this email. Reverting 015b8cc5e7c4d7 fixes the broken wifi. Hector, thank you, you just saved me from a bisect on somewhat slow hardware :) Regards, Hans > > On 24/01/2023 23.18, Alexander Wetzel wrote: >> Key information in wext.connect is not reset on (re)connect and can hold >> data from a previous connection. >> >> Reset key data to avoid that drivers or mac80211 incorrectly detect a >> WEP connection request and access the freed or already reused memory. >> >> Additionally optimize cfg80211_sme_connect() and avoid an useless >> schedule of conn_work. >> >> Fixes: fffd0934b939 ("cfg80211: rework key operation") >> Cc: stable@vger.kernel.org >> Link: https://lore.kernel.org/r/c80f04d2-8159-a02a-9287-26e5ec838826@wetzel-home.de >> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de> >> >> --- >> V2 changes: >> - updated comment >> - reset more key data >> >> --- >> net/wireless/sme.c | 31 ++++++++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/net/wireless/sme.c b/net/wireless/sme.c >> index 123248b2c0be..0cc841c0c59b 100644 >> --- a/net/wireless/sme.c >> +++ b/net/wireless/sme.c > [snip] >> @@ -1464,6 +1476,15 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, > > This if branch only fires if the connection is WEP. > >> } else { >> if (WARN_ON(connkeys)) >> return -EINVAL; >> + >> + /* connect can point to wdev->wext.connect which >> + * can hold key data from a previous connection >> + */ >> + connect->key = NULL; >> + connect->key_len = 0; >> + connect->key_idx = 0; > > And these are indeed only used by WEP. > >> + connect->crypto.cipher_group = 0; >> + connect->crypto.n_ciphers_pairwise = 0; > > But here you're killing the info that is used for *other* auth modes too > if !WEP, breaking WPA and everything else. > >> } >> >> wdev->connect_keys = connkeys; > > - Hector >
On Sat, Mar 11, 2023 at 12:03:44PM +0100, Hans de Goede wrote: > Hi Hector, > > On 3/11/23 10:55, Hector Martin wrote: > > Hi, > > > > This broke WPA auth entirely on brcmfmac (in offload mode) and probably > > others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please > > revert or fix. Notes below. > > > > Reported-by: Ilya <me@0upti.me> > > Reported-by: Janne Grunau <j@jannau.net> > > > > #regzbot introduced: 015b8cc5e7c4d7 > > #regzbot monitor: > > https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wetzel-home.de/ > > I can confirm this bug, I was seeing broken wifi on brcmfmac with 6.3-rc1 > and I was about to start a git bisect for this this morning when I saw > this email. > > Reverting 015b8cc5e7c4d7 fixes the broken wifi. Hector, thank you, you > just saved me from a bisect on somewhat slow hardware :) Great, can someone submit the revert patch to the networking tree so we can get this resolved quickly? thanks, greg k-h
I can also confirm the regression with the (out-of-tree) broadcom-wl driver. As Hector said reverting the cipher_group / n_ciphers_pairwise lines is enough. See: https://gist.github.com/joanbm/fb90f4807b719a2e37a496936faabec9 Regards, - Joan
[TLDR: This mail in primarily relevant for Linux kernel regression tracking. See link in footer if these mails annoy you.] On 11.03.23 10:55, Hector Martin wrote: > > This broke WPA auth entirely on brcmfmac (in offload mode) and probably > others, including on stable 6.2.3 and 6.3-rc1 (tested with iwd). Please > revert or fix. Notes below. > > Reported-by: Ilya <me@0upti.me> > Reported-by: Janne Grunau <j@jannau.net> > > #regzbot introduced: 015b8cc5e7c4d7 > #regzbot monitor: > https://lore.kernel.org/linux-wireless/20230124141856.356646-1-alexander@wetzel-home.de/ #regzbot fix: 79d1ed5ca7db67d48 #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index 123248b2c0be..0cc841c0c59b 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -285,6 +285,15 @@ void cfg80211_conn_work(struct work_struct *work) wiphy_unlock(&rdev->wiphy); } +static void cfg80211_step_auth_next(struct cfg80211_conn *conn, + struct cfg80211_bss *bss) +{ + memcpy(conn->bssid, bss->bssid, ETH_ALEN); + conn->params.bssid = conn->bssid; + conn->params.channel = bss->channel; + conn->state = CFG80211_CONN_AUTHENTICATE_NEXT; +} + /* Returned bss is reference counted and must be cleaned up appropriately. */ static struct cfg80211_bss *cfg80211_get_conn_bss(struct wireless_dev *wdev) { @@ -302,10 +311,7 @@ static struct cfg80211_bss *cfg80211_get_conn_bss(struct wireless_dev *wdev) if (!bss) return NULL; - memcpy(wdev->conn->bssid, bss->bssid, ETH_ALEN); - wdev->conn->params.bssid = wdev->conn->bssid; - wdev->conn->params.channel = bss->channel; - wdev->conn->state = CFG80211_CONN_AUTHENTICATE_NEXT; + cfg80211_step_auth_next(wdev->conn, bss); schedule_work(&rdev->conn_work); return bss; @@ -597,7 +603,12 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev, wdev->conn->params.ssid_len = wdev->u.client.ssid_len; /* see if we have the bss already */ - bss = cfg80211_get_conn_bss(wdev); + bss = cfg80211_get_bss(wdev->wiphy, wdev->conn->params.channel, + wdev->conn->params.bssid, + wdev->conn->params.ssid, + wdev->conn->params.ssid_len, + wdev->conn_bss_type, + IEEE80211_PRIVACY(wdev->conn->params.privacy)); if (prev_bssid) { memcpy(wdev->conn->prev_bssid, prev_bssid, ETH_ALEN); @@ -608,6 +619,7 @@ static int cfg80211_sme_connect(struct wireless_dev *wdev, if (bss) { enum nl80211_timeout_reason treason; + cfg80211_step_auth_next(wdev->conn, bss); err = cfg80211_conn_do_work(wdev, &treason); cfg80211_put_bss(wdev->wiphy, bss); } else { @@ -1464,6 +1476,15 @@ int cfg80211_connect(struct cfg80211_registered_device *rdev, } else { if (WARN_ON(connkeys)) return -EINVAL; + + /* connect can point to wdev->wext.connect which + * can hold key data from a previous connection + */ + connect->key = NULL; + connect->key_len = 0; + connect->key_idx = 0; + connect->crypto.cipher_group = 0; + connect->crypto.n_ciphers_pairwise = 0; } wdev->connect_keys = connkeys;
Key information in wext.connect is not reset on (re)connect and can hold data from a previous connection. Reset key data to avoid that drivers or mac80211 incorrectly detect a WEP connection request and access the freed or already reused memory. Additionally optimize cfg80211_sme_connect() and avoid an useless schedule of conn_work. Fixes: fffd0934b939 ("cfg80211: rework key operation") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/c80f04d2-8159-a02a-9287-26e5ec838826@wetzel-home.de Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de> --- V2 changes: - updated comment - reset more key data --- net/wireless/sme.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)