Hi Marek,
thanks for the review.
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.
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@xxxxxxx>
Seeing as the V2 will be a bit different I won't add that yet.
Martin
