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.
--
Kees Cook
