rsi: fix broken AP mode due to unwanted encryption

Commit Message

Fuzzey, Martin May 28, 2021, 5:17 p.m. UTC

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 <[e-mail]>
CC: [e-mail]
---
 drivers/net/wireless/rsi/rsi_91x_hal.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Marek Vasut May 28, 2021, 6:11 p.m. UTC | #1

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 <[e-mail]>
> CC: [e-mail]

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 <[e-mail]>

Fuzzey, Martin June 1, 2021, 9:02 a.m. UTC | #2

Hi Marek,
thanks for the review.

On Fri, 28 May 2021 at 20:11, Marek Vasut <[e-mail]> wrote:
>
> > Signed-off-by: Martin Fuzzey <[e-mail]>
> > CC: [e-mail]
>
> 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 <[e-mail]>

Seeing as the V2 will be a bit different I won't add that yet.

Martin

Marek Vasut June 1, 2021, 11:24 a.m. UTC | #3

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 <[e-mail]> wrote:
>>
>>> Signed-off-by: Martin Fuzzey <[e-mail]>
>>> CC: [e-mail]
>>
>> 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

Patch

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;