netdev - Re: [PATCH 10/64] lib80211: Use struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Aug 2021 10:04:09 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Kees Cook <keescook@...omium.org>, linux-hardening@...r.kernel.org
Cc:     "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Keith Packard <keithpac@...zon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-staging@...ts.linux.dev, linux-block@...r.kernel.org,
        linux-kbuild@...r.kernel.org, clang-built-linux@...glegroups.com
Subject: Re: [PATCH 10/64] lib80211: Use struct_group() for memcpy() region

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