Re: [PATCH 1/2] wifi: mac80211: Update skb's NULL key in ieee80211_tx_h_select_key()

[Remi Pommarel <repk@triplefau.lt>]

On Mon, Mar 24, 2025 at 03:02:48PM +0100, Remi Pommarel wrote:
> On Mon, Mar 24, 2025 at 01:17:08PM +0100, Johannes Berg wrote:
> > On Fri, 2025-03-14 at 12:04 +0100, Remi Pommarel wrote:
> > > The ieee80211 skb control block key (set when skb was queued) could have
> > > been removed before ieee80211_tx_dequeue() call. ieee80211_tx_dequeue()
> > > already called ieee80211_tx_h_select_key() to get the current key, but
> > > the latter do not update the key in skb control block in case it is
> > > NULL. Because some drivers actually use this key in their TX callbacks
> > > (e.g. ath1{1,2}k_mac_op_tx()) this could lead to the use after free
> > > below:
> > > 
> > >   BUG: KASAN: slab-use-after-free in ath11k_mac_op_tx+0x590/0x61c
> > >   Read of size 4 at addr ffffff803083c248 by task kworker/u16:4/1440
> > 
> > 
> > Maybe should have a Fixes: tag?
> 
> Finding a fix tag is not easy for this case because I am not sure which
> commit exactly introduced the issue. Is it the introduction of
> ieee80211_handle_wake_tx_queue() (i.e. c850e31f79f0) that allows packets
> queued on another dev to be processed or the one that introduced
> ieee80211_tx_dequeue() (i.e.  bb42f2d13ffc) ?
> 
> I would have said the latter, what do you think ?
> 
> > 
> > And please also tag the subject "[PATCH wireless NN/MM]".
> 
> Sure I have seen the new subject tag discussion too late unfortunately.
> 
> > 
> > > +++ b/net/mac80211/tx.c
> > > @@ -668,6 +668,12 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
> > >  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> > >  		return TX_DROP;
> > > +	} else {
> > > +		/* Clear SKB CB key reference, ieee80211_tx_h_select_key()
> > > +		 * could have been called to update key info after its removal
> > > +		 * (e.g. by ieee80211_tx_dequeue()).
> > > +		 */
> > > +		info->control.hw_key = NULL;
> > >  	}
> > 
> > I'm not sure this looks like the right place - should probably be done
> > around line 3897 before the call:
> > 
> >         /*
> >          * The key can be removed while the packet was queued, so need to call
> >          * this here to get the current key.
> >          */
> >         r = ieee80211_tx_h_select_key(&tx);
> > 
> > 
> > I'd think?
> 
> I initially did that, but because I ended up with the following:
> 
> +	info.control.hw_key = (tx->key) ? &tx->key.conf: NULL;
> 
> I found it more readable to do that directly in
> ieee80211_tx_h_select_key(). But I don't have strong feeling about that.
> So both ways are fine with me, let me know which one like the most ?

Oh sorry, you meant to initialize to NULL *before* the call to
ieee80211_tx_h_select_key(), sure will do that instead.

-- 
Remi