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
