[10/64] lib80211: Use struct_group() for memcpy() region

Message ID	20210727205855.411487-11-keescook@chromium.org (mailing list archive)
State	Not Applicable
Delegated to:	Netdev Maintainers
Headers	show Return-Path: <netdev-owner@kernel.org> From: Kees Cook <keescook@chromium.org> To: linux-hardening@vger.kernel.org Cc: Kees Cook <keescook@chromium.org>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, Keith Packard <keithpac@amazon.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org, netdev@vger.kernel.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 Subject: [PATCH 10/64] lib80211: Use struct_group() for memcpy() region Date: Tue, 27 Jul 2021 13:58:01 -0700 Message-Id: <20210727205855.411487-11-keescook@chromium.org> In-Reply-To: <20210727205855.411487-1-keescook@chromium.org> References: <20210727205855.411487-1-keescook@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Introduce strict memcpy() bounds checking \| expand [00/64] Introduce strict memcpy() bounds checking [01/64] media: omap3isp: Extract struct group for memcpy() region [02/64] mac80211: Use flex-array for radiotap header bitmap [03/64] rpmsg: glink: Replace strncpy() with strscpy_pad() [04/64] stddef: Introduce struct_group() helper macro [05/64] skbuff: Switch structure bounds to struct_group() [06/64] bnxt_en: Use struct_group_attr() for memcpy() region [07/64] staging: rtl8192e: Use struct_group() for memcpy() region [08/64] staging: rtl8192u: Use struct_group() for memcpy() region [09/64] staging: rtl8723bs: Avoid field-overflowing memcpy() [10/64] lib80211: Use struct_group() for memcpy() region [11/64] net/mlx5e: Avoid field-overflowing memcpy() [12/64] mwl8k: Use struct_group() for memcpy() region [13/64] libertas: Use struct_group() for memcpy() region [14/64] libertas_tf: Use struct_group() for memcpy() region [15/64] ipw2x00: Use struct_group() for memcpy() region [16/64] thermal: intel: int340x_thermal: Use struct_group() for memcpy() region [17/64] iommu/amd: Use struct_group() for memcpy() region [18/64] cxgb3: Use struct_group() for memcpy() region [19/64] ip: Use struct_group() for memcpy() regions [20/64] intersil: Use struct_group() for memcpy() region [21/64] cxgb4: Use struct_group() for memcpy() region [22/64] bnx2x: Use struct_group() for memcpy() region [23/64] drm/amd/pm: Use struct_group() for memcpy() region [24/64] staging: wlan-ng: Use struct_group() for memcpy() region [25/64] drm/mga/mga_ioc32: Use struct_group() for memcpy() region [26/64] net/mlx5e: Use struct_group() for memcpy() region [27/64] HID: cp2112: Use struct_group() for memcpy() region [28/64] compiler_types.h: Remove __compiletime_object_size() [29/64] lib/string: Move helper functions out of string.c [30/64] fortify: Move remaining fortify helpers into fortify-string.h [31/64] fortify: Explicitly disable Clang support [32/64] fortify: Add compile-time FORTIFY_SOURCE tests [33/64] lib: Introduce CONFIG_TEST_MEMCPY [34/64] fortify: Detect struct member overflows in memcpy() at compile-time [35/64] fortify: Detect struct member overflows in memmove() at compile-time [36/64] scsi: ibmvscsi: Avoid multi-field memset() overflow by aiming at srp [37/64] string.h: Introduce memset_after() for wiping trailing members/padding [38/64] xfrm: Use memset_after() to clear padding [39/64] mac80211: Use memset_after() to clear tx status [40/64] net: 802: Use memset_after() to clear struct fields [41/64] net: dccp: Use memset_after() for TP zeroing [42/64] net: qede: Use memset_after() for counters [43/64] ath11k: Use memset_after() for clearing queue descriptors [44/64] iw_cxgb4: Use memset_after() for cpl_t5_pass_accept_rpl [45/64] intel_th: msu: Use memset_after() for clearing hw header [46/64] IB/mthca: Use memset_after() for clearing mpt_entry [47/64] btrfs: Use memset_after() to clear end of struct [48/64] drbd: Use struct_group() to zero algs [49/64] cm4000_cs: Use struct_group() to zero struct cm4000_dev region [50/64] KVM: x86: Use struct_group() to zero decode cache [51/64] tracing: Use struct_group() to zero struct trace_iterator [52/64] dm integrity: Use struct_group() to zero struct journal_sector [53/64] HID: roccat: Use struct_group() to zero kone_mouse_event [54/64] ipv6: Use struct_group() to zero rt6_info [55/64] RDMA/mlx5: Use struct_group() to zero struct mlx5_ib_mr [56/64] ethtool: stats: Use struct_group() to clear all stats at once [57/64] netfilter: conntrack: Use struct_group() to zero struct nf_conn [58/64] powerpc: Split memset() to avoid multi-field overflow [59/64] fortify: Detect struct member overflows in memset() at compile-time [60/64] fortify: Work around Clang inlining bugs [61/64] Makefile: Enable -Warray-bounds [62/64] netlink: Avoid false-positive memcpy() warning [63/64] iwlwifi: dbg_ini: Split memcpy() to avoid multi-field write [64/64] fortify: Add run-time WARN for cross-field memcpy()

Context	Check	Description
netdev/tree_selection	success	Guessing tree name failed - patch did not apply

Kees Cook July 27, 2021, 8:58 p.m. UTC

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

Use struct_group() around members addr1, addr2, and addr3 in struct
ieee80211_hdr so they can be referenced together. This will allow memcpy()
and sizeof() to more easily reason about sizes, improve readability,
and avoid future warnings about writing beyond the end of addr1.

"pahole" shows no size nor member offset changes to struct ieee80211_hdr.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/staging/rtl8723bs/core/rtw_security.c | 5 +++--
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 5 +++--
 include/linux/ieee80211.h                     | 8 +++++---
 net/wireless/lib80211_crypt_ccmp.c            | 3 ++-
 4 files changed, 13 insertions(+), 8 deletions(-)

Greg KH July 28, 2021, 5:52 a.m. UTC | #1

On Tue, Jul 27, 2021 at 01:58:01PM -0700, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memcpy(), memmove(), and memset(), avoid
> intentionally writing across neighboring fields.
> 
> Use struct_group() around members addr1, addr2, and addr3 in struct
> ieee80211_hdr so they can be referenced together. This will allow memcpy()
> and sizeof() to more easily reason about sizes, improve readability,
> and avoid future warnings about writing beyond the end of addr1.
> 
> "pahole" shows no size nor member offset changes to struct ieee80211_hdr.
> "objdump -d" shows no meaningful object code changes (i.e. only source
> line number induced differences and optimizations).
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/staging/rtl8723bs/core/rtw_security.c | 5 +++--
>  drivers/staging/rtl8723bs/core/rtw_xmit.c     | 5 +++--
>  include/linux/ieee80211.h                     | 8 +++++---
>  net/wireless/lib80211_crypt_ccmp.c            | 3 ++-
>  4 files changed, 13 insertions(+), 8 deletions(-)

For the staging portion:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Johannes Berg Aug. 13, 2021, 8:04 a.m. UTC | #2

On Tue, 2021-07-27 at 13:58 -0700, Kees Cook wrote:
> 
> +++ b/include/linux/ieee80211.h
> @@ -297,9 +297,11 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
>  struct ieee80211_hdr {
>  	__le16 frame_control;
>  	__le16 duration_id;
> -	u8 addr1[ETH_ALEN];
> -	u8 addr2[ETH_ALEN];
> -	u8 addr3[ETH_ALEN];
> +	struct_group(addrs,
> +		u8 addr1[ETH_ALEN];
> +		u8 addr2[ETH_ALEN];
> +		u8 addr3[ETH_ALEN];
> +	);
>  	__le16 seq_ctrl;
>  	u8 addr4[ETH_ALEN];
>  } __packed __aligned(2);

This file isn't really just lib80211, it's also used by everyone else
for 802.11, but I guess that's OK - after all, this doesn't really
result in any changes here.

> +++ b/net/wireless/lib80211_crypt_ccmp.c
> @@ -136,7 +136,8 @@ static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr,
>  	pos = (u8 *) hdr;
>  	aad[0] = pos[0] & 0x8f;
>  	aad[1] = pos[1] & 0xc7;
> -	memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN);
> +	BUILD_BUG_ON(sizeof(hdr->addrs) != 3 * ETH_ALEN);
> +	memcpy(aad + 2, &hdr->addrs, ETH_ALEN);


However, how is it you don't need the same change in net/mac80211/wpa.c?

We have three similar instances:

        /* AAD (extra authenticate-only data) / masked 802.11 header
         * FC | A1 | A2 | A3 | SC | [A4] | [QC] */
        put_unaligned_be16(len_a, &aad[0]);
        put_unaligned(mask_fc, (__le16 *)&aad[2]);
        memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);


and

        memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);

and

        memcpy(aad + 2, &hdr->addr1, 3 * ETH_ALEN);

so those should also be changed, it seems?

In which case I'd probably prefer to do this separately from the staging
drivers ...

johannes

Kees Cook Aug. 13, 2021, 3:49 p.m. UTC | #3

On Fri, Aug 13, 2021 at 10:04:09AM +0200, Johannes Berg wrote:
> On Tue, 2021-07-27 at 13:58 -0700, Kees Cook wrote:
> > 
> > +++ b/include/linux/ieee80211.h
> > @@ -297,9 +297,11 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
> >  struct ieee80211_hdr {
> >  	__le16 frame_control;
> >  	__le16 duration_id;
> > -	u8 addr1[ETH_ALEN];
> > -	u8 addr2[ETH_ALEN];
> > -	u8 addr3[ETH_ALEN];
> > +	struct_group(addrs,
> > +		u8 addr1[ETH_ALEN];
> > +		u8 addr2[ETH_ALEN];
> > +		u8 addr3[ETH_ALEN];
> > +	);
> >  	__le16 seq_ctrl;
> >  	u8 addr4[ETH_ALEN];
> >  } __packed __aligned(2);
> 
> This file isn't really just lib80211, it's also used by everyone else
> for 802.11, but I guess that's OK - after all, this doesn't really
> result in any changes here.
> 
> > +++ b/net/wireless/lib80211_crypt_ccmp.c
> > @@ -136,7 +136,8 @@ static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr,
> >  	pos = (u8 *) hdr;
> >  	aad[0] = pos[0] & 0x8f;
> >  	aad[1] = pos[1] & 0xc7;
> > -	memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN);
> > +	BUILD_BUG_ON(sizeof(hdr->addrs) != 3 * ETH_ALEN);
> > +	memcpy(aad + 2, &hdr->addrs, ETH_ALEN);
> 
> 
> However, how is it you don't need the same change in net/mac80211/wpa.c?
> 
> We have three similar instances:
> 
>         /* AAD (extra authenticate-only data) / masked 802.11 header
>          * FC | A1 | A2 | A3 | SC | [A4] | [QC] */
>         put_unaligned_be16(len_a, &aad[0]);
>         put_unaligned(mask_fc, (__le16 *)&aad[2]);
>         memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
> 
> 
> and
> 
>         memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);
> 
> and
> 
>         memcpy(aad + 2, &hdr->addr1, 3 * ETH_ALEN);
> 
> so those should also be changed, it seems?

Ah! Yes, thanks for pointing this out. During earlier development I split
the "cross-field write" changes from the "cross-field read" changes, and
it looks like I missed moving lib80211_crypt_ccmp.c into that portion of
the series (which I haven't posted nor finished -- it's lower priority
than fixing the cross-field writes).

> In which case I'd probably prefer to do this separately from the staging
> drivers ...

Agreed. Sorry for the noise on that part. I will double-check the other
patches.

Johannes Berg Aug. 13, 2021, 7:44 p.m. UTC | #4

On Fri, 2021-08-13 at 08:49 -0700, Kees Cook wrote:
> 
> Ah! Yes, thanks for pointing this out. During earlier development I split
> the "cross-field write" changes from the "cross-field read" changes, and
> it looks like I missed moving lib80211_crypt_ccmp.c into that portion of
> the series (which I haven't posted nor finished -- it's lower priority
> than fixing the cross-field writes).

Oh, OK. I think all of this patch was cross-field read though.

Anyway, the patch itself is fine, just seems incomplete and somewhat
badly organised :)

johannes

[10/64] lib80211: Use struct_group() for memcpy() region

Checks

Commit Message

Comments

Patch