| Message ID | 20230817053215.51636-1-pkshih@realtek.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFC] wifi: mac80211: fix UBSAN warning caused by reorder_buf_filtered bits shift-out-of-bounds | expand |
Hi, > I mark this as RFC, because I'm not sure if iwlwifi needs to extend > ieee80211_mark_rx_ba_filtered_frames() to support mew hardware that > hw.max_rx_aggregation_subframes is larger than 64. Oh, good catch and question, but no. This firmware notification cannot appear in newer devices, and in fact we don't use mac80211 reordering at all for a long time (since RSS was introduced, basically) and should probably prevent handling of this notification. > If not, we can just > add some conditions to avoid UBSAN warning like this patch. Otherwise, > this RFC can't entirely resolve the problem. Seems fine. I'd kind of probably not word it as "fix UBSAN" since really it's just more along the lines of "warn if API is misused"? :) > Since only old hardware with 64 or less RX aggregation frames uses > ieee80211_mark_rx_ba_filtered_frames(), add a WARN_ONCE() and comment to > note to avoid using this function if hardware capability is not suitable. > > Cc: <Stable@vger.kernel.org> I don't really think this is stable material - if there's a driver that's calling this when >64 frames is supported then it's a driver bug that should be fixed, and if not then there's no bug? > +++ 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; Or maybe no - this part is what you think should be > > 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); And this. > @@ -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); > + From your description I was only thinking about this. So yeah I guess it does make some sense to actually call it a fix - for the parts about using the filtered value with >64 subframes supported ... Looks fine to me then! johannes
On Thu, 2023-08-17 at 09:03 +0200, Johannes Berg wrote: > > > > > If not, we can just > > add some conditions to avoid UBSAN warning like this patch. Otherwise, > > this RFC can't entirely resolve the problem. > > Seems fine. I'd kind of probably not word it as "fix UBSAN" since really > it's just more along the lines of "warn if API is misused"? :) Maybe, combine two? Because I want to avoid UBSAN warning initially. > > > Since only old hardware with 64 or less RX aggregation frames uses > > ieee80211_mark_rx_ba_filtered_frames(), add a WARN_ONCE() and comment to > > note to avoid using this function if hardware capability is not suitable. > > > > Cc: <Stable@vger.kernel.org> > > I don't really think this is stable material - if there's a driver > that's calling this when >64 frames is supported then it's a driver bug > that should be fixed, and if not then there's no bug? I'll remove this. > > > +++ 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; > > Or maybe no - this part is what you think should be This function will be called by all drivers that rely on mac80211's rx reordering. The UBSAN find `index` of `BIT_ULL(index)` could be over 64, for example index=215 when I set hw.max_rx_aggregation_subframes to 256. This is initial problem I want to fix. The `tid_agg_rx->reorder_buf_filtered != 0` means hw.max_rx_aggregation_subframes <= 64 (as well as index <= 64) implicitly, because only this kind of hardware can call ieee80211_mark_rx_ba_filtered_frames(). Maybe, below would be intuitive, but I worry it hides problem that UBSAN can't find. if (index < 64 && tid_agg_rx->reorder_buf_filtered & BIT_ULL(index)) return true; Any suggestion? or prefer? > > > 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); > > And this. The same as above. Thank you Ping-Ke
On Thu, 2023-08-17 at 12:06 +0000, Ping-Ke Shih wrote: > On Thu, 2023-08-17 at 09:03 +0200, Johannes Berg wrote: > > > > > > > > If not, we can just > > > add some conditions to avoid UBSAN warning like this patch. Otherwise, > > > this RFC can't entirely resolve the problem. > > > > Seems fine. I'd kind of probably not word it as "fix UBSAN" since really > > it's just more along the lines of "warn if API is misused"? :) > > Maybe, combine two? Because I want to avoid UBSAN warning initially. Yeah I understood later - never mind. > > > I don't really think this is stable material - if there's a driver > > that's calling this when >64 frames is supported then it's a driver bug > > that should be fixed, and if not then there's no bug? > > I'll remove this. Actually I don't know - do you have a driver that's actually supporting >64 BlockAck on stable? I initially missed entirely the fact that the UBSAN happens on the normal RX path, not (only) on the "mark as filtered" path. Sorry I didn't make that clear. > > > > > > +++ 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; > > > > Or maybe no - this part is what you think should be > > This function will be called by all drivers that rely on mac80211's rx reordering. Yeah. > The UBSAN find `index` of `BIT_ULL(index)` could be over 64, for example > index=215 when I set hw.max_rx_aggregation_subframes to 256. This is initial > problem I want to fix. Right, which I had missed initially in review. > The `tid_agg_rx->reorder_buf_filtered != 0` means hw.max_rx_aggregation_subframes <= 64 > (as well as index <= 64) implicitly, because only this kind of hardware can call > ieee80211_mark_rx_ba_filtered_frames(). Maybe, below would be intuitive, but > I worry it hides problem that UBSAN can't find. > > if (index < 64 && tid_agg_rx->reorder_buf_filtered & BIT_ULL(index)) > return true; > > Any suggestion? or prefer? I think what you did is fine. Just send the patch as it was? johannes
On Thu, 2023-08-17 at 14:30 +0200, Johannes Berg wrote: > > > > I don't really think this is stable material - if there's a driver > > > that's calling this when >64 frames is supported then it's a driver bug > > > that should be fixed, and if not then there's no bug? > > > > I'll remove this. > > Actually I don't know - do you have a driver that's actually supporting > > 64 BlockAck on stable? I don't have that one. Actually the code can work well, even though it has drawback found by UBSAN. So, I thought maybe some drivers work under that situation. > > Just send the patch as it was? > I will. Ping-Ke
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;
Hi, I mark this as RFC, because I'm not sure if iwlwifi needs to extend ieee80211_mark_rx_ba_filtered_frames() to support mew hardware that hw.max_rx_aggregation_subframes is larger than 64. If not, we can just add some conditions to avoid UBSAN warning like this patch. Otherwise, this RFC can't entirely resolve the problem. Summarize possible cases use case hw.max_rx_agg mark_rx_ba w/o this with this ---- ------------- ---------- ----------- --------- 1 > 64 o UBSAN warn UBSAN warn (no change still) WARN_ONCE() to note people don't use mark_rx_ba() 2 > 64 x UBSAN warn no UBSAN warn 3 <= 64 o work no change 4 <= 64 x work no change * hw.max_rx_agg is short for hw.max_rx_aggregation_subframes * mark_rx_ba is short for ieee80211_mark_rx_ba_filtered_frames() This RFC doesn't try to fix case 1, because the case may be not existing. Also, a possible implementation could be to declare 1024 bitmap for EHT hardware, and that has extra cost to shift this bitmap (compare with original u64). So, just give it a warning. Following is candidate of commit message if this RFC can be accepted: The commit 06470f7468c8 ("mac80211: add API to allow filtering frames in BA sessions") adds reorder_buf_filtered to mark frames filtered by firmware, but new hardware can support hw.max_rx_aggregation_subframes more than 64 frames. Then, it 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 with 64 or less RX aggregation frames uses ieee80211_mark_rx_ba_filtered_frames(), add a WARN_ONCE() and comment to note to avoid using this function if hardware capability is not suitable. Cc: <Stable@vger.kernel.org> 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(-)