Date:   Thu, 24 Feb 2022 14:08:39 -0600
From:   "Gustavo A. R. Silva" <gustavoars@...nel.org>
To:     Jeff Johnson <quic_jjohnson@...cinc.com>
Cc:     linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kalle Valo <kvalo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH 4/6][next] ath6kl: wmi: Replace one-element array with
 flexible-array member in struct wmi_connect_event

On Wed, Feb 23, 2022 at 04:50:18PM -0800, Jeff Johnson wrote:
> On 2/22/2022 6:38 PM, Gustavo A. R. Silva wrote:
> > Replace one-element array with flexible-array member in struct
> > wmi_connect_event.
> > 
> > It's also worth noting that due to the flexible array transformation,
> > the size of struct wmi_connect_event changed (now the size is 1 byte
> > smaller), and in order to preserve the logic of before the transformation,
> > the following change is needed:
> > 
> > 	-       if (len < sizeof(struct wmi_connect_event))
> > 	+       if (len <= sizeof(struct wmi_connect_event))
> > 
> > This issue was found with the help of Coccinelle and audited and fixed,
> > manually.
> > 
> > Link: https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> > Link: https://github.com/KSPP/linux/issues/79
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> > ---
> > Hi!
> > 
> > It'd be great if someone can confirm or comment on the following
> > changes described in the changelog text:
> > 
> >          -       if (len < sizeof(struct wmi_connect_event))
> >          +       if (len <= sizeof(struct wmi_connect_event))
> > 
> > Thanks
> > 
> >   drivers/net/wireless/ath/ath6kl/wmi.c | 2 +-
> >   drivers/net/wireless/ath/ath6kl/wmi.h | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
> > index 049d75f31f3c..ccdccead688e 100644
> > --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> > +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> > @@ -857,7 +857,7 @@ static int ath6kl_wmi_connect_event_rx(struct wmi *wmi, u8 *datap, int len,
> >   	struct wmi_connect_event *ev;
> >   	u8 *pie, *peie;
> > -	if (len < sizeof(struct wmi_connect_event))
> > +	if (len <= sizeof(struct wmi_connect_event))
> 
> this is another case where IMO the original code can remain since all it is
> really checking is to see if the entire "fixed" portion is present. in
> reality if there was just one byte of assoc_info the response would actually
> be invalid.

I see; that's actually what I needed to be clarified. I wasn't sure if
a channel list with no channels was valid and expected. Now I see it is. :)

> what is missing is logic that verifies len is large enough to hold the
> payload that is advertised via the beacon_ie_len, assoc_req_len, &
> assoc_resp_len members. without this even if you change the initial len
> check you can have a condition where len says there is one u8 in assoc_info
> (and pass the initial test) but the other three members indicate that much
> more data is present.
> 
> but that is a pre-existing shortcoming that should be handled with a
> separate patch.

Yeah; I'll consider extending this series to include a patch for that.

> >   		return -EINVAL;
> >   	ev = (struct wmi_connect_event *) datap;
> > diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> > index 432e4f428a4a..6b064e669d87 100644
> > --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> > +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> > @@ -1545,7 +1545,7 @@ struct wmi_connect_event {
> >   	u8 beacon_ie_len;
> >   	u8 assoc_req_len;
> >   	u8 assoc_resp_len;
> > -	u8 assoc_info[1];
> > +	u8 assoc_info[];
> >   } __packed;
> >   /* Disconnect Event */
> 
> whether or not you modify the length check consider this:
> Reviewed-by: Jeff Johnson <quic_jjohnson@...cinc.com>

Great. :)

Thanks a lot for the feedback!
--
Gustavo

Powered by blists - more mailing lists