[PATCH v2 0/2] wifi: Fix struct ieee80211_tim_ie::virtual_map

[PATCH v2 0/2] wifi: Fix struct ieee80211_tim_ie::virtual_map

[Jeff Johnson]

To align with [1] change struct ieee80211_tim_ie::virtual_map to be a
flexible array and fix all size references to account for the change
in struct size.

As a precursor, add a size check in a place where one is currently
missing.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
Changes in v2:
- Cover Letter
-     removed internal note
- [PATCH 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie
-     Fixed typo: s/no/now/
- Link to v1: https://lore.kernel.org/r/20230828-ieee80211_tim_ie-v1-0-6d7a4bab70ef@quicinc.com

---
Jeff Johnson (2):
      wifi: cw1200: Avoid processing an invalid TIM IE
      mac80211: Use flexible array in struct ieee80211_tim_ie

 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(-)
---
base-commit: 4dddbad8907bc2ecda6e3714de3ea0a27b90a7d3
change-id: 20230825-ieee80211_tim_ie-0391430af36d

[PATCH v2 1/2] wifi: cw1200: Avoid processing an invalid TIM IE

[Jeff Johnson]

While converting struct ieee80211_tim_ie::virtual_map to be a flexible
array it was observed that the TIM IE processing in cw1200_rx_cb()
could potentially process a malformed IE in a manner that could result
in a buffer over-read. Add logic to verify that the TIM IE length is
large enough to hold a valid TIM payload before processing it.

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/st/cw1200/txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/st/cw1200/txrx.c b/drivers/net/wireless/st/cw1200/txrx.c
index 6894b919ff94..e16e9ae90d20 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) {
+		if (tim_ie && tim_ie[1] >= sizeof(struct ieee80211_tim_ie)) {
 			struct ieee80211_tim_ie *tim =
 				(struct ieee80211_tim_ie *)&tim_ie[2];
 

-- 
2.25.1

[PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Jeff Johnson]

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(-)

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

Re: [PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Christian Lamparter]

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

Re: [PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Jeff Johnson]

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

Re: [PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Kees Cook]

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);
	};

-- 
Kees Cook

Re: [PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Sam James]

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

Re: [PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Kees Cook]

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

Re: [PATCH v2 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie

[Jeff Johnson]

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