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 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 ?

-- 
Remi