Re: [PATCH 2/2] [v2] wifi: mwifiex: fix fortify warningg

[Brian Norris <briannorris@chromium.org>]

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
>