[v2] batman-adv: Add flex array to struct batadv_tvlv_tt_data
Commit Message
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].
At the same time, prepare for the coming implementation by GCC and Clang
of the __counted_by attribute. Flexible array members annotated with
__counted_by can have their accesses bounds-checked at run-time via
CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
strcpy/memcpy-family functions). In this case, it is important to note
that the attribute used is specifically __counted_by_be since variable
"num_vlan" is of type __be16.
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>
---
Changes in v2:
- Add the #include of <linux/overflow.h> for the "flex_array_size"
helper (Sven Eckelmann).
- Add the "ntohs" function to the "flex_array_size" helper removed
by mistake during the conversion (Sven Eckelmann).
- Add the "__counted_by_be" attribute.
Previous versions:
v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237987BF9DFCA030B330F658B3E2@AS8PR02MB7237.eurprd02.prod.outlook.com/
Hi,
In this version I have worked on the "sparse" warnings. The difference
in "sparse" warnings before and after this patch is as follows:
net/batman-adv/translation-table.c:534:21: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:819:25: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:819:25: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:819:25: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:819:25: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:819:25: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:898:25: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:898:25: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:898:25: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:898:25: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:898:25: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:2892:16: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:2892:16: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:2892:16: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:2892:16: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:2892:16: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:3338:21: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:3338:21: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:3338:21: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:3338:21: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:3338:21: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:3942:30: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:3946:27: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:3950:21: warning: expression using sizeof(void)
net/batman-adv/translation-table.c:3986:30: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:3990:27: warning: using sizeof on a flexible structure
net/batman-adv/translation-table.c:3992:23: warning: expression using sizeof(void)
As far as I know, these warnings cannot be removed. However it can be
safely ignored since:
Case 1: Warning "using sizeof on a flexible structure"
The commit 1a88f2f24619 ("flex-array: warn when using sizeof() on a flexible array")
in the sparse git repository says
Using sizeof() on a structure containing a flexible array
will ignore the 'flexible' part. This is maybe what is expected
but maybe not, so add an option -Wflexible-array-sizeof to
warn on such usage.
In this patch we take care of using or ignoring the flexible part
when necessary. Therefore, these warnings can be safely ignored.
Case 2: Warning "expression using sizeof(void)"
This warning is implied by the use of the "struct_size" helper and
"flex_array_size" helper because they both use the "__is_constexpr"
macro. And this macro is defined using "sizeof(void)".
/*
* This returns a constant expression while determining if an argument is
* a constant expression, most importantly without evaluating the argument.
* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de>
*
* Details:
* - sizeof() return an integer constant expression, and does not evaluate
* the value of its operand; it only examines the type of its operand.
* - The results of comparing two integer constant expressions is also
* an integer constant expression.
* - The first literal "8" isn't important. It could be any literal value.
* - The second literal "8" is to avoid warnings about unaligned pointers;
* this could otherwise just be "1".
* - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
* architectures.
* - The C Standard defines "null pointer constant", "(void *)0", as
* distinct from other void pointers.
* - If (x) is an integer constant expression, then the "* 0l" resolves
* it into an integer constant expression of value 0. Since it is cast to
* "void *", this makes the second operand a null pointer constant.
* - If (x) is not an integer constant expression, then the second operand
* resolves to a void pointer (but not a null pointer constant: the value
* is not an integer constant 0).
* - The conditional operator's third operand, "(int *)8", is an object
* pointer (to type "int").
* - The behavior (including the return type) of the conditional operator
* ("operand1 ? operand2 : operand3") depends on the kind of expressions
* given for the second and third operands. This is the central mechanism
* of the macro:
* - When one operand is a null pointer constant (i.e. when x is an integer
* constant expression) and the other is an object pointer (i.e. our
* third operand), the conditional operator returns the type of the
* object pointer operand (i.e. "int *"). Here, within the sizeof(), we
* would then get:
* sizeof(*((int *)(...)) == sizeof(int) == 4
* - When one operand is a void pointer (i.e. when x is not an integer
* constant expression) and the other is an object pointer (i.e. our
* third operand), the conditional operator returns a "void *" type.
* Here, within the sizeof(), we would then get:
* sizeof(*((void *)(...)) == sizeof(void) == 1
* - The equality comparison to "sizeof(int)" therefore depends on (x):
* sizeof(int) == sizeof(int) (x) was a constant expression
* sizeof(int) != sizeof(void) (x) was not a constant expression
*/
#define __is_constexpr(x) \
(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
Therefore, these warnings can also be safely ignored.
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 | 49 ++++++++++++------------------
2 files changed, 35 insertions(+), 42 deletions(-)
Comments
On Fri, Apr 26, 2024 at 07:22:47PM +0200, 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].
>
> At the same time, prepare for the coming implementation by GCC and Clang
> of the __counted_by attribute. Flexible array members annotated with
> __counted_by can have their accesses bounds-checked at run-time via
> CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for
> strcpy/memcpy-family functions). In this case, it is important to note
> that the attribute used is specifically __counted_by_be since variable
> "num_vlan" is of type __be16.
>
> 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>
Thanks for this! I think the readability is significantly improved.
> [...]
> @@ -3957,17 +3947,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);
This is the only part I'm a little worried about. The math all checks
out, but the compiler may get bothered about performing a pointer
calculation using the vlan_data flexible array memory. As in, this may
be calculated as an array offset, since it is the same as:
&tt_data->vlan_data[num_vlan]
Which, for big endian where __counted_by is in effect, the bounds
checker may throw a fit since we're going past the end of the array. In
other "multiple trailing flexible array" situations, we've done the
addressing from the base pointer, since the compiler either knows the
full allocation size or it knows nothing about it (this case, since it
came from a "void *" function argument). I would suggest:
tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data + flex_size);
-Kees
Hi Kees,
On Mon, Apr 29, 2024 at 10:36:23AM -0700, Kees Cook wrote:
> On Fri, Apr 26, 2024 at 07:22:47PM +0200, Erick Archer wrote:
> >
> > @@ -3957,17 +3947,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);
>
> This is the only part I'm a little worried about. The math all checks
> out, but the compiler may get bothered about performing a pointer
> calculation using the vlan_data flexible array memory. As in, this may
> be calculated as an array offset, since it is the same as:
>
> &tt_data->vlan_data[num_vlan]
>
> Which, for big endian where __counted_by is in effect, the bounds
> checker may throw a fit since we're going past the end of the array. In
> other "multiple trailing flexible array" situations, we've done the
> addressing from the base pointer, since the compiler either knows the
> full allocation size or it knows nothing about it (this case, since it
> came from a "void *" function argument). I would suggest:
>
> tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data + flex_size);
>
First of all thanks for the review and the great explanation. The change
looks reasonable to me. I will send a new version with this suggestion.
Regards,
Erick
@@ -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[] __counted_by_be(num_vlan);
+};
+
/**
* struct batadv_tvlv_tt_change - translation table diff data
* @flags: status indicators concerning the non-mesh client (see
@@ -28,6 +28,7 @@
#include <linux/net.h>
#include <linux/netdevice.h>
#include <linux/netlink.h>
+#include <linux/overflow.h>
#include <linux/rculist.h>
#include <linux/rcupdate.h>
#include <linux/skbuff.h>
@@ -815,8 +816,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)
@@ -835,7 +835,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);
@@ -895,8 +895,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)
@@ -915,7 +914,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)
@@ -2875,7 +2874,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;
@@ -2891,7 +2889,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;
@@ -2903,12 +2901,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++;
}
@@ -2960,7 +2956,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;
@@ -2983,10 +2978,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;
@@ -3329,7 +3323,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",
@@ -3342,10 +3335,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) {
@@ -3944,10 +3934,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;
@@ -3957,17 +3947,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);
}
/**
@@ -3998,8 +3989,8 @@ 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,
+ ntohs(tt_data->num_vlan));
if (tvlv_value_len < tt_vlan_len)
return NET_RX_SUCCESS;