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 <[email protected]>
---
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(-)
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
--
2.25.1
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! :)
--
Kees Cook