2024-04-02 17:50:13

by Erick Archer

[permalink] [raw]
Subject: [PATCH] batman-adv: Add flex array to struct batadv_tvlv_tt_data

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 <[email protected]>
---
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)

If there is no problem I can send a v2 with the "__counted_by" tag.

The Coccinelle script used to detect this code pattern is the following:

virtual report

@rule1@
type t1;
type t2;
identifier i0;
identifier i1;
identifier i2;
identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc";
position p1;
@@

i0 = sizeof(t1) + sizeof(t2) * i1;
..
i2 = ALLOC@p1(..., i0, ...);

@script:python depends on report@
p1 << rule1.p1;
@@

msg = "WARNING: verify allocation on line %s" % (p1[0].line)
coccilib.report.print_report(p1[0],msg)

Regards,
Erick
---
include/uapi/linux/batadv_packet.h | 28 +++++++++---------
net/batman-adv/translation-table.c | 47 ++++++++++++------------------
2 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h
index 6e25753015df..49cd345b84a7 100644
--- a/include/uapi/linux/batadv_packet.h
+++ b/include/uapi/linux/batadv_packet.h
@@ -592,19 +592,6 @@ struct batadv_tvlv_gateway_data {
__be32 bandwidth_up;
};

-/**
- * struct batadv_tvlv_tt_data - tt data propagated through the tt tvlv container
- * @flags: translation table flags (see batadv_tt_data_flags)
- * @ttvn: translation table version number
- * @num_vlan: number of announced VLANs. In the TVLV this struct is followed by
- * one batadv_tvlv_tt_vlan_data object per announced vlan
- */
-struct batadv_tvlv_tt_data {
- __u8 flags;
- __u8 ttvn;
- __be16 num_vlan;
-};
-
/**
* struct batadv_tvlv_tt_vlan_data - vlan specific tt data propagated through
* the tt tvlv container
@@ -618,6 +605,21 @@ struct batadv_tvlv_tt_vlan_data {
__u16 reserved;
};

+/**
+ * struct batadv_tvlv_tt_data - tt data propagated through the tt tvlv container
+ * @flags: translation table flags (see batadv_tt_data_flags)
+ * @ttvn: translation table version number
+ * @num_vlan: number of announced VLANs. In the TVLV this struct is followed by
+ * one batadv_tvlv_tt_vlan_data object per announced vlan
+ * @vlan_data: array of batadv_tvlv_tt_vlan_data objects
+ */
+struct batadv_tvlv_tt_data {
+ __u8 flags;
+ __u8 ttvn;
+ __be16 num_vlan;
+ struct batadv_tvlv_tt_vlan_data vlan_data[];
+};
+
/**
* struct batadv_tvlv_tt_change - translation table diff data
* @flags: status indicators concerning the non-mesh client (see
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index b95c36765d04..96d22d8209f4 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -856,8 +856,7 @@ batadv_tt_prepare_tvlv_global_data(struct batadv_orig_node *orig_node,
num_entries += atomic_read(&vlan->tt.num_entries);
}

- change_offset = sizeof(**tt_data);
- change_offset += num_vlan * sizeof(*tt_vlan);
+ change_offset = struct_size(*tt_data, vlan_data, num_vlan);

/* if tt_len is negative, allocate the space needed by the full table */
if (*tt_len < 0)
@@ -876,7 +875,7 @@ batadv_tt_prepare_tvlv_global_data(struct batadv_orig_node *orig_node,
(*tt_data)->ttvn = atomic_read(&orig_node->last_ttvn);
(*tt_data)->num_vlan = htons(num_vlan);

- tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(*tt_data + 1);
+ tt_vlan = (*tt_data)->vlan_data;
hlist_for_each_entry(vlan, &orig_node->vlan_list, list) {
tt_vlan->vid = htons(vlan->vid);
tt_vlan->crc = htonl(vlan->tt.crc);
@@ -936,8 +935,7 @@ batadv_tt_prepare_tvlv_local_data(struct batadv_priv *bat_priv,
total_entries += vlan_entries;
}

- change_offset = sizeof(**tt_data);
- change_offset += num_vlan * sizeof(*tt_vlan);
+ change_offset = struct_size(*tt_data, vlan_data, num_vlan);

/* if tt_len is negative, allocate the space needed by the full table */
if (*tt_len < 0)
@@ -956,7 +954,7 @@ batadv_tt_prepare_tvlv_local_data(struct batadv_priv *bat_priv,
(*tt_data)->ttvn = atomic_read(&bat_priv->tt.vn);
(*tt_data)->num_vlan = htons(num_vlan);

- tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(*tt_data + 1);
+ tt_vlan = (*tt_data)->vlan_data;
hlist_for_each_entry(vlan, &bat_priv->softif_vlan_list, list) {
vlan_entries = atomic_read(&vlan->tt.num_entries);
if (vlan_entries < 1)
@@ -2916,7 +2914,6 @@ static bool batadv_send_tt_request(struct batadv_priv *bat_priv,
{
struct batadv_tvlv_tt_data *tvlv_tt_data = NULL;
struct batadv_tt_req_node *tt_req_node = NULL;
- struct batadv_tvlv_tt_vlan_data *tt_vlan_req;
struct batadv_hard_iface *primary_if;
bool ret = false;
int i, size;
@@ -2932,7 +2929,7 @@ static bool batadv_send_tt_request(struct batadv_priv *bat_priv,
if (!tt_req_node)
goto out;

- size = sizeof(*tvlv_tt_data) + sizeof(*tt_vlan_req) * num_vlan;
+ size = struct_size(tvlv_tt_data, vlan_data, num_vlan);
tvlv_tt_data = kzalloc(size, GFP_ATOMIC);
if (!tvlv_tt_data)
goto out;
@@ -2944,12 +2941,10 @@ static bool batadv_send_tt_request(struct batadv_priv *bat_priv,
/* send all the CRCs within the request. This is needed by intermediate
* nodes to ensure they have the correct table before replying
*/
- tt_vlan_req = (struct batadv_tvlv_tt_vlan_data *)(tvlv_tt_data + 1);
for (i = 0; i < num_vlan; i++) {
- tt_vlan_req->vid = tt_vlan->vid;
- tt_vlan_req->crc = tt_vlan->crc;
+ tvlv_tt_data->vlan_data[i].vid = tt_vlan->vid;
+ tvlv_tt_data->vlan_data[i].crc = tt_vlan->crc;

- tt_vlan_req++;
tt_vlan++;
}

@@ -3001,7 +2996,6 @@ static bool batadv_send_other_tt_response(struct batadv_priv *bat_priv,
struct batadv_orig_node *res_dst_orig_node = NULL;
struct batadv_tvlv_tt_change *tt_change;
struct batadv_tvlv_tt_data *tvlv_tt_data = NULL;
- struct batadv_tvlv_tt_vlan_data *tt_vlan;
bool ret = false, full_table;
u8 orig_ttvn, req_ttvn;
u16 tvlv_len;
@@ -3024,10 +3018,9 @@ static bool batadv_send_other_tt_response(struct batadv_priv *bat_priv,
orig_ttvn = (u8)atomic_read(&req_dst_orig_node->last_ttvn);
req_ttvn = tt_data->ttvn;

- tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(tt_data + 1);
/* this node doesn't have the requested data */
if (orig_ttvn != req_ttvn ||
- !batadv_tt_global_check_crc(req_dst_orig_node, tt_vlan,
+ !batadv_tt_global_check_crc(req_dst_orig_node, tt_data->vlan_data,
ntohs(tt_data->num_vlan)))
goto out;

@@ -3370,7 +3363,6 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
struct batadv_orig_node *orig_node = NULL;
struct batadv_tvlv_tt_change *tt_change;
u8 *tvlv_ptr = (u8 *)tt_data;
- u16 change_offset;

batadv_dbg(BATADV_DBG_TT, bat_priv,
"Received TT_RESPONSE from %pM for ttvn %d t_size: %d [%c]\n",
@@ -3383,10 +3375,7 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,

spin_lock_bh(&orig_node->tt_lock);

- change_offset = sizeof(struct batadv_tvlv_tt_vlan_data);
- change_offset *= ntohs(tt_data->num_vlan);
- change_offset += sizeof(*tt_data);
- tvlv_ptr += change_offset;
+ tvlv_ptr += struct_size(tt_data, vlan_data, ntohs(tt_data->num_vlan));

tt_change = (struct batadv_tvlv_tt_change *)tvlv_ptr;
if (tt_data->flags & BATADV_TT_FULL_TABLE) {
@@ -3985,10 +3974,10 @@ static void batadv_tt_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
u8 flags, void *tvlv_value,
u16 tvlv_value_len)
{
- struct batadv_tvlv_tt_vlan_data *tt_vlan;
struct batadv_tvlv_tt_change *tt_change;
struct batadv_tvlv_tt_data *tt_data;
u16 num_entries, num_vlan;
+ size_t flex_size;

if (tvlv_value_len < sizeof(*tt_data))
return;
@@ -3998,17 +3987,18 @@ static void batadv_tt_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,

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;

- tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(tt_data + 1);
- tt_change = (struct batadv_tvlv_tt_change *)(tt_vlan + num_vlan);
- tvlv_value_len -= sizeof(*tt_vlan) * num_vlan;
+ tt_change = (struct batadv_tvlv_tt_change *)(tt_data->vlan_data +
+ num_vlan);
+ tvlv_value_len -= flex_size;

num_entries = batadv_tt_entries(tvlv_value_len);

- batadv_tt_update_orig(bat_priv, orig, tt_vlan, num_vlan, tt_change,
- num_entries, tt_data->ttvn);
+ batadv_tt_update_orig(bat_priv, orig, tt_data->vlan_data, num_vlan,
+ tt_change, num_entries, tt_data->ttvn);
}

/**
@@ -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);

if (tvlv_value_len < tt_vlan_len)
return NET_RX_SUCCESS;
--
2.25.1



2024-04-02 19:16:33

by Sven Eckelmann

[permalink] [raw]
Subject: Re: [PATCH] batman-adv: Add flex array to struct batadv_tvlv_tt_data

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 <[email protected]>

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


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

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

[....]
> /**
> @@ -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.

Kind regards,
Sven


Attachments:
signature.asc (849.00 B)
This is a digitally signed message part.

2024-04-06 16:46:42

by Erick Archer

[permalink] [raw]
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 <[email protected]>
>
> > ---
> > 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