From: Jonas Gorski <jonas.gorski@gmail.com>
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
Date: Fri, 25 Aug 2023 10:49:08 +0200 [thread overview]
Message-ID: <CAOiHx=mo1WUYKKb5HtiPg0fiomem-QBqR2g9385tuEc-v_889A@mail.gmail.com> (raw)
In-Reply-To: <20230818014004.16177-1-pkshih@realtek.com>
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
next prev parent reply other threads:[~2023-08-25 8:50 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-18 1:40 [PATCH] wifi: mac80211: limit reorder_buf_filtered <=64 to avoid shift-out-of-bounds UBSAN warning Ping-Ke Shih
2023-08-25 8:49 ` Jonas Gorski [this message]
2023-08-25 9:09 ` Ping-Ke Shih
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOiHx=mo1WUYKKb5HtiPg0fiomem-QBqR2g9385tuEc-v_889A@mail.gmail.com' \
--to=jonas.gorski@gmail.com \
--cc=gregory.greenman@intel.com \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
--cc=pkshih@realtek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.