linux-hardening - Re: [PATCH] mwl8k: Avoid overlapping composite structs that contain flex-arrays

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cfc4c4c0-83f8-437c-8146-6b86968db67b@embeddedor.com>
Date: Sat, 16 Mar 2024 12:59:11 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Erick Archer <erick.archer@....com>, Kalle Valo <kvalo@...nel.org>,
 Johannes Berg <johannes.berg@...el.com>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Kees Cook <keescook@...omium.org>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH] mwl8k: Avoid overlapping composite structs that contain
 flex-arrays

[..]

> 
> Link: https://github.com/KSPP/linux/issues/202 [1]
> Signed-off-by: Erick Archer <erick.archer@....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