[v2,44/63] mac80211: Use memset_after() to clear tx status

Message ID	20210818060533.3569517-45-keescook@chromium.org
State	New
Headers	show Return-Path: <netdev-owner@kernel.org> From: Kees Cook <keescook@chromium.org> To: linux-kernel@vger.kernel.org Cc: Kees Cook <keescook@chromium.org>, Johannes Berg <johannes@sipsolutions.net>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, linux-wireless@vger.kernel.org, netdev@vger.kernel.org, "Gustavo A. R. Silva" <gustavoars@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Andrew Morton <akpm@linux-foundation.org>, dri-devel@lists.freedesktop.org, linux-staging@lists.linux.dev, linux-block@vger.kernel.org, linux-kbuild@vger.kernel.org, clang-built-linux@googlegroups.com, Rasmus Villemoes <linux@rasmusvillemoes.dk>, linux-hardening@vger.kernel.org Subject: [PATCH v2 44/63] mac80211: Use memset_after() to clear tx status Date: Tue, 17 Aug 2021 23:05:14 -0700 Message-Id: <20210818060533.3569517-45-keescook@chromium.org> In-Reply-To: <20210818060533.3569517-1-keescook@chromium.org> References: <20210818060533.3569517-1-keescook@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Introduce strict memcpy() bounds checking \| expand [v2,00/63] Introduce strict memcpy() bounds checking [v2,01/63] ipw2x00: Avoid field-overflowing memcpy() [v2,02/63] net/mlx5e: Avoid field-overflowing memcpy() [v2,03/63] rpmsg: glink: Replace strncpy() with strscpy_pad() [v2,04/63] pcmcia: ray_cs: Split memcpy() to avoid bounds check warning [v2,05/63] stddef: Introduce struct_group() helper macro [v2,06/63] cxl/core: Replace unions with struct_group() [v2,07/63] skbuff: Switch structure bounds to struct_group() [v2,08/63] bnxt_en: Use struct_group_attr() for memcpy() region [v2,09/63] mwl8k: Use struct_group() for memcpy() region [v2,10/63] libertas: Use struct_group() for memcpy() region [v2,11/63] libertas_tf: Use struct_group() for memcpy() region [v2,12/63] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region [v2,13/63] iommu/amd: Use struct_group() for memcpy() region [v2,14/63] cxgb3: Use struct_group() for memcpy() region [v2,15/63] intersil: Use struct_group() for memcpy() region [v2,16/63] cxgb4: Use struct_group() for memcpy() region [v2,17/63] bnx2x: Use struct_group() for memcpy() region [v2,18/63] drm/amd/pm: Use struct_group() for memcpy() region [v2,19/63] staging: wlan-ng: Use struct_group() for memcpy() region [v2,20/63] drm/mga/mga_ioc32: Use struct_group() for memcpy() region [v2,21/63] net/mlx5e: Use struct_group() for memcpy() region [v2,22/63] HID: cp2112: Use struct_group() for memcpy() region [v2,23/63] media: omap3isp: Use struct_group() for memcpy() region [v2,24/63] sata_fsl: Use struct_group() for memcpy() region [v2,25/63] compiler_types.h: Remove __compiletime_object_size() [v2,26/63] lib/string: Move helper functions out of string.c [v2,27/63] fortify: Move remaining fortify helpers into fortify-string.h [v2,28/63] fortify: Explicitly disable Clang support [v2,29/63] fortify: Fix dropped strcpy() compile-time write overflow check [v2,30/63] fortify: Prepare to improve strnlen() and strlen() warnings [v2,31/63] fortify: Allow strlen() and strnlen() to pass compile-time known lengths [v2,32/63] fortify: Add compile-time FORTIFY_SOURCE tests [v2,33/63] lib: Introduce CONFIG_TEST_MEMCPY [v2,34/63] fortify: Detect struct member overflows in memcpy() at compile-time [v2,35/63] fortify: Detect struct member overflows in memmove() at compile-time [v2,36/63] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp [v2,37/63] string.h: Introduce memset_after() for wiping trailing members/padding [v2,38/63] xfrm: Use memset_after() to clear padding [v2,39/63] ipv6: Use memset_after() to zero rt6_info [v2,40/63] netfilter: conntrack: Use memset_startat() to zero struct nf_conn [v2,41/63] net: 802: Use memset_startat() to clear struct fields [v2,42/63] net: dccp: Use memset_startat() for TP zeroing [v2,43/63] net: qede: Use memset_startat() for counters [v2,44/63] mac80211: Use memset_after() to clear tx status [v2,45/63] ath11k: Use memset_startat() for clearing queue descriptors [v2,46/63] iw_cxgb4: Use memset_startat() for cpl_t5_pass_accept_rpl [v2,47/63] intel_th: msu: Use memset_startat() for clearing hw header [v2,48/63] IB/mthca: Use memset_startat() for clearing mpt_entry [v2,49/63] btrfs: Use memset_startat() to clear end of struct [v2,50/63] tracing: Use memset_startat() to zero struct trace_iterator [v2,51/63] drbd: Use struct_group() to zero algs [v2,52/63] cm4000_cs: Use struct_group() to zero struct cm4000_dev region [v2,53/63] KVM: x86: Use struct_group() to zero decode cache [v2,54/63] dm integrity: Use struct_group() to zero struct journal_sector [v2,55/63] HID: roccat: Use struct_group() to zero kone_mouse_event [v2,56/63] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr [v2,57/63] powerpc/signal32: Use struct_group() to zero spe regs [v2,58/63] ethtool: stats: Use struct_group() to clear all stats at once [v2,59/63] can: flexcan: Use struct_group() to zero struct flexcan_regs regions [v2,60/63] net/af_iucv: Use struct_group() to zero struct iucv_sock region [v2,61/63] powerpc: Split memset() to avoid multi-field overflow [v2,62/63] fortify: Detect struct member overflows in memset() at compile-time [v2,63/63] fortify: Work around Clang inlining bugs

