| Message ID | 20230818014004.16177-1-pkshih@realtek.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mac80211: limit reorder_buf_filtered <=64 to avoid shift-out-of-bounds UBSAN warning | expand |
Hi, On Sun, 20 Aug 2023 at 13:20, Ping-Ke Shih <pkshih@realtek.com> 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@realtek.com> > --- > 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 > Regards, Jonas
> -----Original Message----- > From: Jonas Gorski <jonas.gorski@gmail.com> > Sent: Friday, August 25, 2023 4:49 PM > To: Ping-Ke Shih <pkshih@realtek.com> > Cc: johannes@sipsolutions.net; gregory.greenman@intel.com; linux-wireless@vger.kernel.org > 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@realtek.com> 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@realtek.com> > > --- > > 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@realtek.com/
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)) 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); 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); + if (!ieee80211_rx_data_set_sta(&rx, sta, -1)) return;
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@realtek.com> --- include/net/mac80211.h | 1 + net/mac80211/rx.c | 12 ++++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-)