Skip to contentSkip to conversation

wifi: brcmfmac: p2p: Introduce generic flexible array frame member

PATCH In review Kees Cook 2 participants 2 messages Feb 15, 2023 – Feb 16, 2023

Subsystems (4)
Networking Network drivers 802.11 (including cfg80211/NL80211) Broadcom BRCM80211 IEEE802.11 wireless drivers
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 wifi: brcmfmac: p2p: Introduce generic flexible array frame member

Silence run-time memcpy() false positive warning when processing management frames:

memcpy: detected field-spanning write (size 27) of single field "&mgmt_frame->u" at drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c:1469 (size 26)

Due to this (soon to be fixed) GCC bug[1], FORTIFY_SOURCE (via __builtin_dynamic_object_size) doesn't recognize that the union may end with a flexible array, and returns "26" (the fixed size of the union), rather than the remaining size of the allocation. Add an explicit flexible array member and set it as the destination here, so that we get the correct coverage for the memcpy().

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832

Reported-by Ard Biesheuvel <[email protected]>
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Johannes Berg <[email protected]>
Cc "Jason A. Donenfeld" <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc "Darrick J. Wong" <[email protected]>
Cc Colin Ian King <[email protected]>
Cc Brian Henriquez <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 4 ++--
 include/linux/ieee80211.h                              | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c +2 -2
index e975d10e6009..4d0db7107e65 100644
@@ -1466,8 +1466,8 @@ int brcmf_p2p_notify_action_frame_rx(struct brcmf_if *ifp,
 			       ETH_ALEN);
 	memcpy(mgmt_frame->sa, e->addr, ETH_ALEN);
 	mgmt_frame->frame_control = cpu_to_le16(IEEE80211_STYPE_ACTION);
-	memcpy(&mgmt_frame->u, frame, mgmt_frame_len);
-	mgmt_frame_len += offsetof(struct ieee80211_mgmt, u);
+	memcpy(mgmt_frame->u.frame, frame, mgmt_frame_len);
+	mgmt_frame_len += offsetof(struct ieee80211_mgmt, u.frame);
 
 	freq = ieee80211_channel_to_frequency(ch.control_ch_num,
 					      ch.band == BRCMU_CHAN_BAND_2G ?
include/linux/ieee80211.h +1 -0
index 80d6308dea06..990d204d918a 100644
@@ -1356,6 +1356,7 @@ struct ieee80211_mgmt {
 				} __packed wnm_timing_msr;
 			} u;
 		} __packed action;
+		DECLARE_FLEX_ARRAY(u8, frame); /* Generic frame contents. */
 	} u;
 } __packed __aligned(2);
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 26 lines · Kees Cook

Silence run-time memcpy() false positive warning when processing management frames:

memcpy: detected field-spanning write (size 27) of single field "&mgmt_frame->u" at drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c:1469 (size 26)

Due to this (soon to be fixed) GCC bug[1], FORTIFY_SOURCE (via __builtin_dynamic_object_size) doesn't recognize that the union may end with a flexible array, and returns "26" (the fixed size of the union), rather than the remaining size of the allocation. Add an explicit flexible array member and set it as the destination here, so that we get the correct coverage for the memcpy().

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832

Reported-by Ard Biesheuvel <[email protected]>
Cc Arend van Spriel <[email protected]>
Cc Franky Lin <[email protected]>
Cc Hante Meuleman <[email protected]>
Cc Kalle Valo <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Johannes Berg <[email protected]>
Cc "Jason A. Donenfeld" <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Quoted 8 lines
Cc "Darrick J. Wong" <[email protected]>
Cc Colin Ian King <[email protected]>
Cc Brian Henriquez <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Us having to deal with this kind of stuff is why we need Rust.

Thanks for doing this Kees,

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij