| Message ID | AS8PR02MB72371F89D188B047410B755E8B192@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] batman-adv: Add flex array to struct batadv_tvlv_tt_data | expand |
On Wed, May 01, 2024 at 05:02:42PM +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 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] > Signed-off-by: Erick Archer <erick.archer@outlook.com> Thanks for the tweak! Reviewed-by: Kees Cook <keescook@chromium.org>
On Wednesday, 1 May 2024 17:02:42 CEST Erick Archer wrote: > diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h > index 6e25753015df..dfbe30536995 100644 > --- a/include/uapi/linux/batadv_packet.h > +++ b/include/uapi/linux/batadv_packet.h [...] > +/** > + * 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); > +}; Thanks for the updates. But I can't accept this at the moment because __counted_by_be is used in an uapi header without it being defined include/uapi/linux/stddef.h (and this file is also not included in this header). See commit c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and identifier expansion") as an example for the similar __counted_by macro. Kind regards, Sven
Hi Sven, On Sat, May 04, 2024 at 11:35:38AM +0200, Sven Eckelmann wrote: > On Wednesday, 1 May 2024 17:02:42 CEST Erick Archer wrote: > > diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h > > index 6e25753015df..dfbe30536995 100644 > > --- a/include/uapi/linux/batadv_packet.h > > +++ b/include/uapi/linux/batadv_packet.h > [...] > > +/** > > + * 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); > > +}; > > Thanks for the updates. But I can't accept this at the moment because > __counted_by_be is used in an uapi header without it being defined > include/uapi/linux/stddef.h (and this file is also not included in this > header). > > See commit c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and > identifier expansion") as an example for the similar __counted_by macro. If I understand correctly, the following changes are also needed because the annotated struct is defined in a "uapi" header. Sorry if it's a stupid question, but I'm new to these topics. diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h index 6e25753015df..41f39d7661c9 100644 --- a/include/uapi/linux/batadv_packet.h +++ b/include/uapi/linux/batadv_packet.h @@ -9,6 +9,7 @@ #include <asm/byteorder.h> #include <linux/if_ether.h> +#include <linux/stddef.h> #include <linux/types.h> /** diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h index 2ec6f35cda32..58154117d9b0 100644 --- a/include/uapi/linux/stddef.h +++ b/include/uapi/linux/stddef.h @@ -55,4 +55,12 @@ #define __counted_by(m) #endif +#ifndef __counted_by_le +#define __counted_by_le(m) +#endif + +#ifndef __counted_by_be +#define __counted_by_be(m) +#endif + #endif /* _UAPI_LINUX_STDDEF_H */ If this is the right path, can these changes be merged into a single patch or is it better to add a previous patch to define __counted_by{le,be}? Regards, Erick > Kind regards, > Sven
On Saturday, 4 May 2024 19:08:39 CEST Erick Archer wrote: [...] > > Thanks for the updates. But I can't accept this at the moment because > > __counted_by_be is used in an uapi header without it being defined > > include/uapi/linux/stddef.h (and this file is also not included in this > > header). > > > > See commit c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and > > identifier expansion") as an example for the similar __counted_by macro. > > If I understand correctly, the following changes are also needed because > the annotated struct is defined in a "uapi" header. Sorry if it's a stupid > question, but I'm new to these topics. No, it is absolutely no stupid question. > diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h > index 6e25753015df..41f39d7661c9 100644 > --- a/include/uapi/linux/batadv_packet.h > +++ b/include/uapi/linux/batadv_packet.h > @@ -9,6 +9,7 @@ > > #include <asm/byteorder.h> > #include <linux/if_ether.h> > +#include <linux/stddef.h> > #include <linux/types.h> > > /** This must definitely go into your "original" patch > diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h > index 2ec6f35cda32..58154117d9b0 100644 > --- a/include/uapi/linux/stddef.h > +++ b/include/uapi/linux/stddef.h > @@ -55,4 +55,12 @@ > #define __counted_by(m) > #endif > > +#ifndef __counted_by_le > +#define __counted_by_le(m) > +#endif If you want to add this (for completeness) then please put it in an extra patch. It is simply not used by batman-adv and I would not be able to find any justification why it should be part of the batman-adv patch. > + > +#ifndef __counted_by_be > +#define __counted_by_be(m) > +#endif > + This part can be either: * in the batman-adv patch * or together with the __counted_by_le change in an additional patch which is "in front" of the batman-adv patch (in the patch series). From my perspective, it is for you to decide - but of course, other maintainers might have a different opinion about it. > #endif /* _UAPI_LINUX_STDDEF_H */ > > If this is the right path, can these changes be merged into a > single patch or is it better to add a previous patch to define > __counted_by{le,be}? I don't have a perfect answer here. See the comments above. The file include/uapi/linux/stddef.h doesn't have a specific maintainer (according to ./scripts/get_maintainer.pl) - so it should be fine to get modified through the net-next tree. But maybe Kees Cook has a different opinion about it. At least there are a lot of Signed-off-bys for this file by Kees. Kind regards, Sven
On Sat, May 04, 2024 at 07:08:39PM +0200, Erick Archer wrote: > Hi Sven, > > On Sat, May 04, 2024 at 11:35:38AM +0200, Sven Eckelmann wrote: > > On Wednesday, 1 May 2024 17:02:42 CEST Erick Archer wrote: > > > diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h > > > index 6e25753015df..dfbe30536995 100644 > > > --- a/include/uapi/linux/batadv_packet.h > > > +++ b/include/uapi/linux/batadv_packet.h > > [...] > > > +/** > > > + * 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); > > > +}; > > > > Thanks for the updates. But I can't accept this at the moment because > > __counted_by_be is used in an uapi header without it being defined > > include/uapi/linux/stddef.h (and this file is also not included in this > > header). > > > > See commit c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and > > identifier expansion") as an example for the similar __counted_by macro. > > If I understand correctly, the following changes are also needed because > the annotated struct is defined in a "uapi" header. Sorry if it's a stupid > question, but I'm new to these topics. > > diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h > index 6e25753015df..41f39d7661c9 100644 > --- a/include/uapi/linux/batadv_packet.h > +++ b/include/uapi/linux/batadv_packet.h > @@ -9,6 +9,7 @@ > > #include <asm/byteorder.h> > #include <linux/if_ether.h> > +#include <linux/stddef.h> > #include <linux/types.h> > > /** > diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h > index 2ec6f35cda32..58154117d9b0 100644 > --- a/include/uapi/linux/stddef.h > +++ b/include/uapi/linux/stddef.h > @@ -55,4 +55,12 @@ > #define __counted_by(m) > #endif > > +#ifndef __counted_by_le > +#define __counted_by_le(m) > +#endif > + > +#ifndef __counted_by_be > +#define __counted_by_be(m) > +#endif > + > #endif /* _UAPI_LINUX_STDDEF_H */ Yup, this is needed for UAPI as you have it. Thanks! I should have noticed the need for this when I reviewed commit ca7e324e8ad3 ("compiler_types: add Endianness-dependent __counted_by_{le,be}"). > If this is the right path, can these changes be merged into a > single patch or is it better to add a previous patch to define > __counted_by{le,be}? We're almost on top of the merge window, so how about this: send me a patch for just the UAPI addition, and I'll include it in this coming (next week expected) merge window. Once -rc2 is out, re-send this batman-adv patch since then netdev will be merged with -rc2 and the UAPI change will be there. -Kees
On Sun, May 05, 2024 at 05:22:10PM +0200, Sven Eckelmann wrote: > On Saturday, 4 May 2024 19:08:39 CEST Erick Archer wrote: > [...] > > > Thanks for the updates. But I can't accept this at the moment because > > > __counted_by_be is used in an uapi header without it being defined > > > include/uapi/linux/stddef.h (and this file is also not included in this > > > header). > > > > > > See commit c8248faf3ca2 ("Compiler Attributes: counted_by: Adjust name and > > > identifier expansion") as an example for the similar __counted_by macro. > > > > If I understand correctly, the following changes are also needed because > > the annotated struct is defined in a "uapi" header. Sorry if it's a stupid > > question, but I'm new to these topics. > > No, it is absolutely no stupid question. > > > diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h > > index 6e25753015df..41f39d7661c9 100644 > > --- a/include/uapi/linux/batadv_packet.h > > +++ b/include/uapi/linux/batadv_packet.h > > @@ -9,6 +9,7 @@ > > > > #include <asm/byteorder.h> > > #include <linux/if_ether.h> > > +#include <linux/stddef.h> > > #include <linux/types.h> > > > > /** > > This must definitely go into your "original" patch > > > diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h > > index 2ec6f35cda32..58154117d9b0 100644 > > --- a/include/uapi/linux/stddef.h > > +++ b/include/uapi/linux/stddef.h > > @@ -55,4 +55,12 @@ > > #define __counted_by(m) > > #endif > > > > +#ifndef __counted_by_le > > +#define __counted_by_le(m) > > +#endif > > If you want to add this (for completeness) then please put it in an extra > patch. It is simply not used by batman-adv and I would not be able to find any > justification why it should be part of the batman-adv patch. > > > + > > +#ifndef __counted_by_be > > +#define __counted_by_be(m) > > +#endif > > + > > This part can be either: > > * in the batman-adv patch > * or together with the __counted_by_le change in an additional patch which is > "in front" of the batman-adv patch (in the patch series). > > From my perspective, it is for you to decide - but of course, other > maintainers might have a different opinion about it. > > > #endif /* _UAPI_LINUX_STDDEF_H */ > > > > If this is the right path, can these changes be merged into a > > single patch or is it better to add a previous patch to define > > __counted_by{le,be}? > > I don't have a perfect answer here. See the comments above. The file > include/uapi/linux/stddef.h doesn't have a specific maintainer (according to > ./scripts/get_maintainer.pl) - so it should be fine to get modified through > the net-next tree. > > But maybe Kees Cook has a different opinion about it. At least there are a lot > of Signed-off-bys for this file by Kees. FWIW, I'm also fine with the UAPI going in via netdev. It's the most likely place to use the be/le variants. Is netdev still open for patches? Whatever the path, we should get it into this coming merge window so we can use it elsewhere too if we need it.
Hi Sven and Kees, First of all, thanks for the reviews, comments and advices. On Mon, May 06, 2024 at 09:27:46AM -0700, Kees Cook wrote: > On Sat, May 04, 2024 at 07:08:39PM +0200, Erick Archer wrote: > > > If this is the right path, can these changes be merged into a > > single patch or is it better to add a previous patch to define > > __counted_by{le,be}? > > We're almost on top of the merge window, so how about this: send me a > patch for just the UAPI addition, and I'll include it in this coming (next > week expected) merge window. Once -rc2 is out, re-send this batman-adv > patch since then netdev will be merged with -rc2 and the UAPI change > will be there. > Ok, I will follow these steps. The patch for the UAPI addition has already been sent. When the -rc2 comes out, I will resend this patch. Again, thanks, Erick > -Kees > > -- > Kees Cook
diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h index 6e25753015df..dfbe30536995 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[] __counted_by_be(num_vlan); +}; + /** * 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 b21ff3c36b07..b44c382226a1 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -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 *)((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); } /** @@ -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;
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] 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. Changes in v3: - Address from the base pointer tt_data to avoid an "out-of-bounds" notification (Kees Cook). - Update the commit message to explain the new change. Previous versions: v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237987BF9DFCA030B330F658B3E2@AS8PR02MB7237.eurprd02.prod.outlook.com/ v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB723756E3D1366C4E8FCD14BF8B162@AS8PR02MB7237.eurprd02.prod.outlook.com/ Hi, 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(-)