Re: [PATCH v2 11/12] mac80211: drop data frames without key on encrypted links

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2021-06-29 at 23:32 +0200, Pali Rohár wrote:
> On Wednesday 23 June 2021 14:16:12 Johannes Berg wrote:
> > On Fri, 2021-06-11 at 12:10 +0200, Pali Rohár wrote:
> > > 
> > > @@ -341,8 +341,11 @@ static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
> > >  	if (sta) {
> > >  		if (pairwise) {
> > >  			rcu_assign_pointer(sta->ptk[idx], new);
> > > -			sta->ptk_idx = idx;
> > > -			ieee80211_check_fast_xmit(sta);
> > > +			if (new) {
> > > +				set_sta_flag(new->sta, WLAN_STA_USES_ENCRYPTION);
> > > +				new->sta->ptk_idx = new->conf.keyidx;
> > 
> > I'm not entirely sure moving that assignment under the guard is correct.
> > 
> > > +				ieee80211_check_fast_xmit(new->sta);
> > 
> > and I'm pretty sure that moving call under the guard is incorrect,
> > although in the end it probably doesn't even matter if we will drop all
> > frames anyway (due to this patch).
> > 
> > So all you need under the assignment is the flag, but also only
> > theoretically, because the function cannot be called with old==NULL &&
> > new==NULL, the first time around it's called we must have old==NULL (no
> > key was ever installed), and so the first time it's called it must be
> > old==NULL && new!=NULL, and then the flag gets set and we never want to
> > clear it again, so I believe you don't need the "if (new)" condition at
> > all.
> > 
> > In the code as it was in (and before) my patch the condition is
> > necessary because we use 'new' to obtain the 'sta' and 'local' pointers,
> > but otherwise we don't really need it even in the current version.
> > 
> > johannes
> 
> Now I see, thank you for explanation. So the code should be like this:
> 
>  		if (pairwise) {
>  			rcu_assign_pointer(sta->ptk[idx], new);
> +			set_sta_flag(sta, WLAN_STA_USES_ENCRYPTION);
>  			sta->ptk_idx = idx;
>  			ieee80211_check_fast_xmit(sta);
>  		} else {
> 
> Right?

Yes, I think so.

johannes