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@xxxxxxx> > > --- > > 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@xxxxxxxxxx/ 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 >
