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

[Erick Archer <erick.archer@gmx.com>]

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 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 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.gustavoars@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/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`.

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

Best regards,
Erick

> Thanks
> --
> Gustavo
>