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

[PATCH v3 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.

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 v3:
- [PATCH 2/2] mac80211: Use flexible array in struct ieee80211_tim_ie
-     Complete rewrite based upon v2 discussion. As a result no driver changes are required
- Link to v2: https://lore.kernel.org/r/20230829-ieee80211_tim_ie-v2-0-fdaf19fb1c0e@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/st/cw1200/txrx.c |  2 +-
 include/linux/ieee80211.h             | 13 ++++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)
---
base-commit: b32add2d20ea6e62f30a3c0a7c2fb306ec5ceb3d
change-id: 20230825-ieee80211_tim_ie-0391430af36d

[PATCH v3 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.42.0

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

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> wrote:

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

Patch applied to wireless-next.git, thanks.

b7bcea9c27b3 wifi: cw1200: Avoid processing an invalid TIM IE

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230831-ieee80211_tim_ie-v3-1-e10ff584ab5d@quicinc.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v3 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.

Per the discussion in [2] wrap the virtual_map in a union with a u8
item in order to preserve the existing expectation that the
virtual_map must contain at least one octet (at least when used in a
non-S1G PPDU). This means that no driver changes are required.

[1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
[2] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 include/linux/ieee80211.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index bd2f6e19c357..340d7e0f6bf7 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -951,17 +951,24 @@ struct ieee80211_wide_bw_chansw_ie {
  * @dtim_count: DTIM Count
  * @dtim_period: DTIM Period
  * @bitmap_ctrl: Bitmap Control
+ * @required_octet: "Syntatic sugar" to force the struct size to the
+ *                  minimum valid size when carried in a non-S1G PPDU
  * @virtual_map: Partial Virtual Bitmap
  *
  * This structure represents the payload of the "TIM element" as
- * described in IEEE Std 802.11-2020 section 9.4.2.5.
+ * described in IEEE Std 802.11-2020 section 9.4.2.5. Note that this
+ * definition is only applicable when the element is carried in a
+ * non-S1G PPDU. When the TIM is carried in an S1G PPDU, the Bitmap
+ * Control and Partial Virtual Bitmap may not be present.
  */
 struct ieee80211_tim_ie {
 	u8 dtim_count;
 	u8 dtim_period;
 	u8 bitmap_ctrl;
-	/* variable size: 1 - 251 bytes */
-	u8 virtual_map[1];
+	union {
+		u8 required_octet;
+		DECLARE_FLEX_ARRAY(u8, virtual_map);
+	};
 } __packed;
 
 /**

-- 
2.42.0

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

[Kees Cook]

On Thu, Aug 31, 2023 at 11:22:58AM -0700, 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.
> 
> Per the discussion in [2] wrap the virtual_map in a union with a u8
> item in order to preserve the existing expectation that the
> virtual_map must contain at least one octet (at least when used in a
> non-S1G PPDU). This means that no driver changes are required.
> 
> [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays
> [2] https://lore.kernel.org/linux-wireless/202308301529.AC90A9EF98@keescook/
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

Looks good to me! Thanks for the conversion. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook