> -----Original Message-----
> From: Jonas Gorski <jonas.gorski@xxxxxxxxx>
> Sent: Friday, August 25, 2023 4:49 PM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: johannes@xxxxxxxxxxxxxxxx; gregory.greenman@xxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] wifi: mac80211: limit reorder_buf_filtered <=64 to avoid shift-out-of-bounds UBSAN
> warning
>
> Hi,
>
> On Sun, 20 Aug 2023 at 13:20, Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:
> >
> > The commit 06470f7468c8 ("mac80211: add API to allow filtering frames in BA sessions")
> > adds reorder_buf_filtered to mark frames filtered by firmware, and it can
> > only work correctly if hw.max_rx_aggregation_subframes <= 64 because
> > maximum BlockAck is 64 at that moment.
> >
> > However, new HE or EHT devices can support BlockAck number up to 256 or
> > 1024, and leads UBSAN warning:
> >
> > UBSAN: shift-out-of-bounds in net/mac80211/rx.c:1129:39
> > shift exponent 215 is too large for 64-bit type 'long long unsigned int'
> > Call Trace:
> > <IRQ>
> > dump_stack_lvl+0x48/0x70
> > dump_stack+0x10/0x20
> > __ubsan_handle_shift_out_of_bounds+0x1ac/0x360
> > ieee80211_release_reorder_frame.constprop.0.cold+0x64/0x69 [mac80211]
> > ieee80211_sta_reorder_release+0x9c/0x400 [mac80211]
> > ieee80211_prepare_and_rx_handle+0x1234/0x1420 [mac80211]
> > ? __pfx_jhash+0x10/0x10
> > ? rht_key_get_hash.isra.0+0x19/0x30 [mac80211]
> > ieee80211_rx_list+0xaef/0xf60 [mac80211]
> > ? kfree_skbmem+0x58/0xb0
> > ? rtw89_vif_rx_stats_iter+0x2bb/0x2e1 [rtw89_core]
> > ieee80211_rx_napi+0x53/0xd0 [mac80211]
> >
> > Since only old hardware that supports <=64 BlockAck uses
> > ieee80211_mark_rx_ba_filtered_frames(), limit the use as it is, so add a
> > WARN_ONCE() and comment to note to avoid using this function if hardware
> > capability is not suitable.
> >
> > Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> > ---
> > include/net/mac80211.h | 1 +
> > net/mac80211/rx.c | 12 ++++++++++--
> > 2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 3a8a2d2c58c3..2a55ae932c56 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -6612,6 +6612,7 @@ void ieee80211_stop_rx_ba_session(struct ieee80211_vif *vif, u16 ba_rx_bitmap,
> > * marks frames marked in the bitmap as having been filtered. Afterwards, it
> > * checks if any frames in the window starting from @ssn can now be released
> > * (in case they were only waiting for frames that were filtered.)
> > + * (Only work correctly if @max_rx_aggregation_subframes <= 64 frames)
> > */
> > void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
> > u16 ssn, u64 filtered,
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 4f707d2a160f..0af2599c17e8 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -1083,7 +1083,8 @@ static inline bool ieee80211_rx_reorder_ready(struct tid_ampdu_rx *tid_agg_rx,
> > struct sk_buff *tail = skb_peek_tail(frames);
> > struct ieee80211_rx_status *status;
> >
> > - if (tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
> > + if (tid_agg_rx->reorder_buf_filtered &&
> > + tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
>
> While it will silence the UBSAN warning, unless you know why the code
> was written this way it will look like a pointless micro-optimization.
>
> So I suggest changing the condition to
>
> if (index < BITS_PER_LONG_LONG &&
> tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
>
> to make it more obvious what the intention of the extra condition is.
>
> > return true;
> >
> > if (!tail)
> > @@ -1124,7 +1125,8 @@ static void ieee80211_release_reorder_frame(struct ieee80211_sub_if_data *sdata,
> > }
> >
> > no_frame:
> > - tid_agg_rx->reorder_buf_filtered &= ~BIT_ULL(index);
> > + if (tid_agg_rx->reorder_buf_filtered)
> > + tid_agg_rx->reorder_buf_filtered &= ~BIT_ULL(index);
>
> likewise
>
> > tid_agg_rx->head_seq_num = ieee80211_sn_inc(tid_agg_rx->head_seq_num);
> > }
> >
> > @@ -4264,6 +4266,7 @@ void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
> > u16 ssn, u64 filtered,
> > u16 received_mpdus)
> > {
> > + struct ieee80211_local *local;
> > struct sta_info *sta;
> > struct tid_ampdu_rx *tid_agg_rx;
> > struct sk_buff_head frames;
> > @@ -4281,6 +4284,11 @@ void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
> >
> > sta = container_of(pubsta, struct sta_info, sta);
> >
> > + local = sta->sdata->local;
> > + WARN_ONCE(local->hw.max_rx_aggregation_subframes > 64,
> > + "RX BA marker can't support max_rx_aggregation_subframes %u > 64\n",
> > + local->hw.max_rx_aggregation_subframes);
>
> And maybe use BITS_PER_LONG_LONG here as well.
>
> Or introduce your own macro. Not sure what's nicer.
>
> > +
> > if (!ieee80211_rx_data_set_sta(&rx, sta, -1))
> > return;
> >
> > --
> > 2.25.1
> >
Please reference to RFC discussion [1] that mentioned your questions.
Ping-Ke
[1] https://lore.kernel.org/linux-wireless/3db092ea0aa6b758e23df577f415f142e82776a2.camel@xxxxxxxxxxx/