Message ID

20210818060533.3569517-45-keescook@chromium.org

State

New

Headers

From: Kees Cook <keescook@chromium.org>
To: linux-kernel@vger.kernel.org
Cc: Kees Cook <keescook@chromium.org>,
	Johannes Berg <johannes@sipsolutions.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	dri-devel@lists.freedesktop.org, linux-staging@lists.linux.dev,
	linux-block@vger.kernel.org, linux-kbuild@vger.kernel.org,
	clang-built-linux@googlegroups.com,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	linux-hardening@vger.kernel.org
Subject: [PATCH v2 44/63] mac80211: Use memset_after() to clear tx status
Date: Tue, 17 Aug 2021 23:05:14 -0700
Message-Id: <20210818060533.3569517-45-keescook@chromium.org>
In-Reply-To: <20210818060533.3569517-1-keescook@chromium.org>
References: <20210818060533.3569517-1-keescook@chromium.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

Introduce strict memcpy() bounds checking | expand

Commit Message

Kees Cook Aug. 18, 2021, 6:05 a.m. UTC

In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.

Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.

Additionally fix the common helper, ieee80211_tx_info_clear_status(),
which was not clearing ack_signal, but the open-coded versions
did. Johannes Berg points out this bug was introduced by commit
e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES") but was harmless.

Also drops the associated unneeded BUILD_BUG_ON()s, and adds a note to
carl9170 about usage.

Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/net/wireless/ath/carl9170/tx.c   | 11 +++++------
 drivers/net/wireless/intersil/p54/txrx.c |  6 +-----
 include/net/mac80211.h                   |  7 +------
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/tx.c b/drivers/net/wireless/ath/carl9170/tx.c
index 88444fe6d1c6..aa95d1a65882 100644
--- a/drivers/net/wireless/ath/carl9170/tx.c
+++ b/drivers/net/wireless/ath/carl9170/tx.c
@@ -275,12 +275,11 @@  static void carl9170_tx_release(struct kref *ref)
 	if (WARN_ON_ONCE(!ar))
 		return;
 
-	BUILD_BUG_ON(
-	    offsetof(struct ieee80211_tx_info, status.ack_signal) != 20);
-
-	memset(&txinfo->status.ack_signal, 0,
-	       sizeof(struct ieee80211_tx_info) -
-	       offsetof(struct ieee80211_tx_info, status.ack_signal));
+	/*
+	 * Should this call ieee80211_tx_info_clear_status() instead of clearing
+	 * manually? txinfo->status.rates do not seem to be used here.
+	 */
+	memset_after(&txinfo->status, 0, rates);
 
 	if (atomic_read(&ar->tx_total_queued))
 		ar->tx_schedule = true;
diff --git a/drivers/net/wireless/intersil/p54/txrx.c b/drivers/net/wireless/intersil/p54/txrx.c
index 873fea59894f..8414aa208655 100644
--- a/drivers/net/wireless/intersil/p54/txrx.c
+++ b/drivers/net/wireless/intersil/p54/txrx.c
@@ -431,11 +431,7 @@  static void p54_rx_frame_sent(struct p54_common *priv, struct sk_buff *skb)
 	 * Clear manually, ieee80211_tx_info_clear_status would
 	 * clear the counts too and we need them.
 	 */
-	memset(&info->status.ack_signal, 0,
-	       sizeof(struct ieee80211_tx_info) -
-	       offsetof(struct ieee80211_tx_info, status.ack_signal));
-	BUILD_BUG_ON(offsetof(struct ieee80211_tx_info,
-			      status.ack_signal) != 20);
+	memset_after(&info->status, 0, rates);
 
 	if (entry_hdr->flags & cpu_to_le16(P54_HDR_FLAG_DATA_ALIGN))
 		pad = entry_data->align[0];
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d8a1d09a2141..4c469b04de37 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1197,12 +1197,7 @@  ieee80211_tx_info_clear_status(struct ieee80211_tx_info *info)
 	/* clear the rate counts */
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
 		info->status.rates[i].count = 0;
-
-	BUILD_BUG_ON(
-	    offsetof(struct ieee80211_tx_info, status.ack_signal) != 20);
-	memset(&info->status.ampdu_ack_len, 0,
-	       sizeof(struct ieee80211_tx_info) -
-	       offsetof(struct ieee80211_tx_info, status.ampdu_ack_len));
+	memset_after(&info->status, 0, rates);
 }

[v2,44/63] mac80211: Use memset_after() to clear tx status

Commit Message

Patch