| Message ID | AS8PR02MB7237987BF9DFCA030B330F658B3E2@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | batman-adv: Add flex array to struct batadv_tvlv_tt_data | expand |
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. 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
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
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;
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) 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(-)