Re: [PATCH] rsi: fix broken AP mode due to unwanted encryption

[Marek Vasut]

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@xxxxxxx> wrote:
> > > Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> > > CC: stable@xxxxxxxxxxxxxxx
> >
> > 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