| Message ID | 20210727205855.411487-11-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Introduce strict memcpy() bounds checking | expand |
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>
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
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.
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
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c b/drivers/staging/rtl8723bs/core/rtw_security.c index a99f439328f1..be7cf42855a1 100644 --- a/drivers/staging/rtl8723bs/core/rtw_security.c +++ b/drivers/staging/rtl8723bs/core/rtw_security.c @@ -1421,8 +1421,9 @@ u32 rtw_BIP_verify(struct adapter *padapter, u8 *precvframe) ClearRetry(BIP_AAD); ClearPwrMgt(BIP_AAD); ClearMData(BIP_AAD); - /* conscruct AAD, copy address 1 to address 3 */ - memcpy(BIP_AAD+2, pwlanhdr->addr1, 18); + /* conscruct AAD, copy address 1 through address 3 */ + BUILD_BUG_ON(sizeof(pwlanhdr->addrs) != 3 * ETH_ALEN); + memcpy(BIP_AAD + 2, &pwlanhdr->addrs, 3 * ETH_ALEN); if (omac1_aes_128(padapter->securitypriv.dot11wBIPKey[padapter->securitypriv.dot11wBIPKeyid].skey , BIP_AAD, ori_len, mic)) diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c b/drivers/staging/rtl8723bs/core/rtw_xmit.c index 79e4d7df1ef5..cb47db784130 100644 --- a/drivers/staging/rtl8723bs/core/rtw_xmit.c +++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c @@ -1198,8 +1198,9 @@ s32 rtw_mgmt_xmitframe_coalesce(struct adapter *padapter, struct sk_buff *pkt, s ClearRetry(BIP_AAD); ClearPwrMgt(BIP_AAD); ClearMData(BIP_AAD); - /* conscruct AAD, copy address 1 to address 3 */ - memcpy(BIP_AAD+2, pwlanhdr->addr1, 18); + /* conscruct AAD, copy address 1 through address 3 */ + BUILD_BUG_ON(sizeof(pwlanhdr->addrs) != 3 * ETH_ALEN); + memcpy(BIP_AAD + 2, &pwlanhdr->addrs, 3 * ETH_ALEN); /* copy management fram body */ memcpy(BIP_AAD+BIP_AAD_SIZE, MGMT_body, frame_body_len); /* calculate mic */ diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h index a6730072d13a..d7932b520aaf 100644 --- a/include/linux/ieee80211.h +++ 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); diff --git a/net/wireless/lib80211_crypt_ccmp.c b/net/wireless/lib80211_crypt_ccmp.c index 6a5f08f7491e..21d7c39bb394 100644 --- a/net/wireless/lib80211_crypt_ccmp.c +++ 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); pos = (u8 *) & hdr->seq_ctrl; aad[20] = pos[0] & 0x0f; aad[21] = 0; /* all bits masked */
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(-)