From: Erick Archer <erick.archer@outlook.com>
To: Sven Eckelmann <sven@narfation.org>
Cc: Marek Lindner <mareklindner@neomailbox.ch>,
Simon Wunderlich <sw@simonwunderlich.de>,
Antonio Quartulli <a@unstable.cc>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Kees Cook <keescook@chromium.org>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Erick Archer <erick.archer@outlook.com>,
b.a.t.m.a.n@lists.open-mesh.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] batman-adv: Add flex array to struct batadv_tvlv_tt_data
Date: Sat, 6 Apr 2024 18:46:11 +0200 [thread overview]
Message-ID: <AS8PR02MB7237161B8D17F83F0A31467F8B022@AS8PR02MB7237.eurprd02.prod.outlook.com> (raw)
In-Reply-To: <5466543.Sb9uPGUboI@sven-l14>
Hi Sven,
On Tue, Apr 02, 2024 at 09:06:35PM +0200, Sven Eckelmann wrote:
> On Tuesday, 2 April 2024 19:23:01 CEST Erick Archer wrote:
> > The "struct batadv_tvlv_tt_data" uses a dynamically sized set of
> > trailing elements. Specifically, it uses an array of structures of type
> > "batadv_tvlv_tt_vlan_data". So, use the preferred way in the kernel
> > declaring a flexible array [1].
> >
> > The order in which the structure batadv_tvlv_tt_data and the structure
> > batadv_tvlv_tt_vlan_data are defined must be swap to avoid an incomplete
> > type error.
> >
> > Also, avoid the open-coded arithmetic in memory allocator functions [2]
> > using the "struct_size" macro and use the "flex_array_size" helper to
> > clarify some calculations, when possible.
> >
> > Moreover, the new structure member also allow us to avoid the open-coded
> > arithmetic on pointers in some situations. Take advantage of this.
> >
> > This code was detected with the help of Coccinelle, and audited and
> > modified manually.
> >
> > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
> > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> > Signed-off-by: Erick Archer <erick.archer@outlook.com>
>
> > ---
> > Hi,
> >
> > I would like to add the "__counted_by(num_vlan)" tag to the new flex member
> > but I don't know if this line can affect it.
> >
> > ntohs(tt_data->num_vlan)
>
>
> Yes, num_vlan is a __be16. I could only identify the kernel-doc related
> scripts as consumer. But maybe they are more - so I would defer this question
> to kernel-hardening.
Thanks for the info.
>
> And with this change, I get a lot of additional warnings (-Wsparse-all)
>
>
> cfg: BLA=n DAT=y DEBUG=y TRACING=n NC=y MCAST=n BATMAN_V=n
> net/batman-adv/translation-table.c:574:21: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:859:25: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:859:25: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:938:25: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:938:25: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:2932:16: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:2932:16: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:3378:21: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:3378:21: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:3982:30: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:3986:27: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:4026:30: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:4030:27: warning: using sizeof on a flexible structure
> net/batman-adv/translation-table.c:4032:23: warning: cast from restricted __be16
> net/batman-adv/translation-table.c:4032:23: warning: restricted __be16 degrades to integer
> net/batman-adv/translation-table.c:4032:23: warning: incorrect type in argument 1 (different base types)
> net/batman-adv/translation-table.c:4032:23: expected unsigned long [usertype] factor1
> net/batman-adv/translation-table.c:4032:23: got restricted __be16 [usertype] num_vlan
>
> [...]
I will work on this for the next version. Thanks for share these warnings.
> > num_vlan = ntohs(tt_data->num_vlan);
> >
> > - if (tvlv_value_len < sizeof(*tt_vlan) * num_vlan)
> > + flex_size = flex_array_size(tt_data, vlan_data, num_vlan);
> > + if (tvlv_value_len < flex_size)
> > return;
>
> This helper would need an #include of <linux/overflow.h> in
> net/batman-adv/translation-table.c
Understood.
>
> [....]
> > /**
> > @@ -4039,8 +4029,7 @@ static int batadv_tt_tvlv_unicast_handler_v1(struct batadv_priv *bat_priv,
> > tt_data = tvlv_value;
> > tvlv_value_len -= sizeof(*tt_data);
> >
> > - tt_vlan_len = sizeof(struct batadv_tvlv_tt_vlan_data);
> > - tt_vlan_len *= ntohs(tt_data->num_vlan);
> > + tt_vlan_len = flex_array_size(tt_data, vlan_data, tt_data->num_vlan);
>
> This is definitely wrong on little endian systems. You first need to convert
> num_vlan from network (big endian) to host order.
I'm sorry. My bad. I forgot to add the "ntohs".
I will fix it for the next version.
>
> Kind regards,
> Sven
Regards,
Erick
prev parent reply other threads:[~2024-04-06 17:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 17:23 [PATCH] batman-adv: Add flex array to struct batadv_tvlv_tt_data Erick Archer
2024-04-02 19:06 ` Sven Eckelmann
2024-04-06 16:46 ` Erick Archer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS8PR02MB7237161B8D17F83F0A31467F8B022@AS8PR02MB7237.eurprd02.prod.outlook.com \
--to=erick.archer@outlook.com \
--cc=a@unstable.cc \
--cc=b.a.t.m.a.n@lists.open-mesh.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mareklindner@neomailbox.ch \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sven@narfation.org \
--cc=sw@simonwunderlich.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).