Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Jun 2023 09:26:22 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Dmitry Antipov <dmantipov@yandex.ru>
Cc:     Christophe Jaillet <christophe.jaillet@wanadoo.fr>,
        Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org
Subject: Re: [PATCH 2/2] [v2] wifi: mwifiex: fix fortify warningg
Message-ID: <ZJHTLhQNM4oj40Zn@google.com>
References: <20230620100803.519926-1-dmantipov@yandex.ru>
 <20230620100803.519926-2-dmantipov@yandex.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230620100803.519926-2-dmantipov@yandex.ru>
Precedence: bulk

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
>