Received-SPF: pass (google.com: domain of linux-wireless+bounces-4803-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <cfc4c4c0-83f8-437c-8146-6b86968db67b@embeddedor.com>
Date: Sat, 16 Mar 2024 12:59:11 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mwl8k: Avoid overlapping composite structs that contain
 flex-arrays
To: Erick Archer <erick.archer@gmx.com>, Kalle Valo <kvalo@kernel.org>,
 Johannes Berg <johannes.berg@intel.com>,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>,
 Kees Cook <keescook@chromium.org>
Cc: linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-hardening@vger.kernel.org
References: <20240316150712.4633-1-erick.archer@gmx.com>
Content-Language: en-US
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
In-Reply-To: <20240316150712.4633-1-erick.archer@gmx.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


[..]

> 
> Link: https://github.com/KSPP/linux/issues/202 [1]
> Signed-off-by: Erick Archer <erick.archer@gmx.com>
> ---
> Hi everyone,
> 
> This patch is based on my understanding of the code. Any comments would
> be greatly appreciated.

Thanks for looking into this. :)

I'm currently in the process of trying a general solution for all these
composite structures without having to use two separate structs, avoid too
much code churn, and continue allowing for __counted_by() annotations at
the same time.

I'll be sending a bunch of patches once the merge window closes. So, for
now, I think it's wise to wait for those patches.

More comments below.

[..]

> diff --git a/drivers/net/wireless/marvell/mwl8k.c b/drivers/net/wireless/marvell/mwl8k.c
> index ce8fea76dbb2..57de32ba4efc 100644
> --- a/drivers/net/wireless/marvell/mwl8k.c
> +++ b/drivers/net/wireless/marvell/mwl8k.c
> @@ -586,13 +586,17 @@ static int mwl8k_request_firmware(struct mwl8k_priv *priv, char *fw_image,
>   	return 0;
>   }
> 
> -struct mwl8k_cmd_pkt {
> +struct mwl8k_cmd_pkt_hdr {
>   	__le16	code;
>   	__le16	length;
>   	__u8	seq_num;
>   	__u8	macid;
>   	__le16	result;
> -	char	payload[];
> +} __packed;
> +
> +struct mwl8k_cmd_pkt {
> +	struct mwl8k_cmd_pkt_hdr header;
> +	char payload[];
>   } __packed;

One of the problems with this is that `struct mwl8k_cmd_pkt` is candidate for a
`__counted_by()` annotation:

@@ -592,7 +592,7 @@ struct mwl8k_cmd_pkt {
         __u8    seq_num;
         __u8    macid;
         __le16  result;
-       char    payload[];
+       char    payload[] __counted_by(length);
  } __packed;

and with the changes you propose, that is not possible anymore because the counter
member must be at the same level or in an anonymous struct also at the same level
as `payload`.

Thanks
--
Gustavo