diff mbox series

[4/6,next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_connect_event

Message ID 8a0e347615a3516980fd8b6ad2dc4864a880613b.1645583264.git.gustavoars@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ath6kl: wmi: Replace one-element arrays with flexible-array members | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Gustavo A. R. Silva Feb. 23, 2022, 2:38 a.m. UTC
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@kernel.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(-)

Comments

Jeff Johnson Feb. 24, 2022, 12:50 a.m. UTC | #1
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@kernel.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.

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.


>   		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@quicinc.com>
Gustavo A. R. Silva Feb. 24, 2022, 8:08 p.m. UTC | #2
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@kernel.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@quicinc.com>

Great. :)

Thanks a lot for the feedback!
--
Gustavo
diff mbox series

Patch

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))
 		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 */