| Message ID | 1622222314-17192-1-git-send-email-martin.fuzzey@flowbird.group (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | rsi: fix broken AP mode due to unwanted encryption | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 5/28/21 7:17 PM, Martin Fuzzey wrote: > In AP mode WPA-PSK connections were not established. > > The reason was that the AP was sending the first message > of the 4 way handshake encrypted, even though no key had > (correctly) yet been set. > > Fix this by adding an extra check that we have a key. > > The redpine downstream out of tree driver does it this way too. > > Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> > CC: stable@vger.kernel.org This likely needs a Fixes: tag ? > --- > drivers/net/wireless/rsi/rsi_91x_hal.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c > index ce98921..ef84262 100644 > --- a/drivers/net/wireless/rsi/rsi_91x_hal.c > +++ b/drivers/net/wireless/rsi/rsi_91x_hal.c > @@ -203,6 +203,7 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb) > wh->frame_control |= cpu_to_le16(RSI_SET_PS_ENABLE); > > if ((!(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) && > + (info->control.hw_key) && The () are not needed. > (common->secinfo.security_enable)) { > if (rsi_is_cipher_wep(common)) > ieee80211_size += 4; > Otherwise, looks good, thanks. With those two things above fixed, add: Reviewed-by: Marek Vasut <marex@denx.de>
Hi Marek, thanks for the review. On Fri, 28 May 2021 at 20:11, Marek Vasut <marex@denx.de> wrote: > > > Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> > > CC: stable@vger.kernel.org > > This likely needs a Fixes: tag ? > I'm not quite sure what that should be. The test involved here has been present since the very first version of the driver back in 2014 but at that time AP mode wasn't supported. AP mode was added in 2017 by the patch series "rsi: support for AP mode" [1] In particular 38ef62353acb ("rsi: security enhancements for AP mode") does some stuff relating to AP key configuration but it doesn't actually change the behaviour concerning the encryption condition. In fact I don't understand how it ever worked in AP WPA2 mode given that secinfo->security_enable (which is tested in the encryption condition) has always been unconditionally set in set_key (when setting not deleting). Yet the series cover letter [1] says "Tests are performed to confirm aggregation, connections in WEP and WPA/WPA2 security." The problem is that in AP mode with WPA2 there is a set_key done at AP startup time to set the GTK (but not yet the pairwise key which is only done after the 4 way handshake) so this sets security_enable to true which later causes the EAPOL messages to be sent encrypted. Maybe there have been userspace changes to hostapd that have changed the time at which the GTK is set? I had a quick look at the hostapd history but didn't see anything obvious. I'm going to send a V2 completely removing the security_enable flag in addition to adding the new test (which is what downstream has too). Keeping security_enable doesn't actually break anything but is redundant and confusing. Unfortunately I cannot find any downstream history, I just have 2 downstream tarballs, a "2.0 RC2" which has the same problem as mainline and a "2.0 RC4" which does not [1] https://www.spinics.net/lists/linux-wireless/msg165302.html > > if ((!(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) && > > + (info->control.hw_key) && > > The () are not needed. > Ok, will fix for V2 > Reviewed-by: Marek Vasut <marex@denx.de> Seeing as the V2 will be a bit different I won't add that yet. Martin
On 6/1/21 11:02 AM, Fuzzey, Martin wrote: > Hi Marek, > thanks for the review. Hi, > On Fri, 28 May 2021 at 20:11, Marek Vasut <marex@denx.de> wrote: >> >>> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> >>> CC: stable@vger.kernel.org >> >> This likely needs a Fixes: tag ? >> > > I'm not quite sure what that should be. Based on git log -p --follow drivers/net/wireless/rsi/rsi_91x_hal.c Fixes: dad0d04fa7ba ("rsi: Add RS9113 wireless driver") > The test involved here has been present since the very first version > of the driver back in 2014 but at that time AP mode wasn't supported. > > AP mode was added in 2017 by the patch series "rsi: support for AP mode" [1] > In particular 38ef62353acb ("rsi: security enhancements for AP mode") > does some stuff relating to AP key configuration but it doesn't > actually change the behaviour concerning the encryption condition. > > In fact I don't understand how it ever worked in AP WPA2 mode given > that secinfo->security_enable (which is tested in the encryption > condition) has always been unconditionally set in set_key (when > setting not deleting). > Yet the series cover letter [1] says "Tests are performed to confirm > aggregation, connections in WEP and WPA/WPA2 security." There are way too many bugs in that RSI driver, yes. Compared to the other WiFi vendors, this driver seems to be real poor. > The problem is that in AP mode with WPA2 there is a set_key done at AP > startup time to set the GTK (but not yet the pairwise key which is > only done after the 4 way handshake) so this sets security_enable to > true which later causes the EAPOL messages to be sent encrypted. > > Maybe there have been userspace changes to hostapd that have changed > the time at which the GTK is set? > I had a quick look at the hostapd history but didn't see anything obvious. I recall seeing a patched WPA supplicant in one set of the RSI downstream drivers. And then there is another older RSI "onebox" driver which definitely ships patched userspace components. So I wouldn't be surprised if some of that was involved. > I'm going to send a V2 completely removing the security_enable flag in > addition to adding the new test (which is what downstream has too). > Keeping security_enable doesn't actually break anything but is > redundant and confusing. > > Unfortunately I cannot find any downstream history, I just have 2 > downstream tarballs, a "2.0 RC2" which has the same problem as > mainline and a "2.0 RC4" which does not Yes indeed, I pointed that out to RSI before, that this kind of release management is useless. I'm still waiting for any input from them. > [1] https://www.spinics.net/lists/linux-wireless/msg165302.html > >>> if ((!(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) && >>> + (info->control.hw_key) && >> >> The () are not needed. >> > > Ok, will fix for V2 Thanks
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c index ce98921..ef84262 100644 --- a/drivers/net/wireless/rsi/rsi_91x_hal.c +++ b/drivers/net/wireless/rsi/rsi_91x_hal.c @@ -203,6 +203,7 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb) wh->frame_control |= cpu_to_le16(RSI_SET_PS_ENABLE); if ((!(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)) && + (info->control.hw_key) && (common->secinfo.security_enable)) { if (rsi_is_cipher_wep(common)) ieee80211_size += 4;
In AP mode WPA-PSK connections were not established. The reason was that the AP was sending the first message of the 4 way handshake encrypted, even though no key had (correctly) yet been set. Fix this by adding an extra check that we have a key. The redpine downstream out of tree driver does it this way too. Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group> CC: stable@vger.kernel.org --- drivers/net/wireless/rsi/rsi_91x_hal.c | 1 + 1 file changed, 1 insertion(+)