Received-SPF: pass (google.com: domain of linux-wireless+bounces-2213-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 18 Jan 2024 12:19:49 -0800
From: Kees Cook <keescook@chromium.org>
To: Arend van Spriel <arend.vanspriel@broadcom.com>
Cc: Atul Raut <rauji.raut@gmail.com>, aspriel@gmail.com,
	franky.lin@broadcom.com, kvalo@kernel.org,
	linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH v2] brcmfmac: replace one-element array with
 flexible-array member
Message-ID: <202401181217.1D75C32E@keescook>
References: <20230803052238.12147-1-rauji.raut@gmail.com>
 <20230803052238.12147-2-rauji.raut@gmail.com>
 <f739cc15-dade-4f69-a17a-30242026c099@broadcom.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f739cc15-dade-4f69-a17a-30242026c099@broadcom.com>

On Thu, Jan 18, 2024 at 02:53:28PM +0100, Arend van Spriel wrote:
> + Kees
> 
> On 8/3/2023 7:22 AM, Atul Raut wrote:
> > One-element arrays are obsolete, and flexible
> > array members have taken their place. So, in
> > struct cca_stats_n_flags, replace the one-element
> > array with a flexible-array member.
> > 
> > This fixes warnings such as:
> > ./drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:119:6-9: WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)
> 
> I think this correct, but maybe Kees can give definitive answer here.
> 
> > Signed-off-by: Atul Raut <rauji.raut@gmail.com>
> > ---
> >   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > index de8a2e27f49c..fff32e54833d 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> > @@ -116,7 +116,7 @@ struct brcmf_dump_survey {
> >   struct cca_stats_n_flags {
> >   	u32 msrmnt_time; /* Time for Measurement (msec) */
> >   	u32 msrmnt_done; /* flag set when measurement complete */
> > -	char buf[1];
> > +	char buf[];
> >   };
> >   struct cca_msrmnt_query {

Normally a [1] -> [] conversion needs some details in the commit log
about why this is safe (since it runs the risk of changing sizeof(struct
cca_stats_n_flags). In this case, there's only a single user of the
struct, and nothing about its use depends on its size:

#define BRCMF_DCMD_MEDLEN       1536
...
        struct cca_stats_n_flags *results;
        char *buf;
	...
        buf = kzalloc(sizeof(char) * BRCMF_DCMD_MEDLEN, GFP_KERNEL);
	...
        results = (struct cca_stats_n_flags *)(buf);
	...
                brcmf_parse_dump_obss(results->buf, survey);

So, the allocation size is big enough for the struct, and nothing
depends on the struct size.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook