| Message ID | 20230829-ieee80211_tim_ie-v2-2-fdaf19fb1c0e@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: Fix struct ieee80211_tim_ie::virtual_map | expand |
Hi, On 8/29/23 15:29, Jeff Johnson wrote: > Currently struct ieee80211_tim_ie defines: > u8 virtual_map[1]; > > Per the guidance in [1] change this to be a flexible array. > > As a result of this change, adjust all related struct size tests to > account for the fact that the sizeof(struct ieee80211_tim_ie) now > accounts for the minimum size of the virtual_map. > > [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays > > Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> > --- > diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h > index bd2f6e19c357..4cdc2eb98f16 100644 > --- a/include/linux/ieee80211.h > +++ b/include/linux/ieee80211.h > @@ -961,7 +961,7 @@ struct ieee80211_tim_ie { > u8 dtim_period; > u8 bitmap_ctrl; > /* variable size: 1 - 251 bytes */ > - u8 virtual_map[1]; > + u8 virtual_map[]; > } __packed; Uhh, the 802.11 (my 2012 Version has this in) spec in 8.4.2.7 TIM Element demands this to be 1 - 251 bytes. And this is why there's a comment above... With your change this could be confusing. Would it be possible to fix that somehow? Like in a anonymous union/group with a flexible array and a u8? Cheers, Christian
On 8/30/2023 12:51 PM, Christian Lamparter wrote: > Hi, > > On 8/29/23 15:29, Jeff Johnson wrote: >> Currently struct ieee80211_tim_ie defines: >> u8 virtual_map[1]; >> >> Per the guidance in [1] change this to be a flexible array. >> >> As a result of this change, adjust all related struct size tests to >> account for the fact that the sizeof(struct ieee80211_tim_ie) now >> accounts for the minimum size of the virtual_map. >> >> [1] >> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays >> >> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> >> --- >> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h >> index bd2f6e19c357..4cdc2eb98f16 100644 >> --- a/include/linux/ieee80211.h >> +++ b/include/linux/ieee80211.h >> @@ -961,7 +961,7 @@ struct ieee80211_tim_ie { >> u8 dtim_period; >> u8 bitmap_ctrl; >> /* variable size: 1 - 251 bytes */ >> - u8 virtual_map[1]; >> + u8 virtual_map[]; >> } __packed; > > > Uhh, the 802.11 (my 2012 Version has this in) spec in > 8.4.2.7 TIM Element demands this to be 1 - 251 bytes. > And this is why there's a comment above... With your > change this could be confusing. Would it be possible > to fix that somehow? Like in a anonymous union/group > with a flexible array and a u8? Adding Kees to the discussion for any advice. Yes, the virtual_map must contain at least one octet but may contain more than one. And to complicate matters, the information that tells us how many octets are actually present is found outside the struct; the TLV header that precedes the struct will contain the length of the struct, and hence the length of the bitmap is that size - 2 (the size of the dtim_period and bitmap_ctrl fields). I don't think it is unique to this struct that an 802.11 element will have optional octets for which one or more octets must always be present. For that reason I've been updating ieee80211.h kdoc to point to the latest specification since that ultimately provides the necessary guidance. /jeff
On 8/30/2023 12:51 PM, Christian Lamparter wrote: > Hi, > > On 8/29/23 15:29, Jeff Johnson wrote: >> Currently struct ieee80211_tim_ie defines: >> u8 virtual_map[1]; >> >> Per the guidance in [1] change this to be a flexible array. >> >> As a result of this change, adjust all related struct size tests to >> account for the fact that the sizeof(struct ieee80211_tim_ie) now >> accounts for the minimum size of the virtual_map. >> >> [1] >> https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays >> >> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> >> --- >> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h >> index bd2f6e19c357..4cdc2eb98f16 100644 >> --- a/include/linux/ieee80211.h >> +++ b/include/linux/ieee80211.h >> @@ -961,7 +961,7 @@ struct ieee80211_tim_ie { >> u8 dtim_period; >> u8 bitmap_ctrl; >> /* variable size: 1 - 251 bytes */ >> - u8 virtual_map[1]; >> + u8 virtual_map[]; >> } __packed; > > > Uhh, the 802.11 (my 2012 Version has this in) spec in > 8.4.2.7 TIM Element demands this to be 1 - 251 bytes. > And this is why there's a comment above... With your > change this could be confusing. Would it be possible > to fix that somehow? Like in a anonymous union/group > with a flexible array and a u8? Adding Kees to the discussion for any advice. Yes, the virtual_map must contain at least one octet but may contain more than one. And to complicate matters, the information that tells us how many octets are actually present is found outside the struct; the TLV header that precedes the struct will contain the length of the struct, and hence the length of the bitmap is that size - 2 (the size of the dtim_period and bitmap_ctrl fields). I don't think it is unique to this struct that an 802.11 element will have optional octets for which one or more octets must always be present. For that reason I've been updating ieee80211.h kdoc to point to the latest specification since that ultimately provides the necessary guidance. /jeff
On Wed, Aug 30, 2023 at 01:22:37PM -0700, Jeff Johnson wrote: > On 8/30/2023 12:51 PM, Christian Lamparter wrote: > > Hi, > > > > On 8/29/23 15:29, Jeff Johnson wrote: > > > Currently struct ieee80211_tim_ie defines: > > > u8 virtual_map[1]; > > > > > > Per the guidance in [1] change this to be a flexible array. > > > > > > As a result of this change, adjust all related struct size tests to > > > account for the fact that the sizeof(struct ieee80211_tim_ie) now > > > accounts for the minimum size of the virtual_map. > > > > > > [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays > > > > > > Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> > > > --- > > > diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h > > > index bd2f6e19c357..4cdc2eb98f16 100644 > > > --- a/include/linux/ieee80211.h > > > +++ b/include/linux/ieee80211.h > > > @@ -961,7 +961,7 @@ struct ieee80211_tim_ie { > > > u8 dtim_period; > > > u8 bitmap_ctrl; > > > /* variable size: 1 - 251 bytes */ > > > - u8 virtual_map[1]; > > > + u8 virtual_map[]; > > > } __packed; > > > > > > Uhh, the 802.11 (my 2012 Version has this in) spec in > > 8.4.2.7 TIM Element demands this to be 1 - 251 bytes. > > And this is why there's a comment above... With your > > change this could be confusing. Would it be possible > > to fix that somehow? Like in a anonymous union/group > > with a flexible array and a u8? > > Adding Kees to the discussion for any advice. Yes, the virtual_map must > contain at least one octet but may contain more than one. And to complicate > matters, the information that tells us how many octets are actually present > is found outside the struct; the TLV header that precedes the struct will > contain the length of the struct, and hence the length of the bitmap is that > size - 2 (the size of the dtim_period and bitmap_ctrl fields). Bummer about the count variable being elsewhere, but we'll deal with that later. :) For the array declaration, though, yes, we can do a "minimum size 1" like this: union { u8 required_byte; DECLARE_FLEX_ARRAY(u8, virtual_map); };
I think I've just hit this, unless it's been fixed since and it's just similar. ``` [ 291.051876] ================================================================================ [ 291.051892] UBSAN: array-index-out-of-bounds in /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.30/work/linux-6.6/include/linux/ieee80211.h:4455:28 [ 291.051901] index 1 is out of range for type 'u8 [1]' [ 291.051908] CPU: 2 PID: 627 Comm: kworker/2:3 Not tainted 6.6.30-gentoo-dist-hardened #1 [ 291.051917] Hardware name: ASUSTeK COMPUTER INC. UX305FA/UX305FA, BIOS UX305FA.216 04/17/2019 [ 291.051922] Workqueue: events cfg80211_wiphy_work [cfg80211] [ 291.052082] Call Trace: [ 291.052088] <TASK> [ 291.052096] dump_stack_lvl (lib/dump_stack.c:107) [ 291.052114] __ubsan_handle_out_of_bounds (lib/ubsan.c:218 (discriminator 1) lib/ubsan.c:348 (discriminator 1)) [ 291.052130] ieee80211_rx_mgmt_beacon (include/linux/ieee80211.h:4455 net/mac80211/mlme.c:6047) mac80211 [ 291.052354] ? check_preempt_wakeup (kernel/sched/fair.c:977 kernel/sched/fair.c:8226) [ 291.052368] ? check_preempt_curr (kernel/sched/core.c:2232) [ 291.052375] ? ttwu_do_activate (kernel/sched/core.c:3766 (discriminator 2) kernel/sched/core.c:3794 (discriminator 2)) [ 291.052383] ? __mutex_lock.constprop.0 (kernel/locking/mutex.c:489 kernel/locking/mutex.c:607 kernel/locking/mutex.c:747) [ 291.052393] ieee80211_sta_rx_queued_mgmt (net/mac80211/mlme.c:6288) mac80211 [ 291.052599] ? finish_task_switch.isra.0 (arch/x86/include/asm/paravirt.h:700 kernel/sched/sched.h:1386 kernel/sched/core.c:5138 kernel/sched/core.c:5256) [ 291.052613] ieee80211_iface_work (net/mac80211/iface.c:1602 net/mac80211/iface.c:1658) mac80211 [ 291.052792] ? __pm_runtime_suspend (drivers/base/power/runtime.c:1128) [ 291.052807] cfg80211_wiphy_work (include/net/cfg80211.h:5789 net/wireless/core.c:442) cfg80211 [ 291.052889] process_one_work (kernel/workqueue.c:2632) [ 291.052894] worker_thread (kernel/workqueue.c:2694 (discriminator 2) kernel/workqueue.c:2781 (discriminator 2)) [ 291.052897] ? __pfx_worker_thread (kernel/workqueue.c:2727) [ 291.052900] kthread (kernel/kthread.c:388) [ 291.052905] ? __pfx_kthread (kernel/kthread.c:341) [ 291.052909] ret_from_fork (arch/x86/kernel/process.c:153) [ 291.052913] ? __pfx_kthread (kernel/kthread.c:341) [ 291.052917] ret_from_fork_asm (arch/x86/entry/entry_64.S:314) [ 291.052922] </TASK> [ 291.052923] ================================================================================ ``` I can reproduce it fairly easily when changing wifi adapters and toggling connecting to an AP. (It was a fun mini-adventure to get the trace usable and I should send some patches to decode_stacktrace.sh, I think...) thanks, sam
On Tue, May 14, 2024 at 05:51:02AM +0100, Sam James wrote: > I think I've just hit this, unless it's been fixed since and it's just > similar. > > ``` > [ 291.051876] ================================================================================ > [ 291.051892] UBSAN: array-index-out-of-bounds in /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.30/work/linux-6.6/include/linux/ieee80211.h:4455:28 > [ 291.051901] index 1 is out of range for type 'u8 [1]' > [ 291.051908] CPU: 2 PID: 627 Comm: kworker/2:3 Not tainted 6.6.30-gentoo-dist-hardened #1 > [ 291.051917] Hardware name: ASUSTeK COMPUTER INC. UX305FA/UX305FA, BIOS UX305FA.216 04/17/2019 > [ 291.051922] Workqueue: events cfg80211_wiphy_work [cfg80211] > [ 291.052082] Call Trace: > [ 291.052088] <TASK> > [ 291.052096] dump_stack_lvl (lib/dump_stack.c:107) > [ 291.052114] __ubsan_handle_out_of_bounds (lib/ubsan.c:218 (discriminator 1) lib/ubsan.c:348 (discriminator 1)) > [ 291.052130] ieee80211_rx_mgmt_beacon (include/linux/ieee80211.h:4455 net/mac80211/mlme.c:6047) mac80211 This looks like it's this line in ieee80211_rx_mgmt_beacon(): ieee80211_check_tim(elems->tim, elems->tim_len, vif_cfg->aid)) { which is: static inline bool ieee80211_check_tim(const struct ieee80211_tim_ie *tim, u8 tim_len, u16 aid) { ... return !!(tim->virtual_map[index] & mask); ^^^^^^^^^^^^^^^^^^^^^^^ } UBSAN says it's because the array is defined as "virtual_map[1]": struct ieee80211_tim_ie { u8 dtim_count; u8 dtim_period; u8 bitmap_ctrl; /* variable size: 1 - 251 bytes */ u8 virtual_map[1]; } __packed; This was fixed in commit 2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie") which was part of the v6.7 release. > (It was a fun mini-adventure to get the trace usable and I should send > some patches to decode_stacktrace.sh, I think...) Please do! :)
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c index 0c0624a3b40d..2a263a1b7fbf 100644 --- a/drivers/net/wireless/ath/ath9k/recv.c +++ b/drivers/net/wireless/ath/ath9k/recv.c @@ -520,7 +520,7 @@ static bool ath_beacon_dtim_pending_cab(struct sk_buff *skb) break; if (id == WLAN_EID_TIM) { - if (elen < sizeof(*tim)) + if (elen <= sizeof(*tim)) break; tim = (struct ieee80211_tim_ie *) pos; if (tim->dtim_count != 0) diff --git a/drivers/net/wireless/ath/carl9170/rx.c b/drivers/net/wireless/ath/carl9170/rx.c index 908c4c8b7f82..5bdbde8c98a3 100644 --- a/drivers/net/wireless/ath/carl9170/rx.c +++ b/drivers/net/wireless/ath/carl9170/rx.c @@ -542,7 +542,7 @@ static void carl9170_ps_beacon(struct ar9170 *ar, void *data, unsigned int len) if (!tim) return; - if (tim[1] < sizeof(*tim_ie)) + if (tim[1] <= sizeof(*tim_ie)) return; tim_len = tim[1]; diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c index 9a9cfd0ce402..f594835da7ce 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c @@ -669,7 +669,7 @@ static void rt2x00lib_rxdone_check_ps(struct rt2x00_dev *rt2x00dev, if (!tim) return; - if (tim[1] < sizeof(*tim_ie)) + if (tim[1] <= sizeof(*tim_ie)) return; tim_len = tim[1]; diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c b/drivers/net/wireless/realtek/rtlwifi/ps.c index 629c03271bde..ea4b055bc6d8 100644 --- a/drivers/net/wireless/realtek/rtlwifi/ps.c +++ b/drivers/net/wireless/realtek/rtlwifi/ps.c @@ -506,7 +506,7 @@ void rtl_swlps_beacon(struct ieee80211_hw *hw, void *data, unsigned int len) if (!tim) return; - if (tim[1] < sizeof(*tim_ie)) + if (tim[1] <= sizeof(*tim_ie)) return; tim_len = tim[1]; diff --git a/drivers/net/wireless/st/cw1200/txrx.c b/drivers/net/wireless/st/cw1200/txrx.c index e16e9ae90d20..c2a51cd79ab8 100644 --- a/drivers/net/wireless/st/cw1200/txrx.c +++ b/drivers/net/wireless/st/cw1200/txrx.c @@ -1166,7 +1166,7 @@ void cw1200_rx_cb(struct cw1200_common *priv, size_t ies_len = skb->len - (ies - (u8 *)(skb->data)); tim_ie = cfg80211_find_ie(WLAN_EID_TIM, ies, ies_len); - if (tim_ie && tim_ie[1] >= sizeof(struct ieee80211_tim_ie)) { + if (tim_ie && tim_ie[1] > sizeof(struct ieee80211_tim_ie)) { struct ieee80211_tim_ie *tim = (struct ieee80211_tim_ie *)&tim_ie[2]; diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h index bd2f6e19c357..4cdc2eb98f16 100644 --- a/include/linux/ieee80211.h +++ b/include/linux/ieee80211.h @@ -961,7 +961,7 @@ struct ieee80211_tim_ie { u8 dtim_period; u8 bitmap_ctrl; /* variable size: 1 - 251 bytes */ - u8 virtual_map[1]; + u8 virtual_map[]; } __packed; /** @@ -4405,7 +4405,7 @@ static inline bool ieee80211_check_tim(const struct ieee80211_tim_ie *tim, u8 mask; u8 index, indexn1, indexn2; - if (unlikely(!tim || tim_len < sizeof(*tim))) + if (unlikely(!tim || tim_len <= sizeof(*tim))) return false; aid &= 0x3fff; diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 8a6917cf63cf..0c23223bb030 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -1123,7 +1123,7 @@ _ieee802_11_parse_elems_full(struct ieee80211_elems_parse_params *params, elem_parse_failed = true; break; case WLAN_EID_TIM: - if (elen >= sizeof(struct ieee80211_tim_ie)) { + if (elen > sizeof(struct ieee80211_tim_ie)) { elems->tim = (void *)pos; elems->tim_len = elen; } else
Currently struct ieee80211_tim_ie defines: u8 virtual_map[1]; Per the guidance in [1] change this to be a flexible array. As a result of this change, adjust all related struct size tests to account for the fact that the sizeof(struct ieee80211_tim_ie) now accounts for the minimum size of the virtual_map. [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> --- drivers/net/wireless/ath/ath9k/recv.c | 2 +- drivers/net/wireless/ath/carl9170/rx.c | 2 +- drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 2 +- drivers/net/wireless/realtek/rtlwifi/ps.c | 2 +- drivers/net/wireless/st/cw1200/txrx.c | 2 +- include/linux/ieee80211.h | 4 ++-- net/mac80211/util.c | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-)