linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Johnson <quic_jjohnson@quicinc.com>
To: "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	<linux-wireless@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Cc: Kalle Valo <kvalo@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, <netdev@vger.kernel.org>,
	<linux-hardening@vger.kernel.org>
Subject: Re: [PATCH 3/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_channel_list_reply
Date: Wed, 23 Feb 2022 16:49:08 -0800	[thread overview]
Message-ID: <3abb0846-a26f-3d76-8936-cd23cf4387f1@quicinc.com> (raw)
In-Reply-To: <30306253b1b5e6b8f5c0faba97e935eda4638020.1645583264.git.gustavoars@kernel.org>

On 2/22/2022 6:38 PM, Gustavo A. R. Silva wrote:
> Replace one-element array with flexible-array member in struct
> wmi_channel_list_reply.
> 
> It's also worth noting that due to the flexible array transformation,
> the size of struct wmi_channel_list_reply changed, see below.
> 
> Before flex-array transformation:
> 
> struct wmi_channel_list_reply {
> 	u8                         reserved;             /*     0     1 */
> 	u8                         num_ch;               /*     1     1 */
> 	__le16                     ch_list[1];           /*     2     2 */
> 
> 	/* size: 4, cachelines: 1, members: 3 */
> 	/* last cacheline: 4 bytes */
> };
> 
> After flex-array transformation:
> 
> struct wmi_channel_list_reply {
> 	u8                         reserved;             /*     0     1 */
> 	u8                         num_ch;               /*     1     1 */
> 	__le16                     ch_list[];            /*     2     0 */
> 
> 	/* size: 2, cachelines: 1, members: 3 */
> 	/* last cacheline: 2 bytes */
> };
> 
> So, the following change preserves the logic that if _len_ is at least
> 4 bytes in size, this is the existence of at least one channel in
> ch_list[] is being considered, then the execution jumps to call
> ath6kl_wakeup_event(wmi->parent_dev);, otherwise _len_ is 2 bytes or
> less and the code returns -EINVAL:
> 
> 	-       if (len < sizeof(struct wmi_channel_list_reply))
> 	+       if (len <= sizeof(struct wmi_channel_list_reply))
> 
> 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_channel_list_reply))
> 	+       if (len <= sizeof(struct wmi_channel_list_reply))

My opinion is this can remain unchanged since being unchanged would 
correctly handle a channel list with no channels whereas the original 
code required that at least one channel be present.

The test is really there just to make sure the entirety of the "fixed" 
portion of the message is present.

Ultimately it doesn't matter since no actual processing of the channel 
list takes place.

If actual processing took place, then it would make sense to have an 
additional test to verify the len is large enough to handle num_ch entries.


> 
> 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 bdfc057c5a82..049d75f31f3c 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.c
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.c
> @@ -1240,7 +1240,7 @@ static int ath6kl_wmi_ratemask_reply_rx(struct wmi *wmi, u8 *datap, int len)
>   
>   static int ath6kl_wmi_ch_list_reply_rx(struct wmi *wmi, u8 *datap, int len)
>   {
> -	if (len < sizeof(struct wmi_channel_list_reply))
> +	if (len <= sizeof(struct wmi_channel_list_reply))
>   		return -EINVAL;
>   
>   	ath6kl_wakeup_event(wmi->parent_dev);
> diff --git a/drivers/net/wireless/ath/ath6kl/wmi.h b/drivers/net/wireless/ath/ath6kl/wmi.h
> index 9e168752bec2..432e4f428a4a 100644
> --- a/drivers/net/wireless/ath/ath6kl/wmi.h
> +++ b/drivers/net/wireless/ath/ath6kl/wmi.h
> @@ -1373,7 +1373,7 @@ struct wmi_channel_list_reply {
>   	u8 num_ch;
>   
>   	/* channel in Mhz */
> -	__le16 ch_list[1];
> +	__le16 ch_list[];
>   } __packed;
>   
>   /* List of Events (target to host) */

whether or not you modify the length check consider this:
Reviewed-by: Jeff Johnson <quic_jjohnson@quicinc.com>

  reply	other threads:[~2022-02-24  2:11 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-23  2:38 [PATCH 0/6][next] ath6kl: wmi: Replace one-element arrays with flexible-array members Gustavo A. R. Silva
2022-02-23  2:38 ` [PATCH 1/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_begin_scan_cmd Gustavo A. R. Silva
2022-02-24  0:48   ` Jeff Johnson
2022-02-23  2:38 ` [PATCH 2/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_start_scan_cmd Gustavo A. R. Silva
2022-02-23  2:38 ` [PATCH 3/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_channel_list_reply Gustavo A. R. Silva
2022-02-24  0:49   ` Jeff Johnson [this message]
2022-02-23  2:38 ` [PATCH 4/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_connect_event Gustavo A. R. Silva
2022-02-24  0:50   ` Jeff Johnson
2022-02-24 20:08     ` Gustavo A. R. Silva
2022-02-23  2:39 ` [PATCH 5/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_disconnect_event Gustavo A. R. Silva
2022-02-24  0:53   ` Jeff Johnson
2022-02-23  2:39 ` [PATCH 6/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_aplist_event Gustavo A. R. Silva
2022-02-24  1:01   ` Jeff Johnson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3abb0846-a26f-3d76-8936-cd23cf4387f1@quicinc.com \
    --to=quic_jjohnson@quicinc.com \
    --cc=davem@davemloft.net \
    --cc=gustavoars@kernel.org \
    --cc=kuba@kernel.org \
    --cc=kvalo@kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH 3/6][next] ath6kl: wmi: Replace one-element array with flexible-array member in struct wmi_channel_list_reply' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).