| Message ID | 20230620100803.519926-2-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/2,v2] wifi: mwifiex: avoid strlcpy() and use strscpy() where appropriate | expand |
On Tue, Jun 20, 2023 at 01:07:37PM +0300, Dmitry Antipov wrote: > 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. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > drivers/net/wireless/marvell/mwifiex/tdls.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c > index 97bb87c3676b..5a2941965757 100644 > --- a/drivers/net/wireless/marvell/mwifiex/tdls.c > +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c > @@ -762,7 +762,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, > + memmove(pos + ETH_ALEN, &mgmt->u.action, > sizeof(mgmt->u.action.u.tdls_discover_resp)); This invocation seems a bit suspect, as it uses a 'sizeof' of a field that doesn't match the actual pointer (it's off by 1 byte), but that's not your fault. I suppose it's no wonder we had so many problems with TDLS support on mwifiex... Anyway, the refactor looks fine: Reviewed-by: Brian Norris <briannorris@chromium.org> > /* init address 4 */ > eth_broadcast_addr(pos); > -- > 2.41.0 >
On 6/20/23 19:26, Brian Norris wrote: > This invocation seems a bit suspect, as it uses a 'sizeof' of a field > that doesn't match the actual pointer (it's off by 1 byte), but that's > not your fault. I suppose it's no wonder we had so many problems with > TDLS support on mwifiex... Hm, ieee80211_prep_tdls_direct() seems takes this byte into account. But do you know why 'u.action.u.tdls_discover_resp' is ended with a flexible array, e.g.: struct { u8 action_code; u8 dialog_token; __le16 capability; u8 variable[0]; } __packed tdls_discover_resp; Dmitry
On Wed, Jun 21, 2023 at 11:32:25AM +0300, Dmitry Antipov wrote: > On 6/20/23 19:26, Brian Norris wrote: > > > This invocation seems a bit suspect, as it uses a 'sizeof' of a field > > that doesn't match the actual pointer (it's off by 1 byte), but that's > > not your fault. I suppose it's no wonder we had so many problems with > > TDLS support on mwifiex... > > Hm, ieee80211_prep_tdls_direct() seems takes this byte into account. Presumably it's part of the standard packet format. (I haven't checked.) But in this case, we're talking about the firmware format that Marvell firmware expects -- which isn't documented at all. Usually it's at least related to the IEEE spec, but it isn't guaranteed to be laid out exactly the same. BTW, mwifiex doesn't actually use those ieee8021_*() functions for the most part, because it's not a mac80211 driver. > But > do you know why 'u.action.u.tdls_discover_resp' is ended with a flexible > array, e.g.: > > struct { > u8 action_code; > u8 dialog_token; > __le16 capability; > u8 variable[0]; > } __packed tdls_discover_resp; Not without more guess-based investigation. My poking around this driver is more often based on code reading and problem investigation, not based on any private knowledge of the mwifiex firmware or hardware. But my guess is that it's supposed to reflect the dynamic amount of additional IEs appended to this frame, before the body -- such as what is copied in mwifiex_tdls_append_rates_ie() (or ieee80211_tdls_add_ies() if we're talking about a mac80211 driver?). The field itself doesn't actually matter, because it isn't used in the driver AFAICT. Brian
diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c index 97bb87c3676b..5a2941965757 100644 --- a/drivers/net/wireless/marvell/mwifiex/tdls.c +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c @@ -762,7 +762,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, + memmove(pos + ETH_ALEN, &mgmt->u.action, sizeof(mgmt->u.action.u.tdls_discover_resp)); /* 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. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- drivers/net/wireless/marvell/mwifiex/tdls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)