netdev - Re: [PATCH] batman-adv: Add flex array to struct batadv_tvlv_tt

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] [day] [month] [year] [list]
Date: Sat, 6 Apr 2024 18:46:11 +0200
From: Erick Archer <erick.archer@...look.com>
To: Sven Eckelmann <sven@...fation.org>
Cc: Marek Lindner <mareklindner@...mailbox.ch>,
	Simon Wunderlich <sw@...onwunderlich.de>,
	Antonio Quartulli <a@...table.cc>,
	"David S.  Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Kees Cook <keescook@...omium.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Erick Archer <erick.archer@...look.com>,
	b.a.t.m.a.n@...ts.open-mesh.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] batman-adv: Add flex array to struct batadv_tvlv_tt_data

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