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:   Wed, 21 Jun 2023 08:47:32 -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: <ZJMblMdEOQ6/ps2J@google.com>
References: <20230620100803.519926-1-dmantipov@yandex.ru>
 <20230620100803.519926-2-dmantipov@yandex.ru>
 <ZJHTLhQNM4oj40Zn@google.com>
 <a1f55197-6138-68d0-9e4f-8e53840d5d90@yandex.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a1f55197-6138-68d0-9e4f-8e53840d5d90@yandex.ru>
Precedence: bulk

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