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
