| Message ID | 20211118184158.1284180-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | intersil: Use struct_group() for memcpy() region | expand |
On Thu, 2021-11-18 at 10:41 -0800, Kees Cook wrote: > > /* 802.11 */ > - __le16 frame_control; /* parts not used */ > - __le16 duration_id; > - u8 addr1[ETH_ALEN]; > - u8 addr2[ETH_ALEN]; /* filled by firmware */ > - u8 addr3[ETH_ALEN]; > - __le16 seq_ctrl; /* filled by firmware */ > + struct_group(frame, Arguably, that should be 'header' rather than 'frame' :) johannes
On Thu, Nov 18, 2021 at 08:46:11PM +0100, Johannes Berg wrote: > On Thu, 2021-11-18 at 10:41 -0800, Kees Cook wrote: > > > > /* 802.11 */ > > - __le16 frame_control; /* parts not used */ > > - __le16 duration_id; > > - u8 addr1[ETH_ALEN]; > > - u8 addr2[ETH_ALEN]; /* filled by firmware */ > > - u8 addr3[ETH_ALEN]; > > - __le16 seq_ctrl; /* filled by firmware */ > > + struct_group(frame, > > Arguably, that should be 'header' rather than 'frame' :) Works for me. :) I will rename it. Thanks!
diff --git a/drivers/net/wireless/intersil/hostap/hostap_hw.c b/drivers/net/wireless/intersil/hostap/hostap_hw.c index e459e7192ae9..8b7374023659 100644 --- a/drivers/net/wireless/intersil/hostap/hostap_hw.c +++ b/drivers/net/wireless/intersil/hostap/hostap_hw.c @@ -1815,8 +1815,9 @@ static int prism2_tx_80211(struct sk_buff *skb, struct net_device *dev) memset(&txdesc, 0, sizeof(txdesc)); /* skb->data starts with txdesc->frame_control */ - hdr_len = 24; - skb_copy_from_linear_data(skb, &txdesc.frame_control, hdr_len); + hdr_len = sizeof(txdesc.frame); + BUILD_BUG_ON(hdr_len != 24); + skb_copy_from_linear_data(skb, &txdesc.frame, hdr_len); if (ieee80211_is_data(txdesc.frame_control) && ieee80211_has_a4(txdesc.frame_control) && skb->len >= 30) { diff --git a/drivers/net/wireless/intersil/hostap/hostap_wlan.h b/drivers/net/wireless/intersil/hostap/hostap_wlan.h index dd2603d9b5d3..174735a137c5 100644 --- a/drivers/net/wireless/intersil/hostap/hostap_wlan.h +++ b/drivers/net/wireless/intersil/hostap/hostap_wlan.h @@ -115,12 +115,14 @@ struct hfa384x_tx_frame { __le16 tx_control; /* HFA384X_TX_CTRL_ flags */ /* 802.11 */ - __le16 frame_control; /* parts not used */ - __le16 duration_id; - u8 addr1[ETH_ALEN]; - u8 addr2[ETH_ALEN]; /* filled by firmware */ - u8 addr3[ETH_ALEN]; - __le16 seq_ctrl; /* filled by firmware */ + struct_group(frame, + __le16 frame_control; /* parts not used */ + __le16 duration_id; + u8 addr1[ETH_ALEN]; + u8 addr2[ETH_ALEN]; /* filled by firmware */ + u8 addr3[ETH_ALEN]; + __le16 seq_ctrl; /* filled by firmware */ + ); u8 addr4[ETH_ALEN]; __le16 data_len;
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() in struct hfa384x_tx_frame around members frame_control, duration_id, addr1, addr2, addr3, and seq_ctrl, 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 frame_control. "pahole" shows no size nor member offset changes to struct hfa384x_tx_frame. "objdump -d" shows no object code changes. Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/net/wireless/intersil/hostap/hostap_hw.c | 5 +++-- drivers/net/wireless/intersil/hostap/hostap_wlan.h | 14 ++++++++------ 2 files changed, 11 insertions(+), 8 deletions(-)