Received-SPF: pass (google.com: domain of linux-wireless+bounces-4806-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 17 Mar 2024 16:22:25 +0100
From: Erick Archer <erick.archer@gmx.com>
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc: Erick Archer <erick.archer@gmx.com>, Kalle Valo <kvalo@kernel.org>,
	Johannes Berg <johannes.berg@intel.com>,
	Kees Cook <keescook@chromium.org>, linux-wireless@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] mwl8k: Avoid overlapping composite structs that contain
 flex-arrays
Message-ID: <20240317152225.GA2850@titan>
References: <20240316150712.4633-1-erick.archer@gmx.com>
 <cfc4c4c0-83f8-437c-8146-6b86968db67b@embeddedor.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cfc4c4c0-83f8-437c-8146-6b86968db67b@embeddedor.com>
UI-OutboundReport: notjunk:1;M01:P0:HvsWI6BjGPo=;J4J2pYE0PSII/hrBc70lKcFg4X2
 am2ZTLIRcO582PnieP8iYg6pWc0G2ahvPVjvdMpZlXt5I7QsURD+MmauSXbshn1CUmu6iNrsJ
 vcjoz/7afePOcAJPnWg0MdpW5PDkZ0Na12JY+UpYcx7bpMc2vExKI+AKVVtKRmr9r+yoNXXDq
 Uv2PXagj10MOcMXOLTlOg3Yqma4Xyys8Gvj2hOqFQhSUoJHQnojV5jrw3YkfIMKQfSztCq75r
 jwU2fUtzto/Oj9rd8iqqg4EVzXNSiAvLHN4F4t2ke6W7mUj/zo0d7dBW3/fyI/6dYqKP/Nj8O
 evX10tzKMZNKhxV9qX+IVaBQZipZu8uBIkecZ4+CBfqPSDsAx/WpVG4iNVL6+zv2DEHbTTP5F
 Zl4N1fs/kG4gFG2r26qOkSLbPqfv8HXpF1nRTT1pjB7rld8xd9tK4WJQwUGubmmIutS1VOr11
 GiBl73Acp8+z55qLtbvAm+YiUxaOeC91vcRTOwW0UdUM59Wc2EqCcJWs46zvIWDQReJbxskt7
 Eh/kV5LlSP+BYcRVpmwZ/HGUB6lljDhyH5vXtF5V7cyvyMiLCcSFpt1C3QyWFCTJl29CYantk
 KGbyrczaZWP+YDhEFxAmjcQGtDAq822pGRRpXFYZVD12zxs89UPI+el125bAWTs6I2YEoNOLk
 BLjHloSAQ8rOk/v3lwNlTl5N0hAEZC9CZO3wpIojnHgTbOiocNvD/JsW/5/rbR4cffp1djk1H
 QoWHZuoVGpWsKFgsfhiSj5smrihe4EcJjPzIiig645MB7ZWJvtmhW5fPdQvgw0A4jeAmGBTbq
 5l/HTJr2EZZgeihSwgVlAde4TA2Y91L4kAyjPpCWJ1t4M=
Content-Transfer-Encoding: quoted-printable

Hi Gustavo,

On Sat, Mar 16, 2024 at 12:59:11PM -0600, Gustavo A. R. Silva wrote:
>
> [..]
>
> >
> > 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 woul=
d
> > 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 t=
oo
> much code churn, and continue allowing for __counted_by() annotations at
> the same time.

I searched the mailing list and found several of your patches:

Link: https://lore.kernel.org/linux-hardening/ZfCXBykRw5XqBvf0@neat/
Link: https://lore.kernel.org/linux-hardening/cover.1709658886.git.gustavo=
ars@kernel.org/
Link: https://lore.kernel.org/linux-hardening/ZeeaRuTpuxInH6ZB@neat/

In all of them you use the `struct_group_tagged()` helper to solve the
overlapping scenario. Great proposal ;)

> 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.

So, are you working in a patch for the "mwl8k"? Or do you prefer
a v2 of this patch based on your proposal?

>
> More comments below.
>
> [..]
>
> > diff --git a/drivers/net/wireless/marvell/mwl8k.c b/drivers/net/wirele=
ss/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_p=
riv *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 candidat=
e 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 t=
he counter
> member must be at the same level or in an anonymous struct also at the s=
ame level
> as `payload`.

Ok, I understand the problem you raise and I agree.
Anyway, thanks for your comments.

Best regards,
Erick

> Thanks
> --
> Gustavo
>