Skip to content

Commit

Permalink
batman-adv: Add flex array to struct batadv_tvlv_tt_data
Browse files Browse the repository at this point in the history 
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 following change to the "batadv_tt_tvlv_ogm_handler_v1" function:

-	tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(tt_data + 1);
-	tt_change = (struct batadv_tvlv_tt_change *)(tt_vlan + num_vlan);

+	tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data
+						     + flex_size);

is intended to prevent the compiler from generating an "out-of-bounds"
notification due to the __counted_by attribute. The compiler can do a
pointer calculation using the vlan_data flexible array memory, or in
other words, this may be calculated as an array offset, since it is the
same as:

        &tt_data->vlan_data[num_vlan]

Therefore, we go past the end of the array. In other "multiple trailing
flexible array" situations, this has been solved by 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).

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]
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Erick Archer <erick.archer@outlook.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
  • Loading branch information
Erick Archer authored and Simon Wunderlich committed Oct 5, 2024
1 parent 0f4e6f9 commit 4436df4
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 42 deletions.
29 changes: 16 additions & 13 deletions include/uapi/linux/batadv_packet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include <asm/byteorder.h>
#include <linux/if_ether.h>
#include <linux/stddef.h>
#include <linux/types.h>

/**
Expand Down Expand Up @@ -592,19 +593,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
Expand All @@ -618,6 +606,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
Expand Down
49 changes: 20 additions & 29 deletions net/batman-adv/translation-table.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -856,8 +857,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)
Expand All @@ -876,7 +876,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);
Expand Down Expand Up @@ -936,8 +936,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)
Expand All @@ -956,7 +955,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)
Expand Down Expand Up @@ -2916,7 +2915,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;
Expand All @@ -2932,7 +2930,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;
Expand All @@ -2944,12 +2942,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++;
}

Expand Down Expand Up @@ -3001,7 +2997,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;
Expand All @@ -3024,10 +3019,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;

Expand Down Expand Up @@ -3370,7 +3364,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",
Expand All @@ -3383,10 +3376,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) {
Expand Down Expand Up @@ -3985,10 +3975,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;
Expand All @@ -3998,17 +3988,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 *)((void *)tt_data
+ flex_size);
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);
}

/**
Expand Down Expand Up @@ -4039,8 +4030,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;
Expand Down

0 comments on commit 4436df4

Please sign in to comment.