| Message ID | 20230629085115.180499-2-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dcce94b80a954a8968ff29fafcfb066d6197fa9a |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/3,v4] wifi: mwifiex: prefer strscpy() over strlcpy() | expand |
On Thu, Jun 29, 2023 at 11:51:01AM +0300, Dmitry Antipov wrote: [...] > This also fixes an improper usage of 'sizeof()'. Since 'skb' is > extended with 'sizeof(mgmt->u.action.u.tdls_discover_resp) + 1' > bytes (where 1 is actually 'sizeof(mgmt->u.action.category)'), > I assume that the same number of bytes should be copied. > > Suggested-by: Brian Norris <briannorris@chromium.org> I don't believe I actually *suggested* the change; I just highlighted that the size looked sketchy in the original code. :) But your change does look correct, and I don't see how we could possibly *want* to be off by 1 here, so: Reviewed-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > v4: fix memmove() size calculation (Brian Norris) > --- > drivers/net/wireless/marvell/mwifiex/tdls.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c index 97bb87c3676b..6c60621b6ccc 100644 --- a/drivers/net/wireless/marvell/mwifiex/tdls.c +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c @@ -735,6 +735,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv, int ret; u16 capab; struct ieee80211_ht_cap *ht_cap; + unsigned int extra; u8 radio, *pos; capab = priv->curr_bss_params.bss_descriptor.cap_info_bitmap; @@ -753,7 +754,10 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv, switch (action_code) { case WLAN_PUB_ACTION_TDLS_DISCOVER_RES: - skb_put(skb, sizeof(mgmt->u.action.u.tdls_discover_resp) + 1); + /* See the layout of 'struct ieee80211_mgmt'. */ + extra = sizeof(mgmt->u.action.u.tdls_discover_resp) + + sizeof(mgmt->u.action.category); + skb_put(skb, extra); mgmt->u.action.category = WLAN_CATEGORY_PUBLIC; mgmt->u.action.u.tdls_discover_resp.action_code = WLAN_PUB_ACTION_TDLS_DISCOVER_RES; @@ -762,8 +766,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv, mgmt->u.action.u.tdls_discover_resp.capability = cpu_to_le16(capab); /* move back for addr4 */ - memmove(pos + ETH_ALEN, &mgmt->u.action.category, - sizeof(mgmt->u.action.u.tdls_discover_resp)); + memmove(pos + ETH_ALEN, &mgmt->u.action, extra); /* init address 4 */ eth_broadcast_addr(pos);
When compiling with gcc 13.1 and CONFIG_FORTIFY_SOURCE=y, I've noticed the following: In function ‘fortify_memcpy_chk’, inlined from ‘mwifiex_construct_tdls_action_frame’ at drivers/net/wireless/marvell/mwifiex/tdls.c:765:3, inlined from ‘mwifiex_send_tdls_action_frame’ at drivers/net/wireless/marvell/mwifiex/tdls.c:856:6: ./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning] 529 | __read_overflow2_field(q_size_field, size); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The compiler actually complains on: memmove(pos + ETH_ALEN, &mgmt->u.action.category, sizeof(mgmt->u.action.u.tdls_discover_resp)); and it happens because the fortification logic interprets this as an attempt to overread 1-byte 'u.action.category' member of 'struct ieee80211_mgmt'. To silence this warning, it's enough to pass an address of 'u.action' itself instead of an address of its first member. This also fixes an improper usage of 'sizeof()'. Since 'skb' is extended with 'sizeof(mgmt->u.action.u.tdls_discover_resp) + 1' bytes (where 1 is actually 'sizeof(mgmt->u.action.category)'), I assume that the same number of bytes should be copied. Suggested-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- v4: fix memmove() size calculation (Brian Norris) --- drivers/net/wireless/marvell/mwifiex/tdls.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)