[PATCH] batman-adv: Fix incorrect offset in batadv_tt_tvlv_ogm_handler_v1()

[PATCH] batman-adv: Fix incorrect offset in batadv_tt_tvlv_ogm_handler_v1()

[Remi Pommarel]

Since commit 4436df478860 ("batman-adv: Add flex array to struct
batadv_tvlv_tt_data"), the introduction of batadv_tvlv_tt_data's flex
array member in batadv_tt_tvlv_ogm_handler_v1() put tt_changes at
invalid offset. Those TT changes are supposed to be filled from the end
of batadv_tvlv_tt_data structure (including vlan_data flexible array),
but only the flex array size is taken into account missing completely
the size of the fixed part of the structure itself.

Fix the tt_change offset computation by using struct_size() instead of
flex_array_size() so both flex array member and its container structure
sizes are taken into account.

Fixes: 4436df478860 ("batman-adv: Add flex array to struct batadv_tvlv_tt_data")
Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 net/batman-adv/translation-table.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 3c0a14a582e4..d4b71d34310f 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -3937,23 +3937,21 @@ static void batadv_tt_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
 	struct batadv_tvlv_tt_change *tt_change;
 	struct batadv_tvlv_tt_data *tt_data;
 	u16 num_entries, num_vlan;
-	size_t flex_size;
+	size_t tt_data_sz;
 
 	if (tvlv_value_len < sizeof(*tt_data))
 		return;
 
 	tt_data = tvlv_value;
-	tvlv_value_len -= sizeof(*tt_data);
-
 	num_vlan = ntohs(tt_data->num_vlan);
 
-	flex_size = flex_array_size(tt_data, vlan_data, num_vlan);
-	if (tvlv_value_len < flex_size)
+	tt_data_sz = struct_size(tt_data, vlan_data, num_vlan);
+	if (tvlv_value_len < tt_data_sz)
 		return;
 
 	tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data
-						     + flex_size);
-	tvlv_value_len -= flex_size;
+						     + tt_data_sz);
+	tvlv_value_len -= tt_data_sz;
 
 	num_entries = batadv_tt_entries(tvlv_value_len);
 
-- 
2.40.0

Re: [PATCH] batman-adv: Fix incorrect offset in batadv_tt_tvlv_ogm_handler_v1()

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 5216 bytes --]

On Tuesday, 28 January 2025 16:11:06 GMT+1 Remi Pommarel wrote:
> Since commit 4436df478860 ("batman-adv: Add flex array to struct
> batadv_tvlv_tt_data"), the introduction of batadv_tvlv_tt_data's flex
> array member in batadv_tt_tvlv_ogm_handler_v1() put tt_changes at
> invalid offset. Those TT changes are supposed to be filled from the end
> of batadv_tvlv_tt_data structure (including vlan_data flexible array),
> but only the flex array size is taken into account missing completely
> the size of the fixed part of the structure itself.
> 
> Fix the tt_change offset computation by using struct_size() instead of
> flex_array_size() so both flex array member and its container structure
> sizes are taken into account.
> 
> Fixes: 4436df478860 ("batman-adv: Add flex array to struct batadv_tvlv_tt_data")
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>

Thanks for the patch. I just wanted to dump my notes here (because it is 
getting a little late).


Original calculation was:

1. tvlv_value_len -= 4 [sizeof(*tt_data)]
2. check if tvlv_value_len contains at least num_vlan * 8 bytes [sizeof(*tt_vlan)]
3. tt_vlan = vlan section at offset 4 [sizeof(*tt_data)]
4. tt_change = change section at offset offset 4 [sizeof(*tt_data)] + num_vlan * 8 bytes [sizeof(*tt_vlan)]
5. tvlv_value_len was reduced by num_vlan * 8 bytes [sizeof(*tt_vlan)]
6. num_entries was calculated using tvlv_value_len / 12 [sizeof(batadv_tvlv_tt_change)]

result:

* tt_vlan = tt_data + 4
* tt_change = tt_data + 4 + num_vlan * 8
* num_entries = (tvlv_value_len - (4 + num_vlan * 8)) / 12


After Erick's change

1. tvlv_value_len -= 4 [sizeof(*tt_data)]
2. calculation of the flexible (vlan) part as num_vlan * 8 [sizeof(tt_data->vlan_data)]
3. check if tvlv_value_len contains at the flexible (vlan) part
4. tt_change = change section at offset num_vlan * 8 bytes [sizeof(*tt_vlan)]
   (which is wrong by 4 bytes)
5. tvlv_value_len was reduced by num_vlan * 8 bytes [sizeof(*tt_vlan)]
6. num_entries was calculated using tvlv_value_len / 12 [sizeof(batadv_tvlv_tt_change)]
7. "tt_vlan" is implicitly used from offset  4 [tt_data->ttvn]

result:

* tt_vlan = tt_data + 4
* tt_change = tt_data + num_vlan * 8
* num_entries = (tvlv_value_len - (4 + num_vlan * 8)) / 12


The broken part of the change was basically following:

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


if the line

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

would have been replaced with

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

then it should also have worked.

(calculation for this changes follows below)

> ---
>  net/batman-adv/translation-table.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> index 3c0a14a582e4..d4b71d34310f 100644
> --- a/net/batman-adv/translation-table.c
> +++ b/net/batman-adv/translation-table.c
> @@ -3937,23 +3937,21 @@ static void batadv_tt_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
>  	struct batadv_tvlv_tt_change *tt_change;
>  	struct batadv_tvlv_tt_data *tt_data;
>  	u16 num_entries, num_vlan;
> -	size_t flex_size;
> +	size_t tt_data_sz;
>  
>  	if (tvlv_value_len < sizeof(*tt_data))
>  		return;
>  
>  	tt_data = tvlv_value;
> -	tvlv_value_len -= sizeof(*tt_data);
> -
>  	num_vlan = ntohs(tt_data->num_vlan);
>  
> -	flex_size = flex_array_size(tt_data, vlan_data, num_vlan);
> -	if (tvlv_value_len < flex_size)
> +	tt_data_sz = struct_size(tt_data, vlan_data, num_vlan);
> +	if (tvlv_value_len < tt_data_sz)
>  		return;
>  
>  	tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data
> -						     + flex_size);
> -	tvlv_value_len -= flex_size;
> +						     + tt_data_sz);
> +	tvlv_value_len -= tt_data_sz;
>  
>  	num_entries = batadv_tt_entries(tvlv_value_len);


Remi's change:

1. size of first data part is calculated using 4 [sizeof(*tt_data)] + num_vlan * 8 bytes [sizeof(*tt_vlan)]
2. check if tvlv_value_len contains at least 4 [sizeof(*tt_data)] + num_vlan * 8 bytes [sizeof(*tt_vlan)]
3. tt_change = change section at offset offset 4 [sizeof(*tt_data)] + num_vlan * 8 bytes [sizeof(*tt_vlan)]
4. tvlv_value_len was reduced by 4 [sizeof(*tt_data)] + num_vlan * 8 bytes [sizeof(*tt_vlan)]
5. num_entries was calculated using tvlv_value_len / 12 [sizeof(batadv_tvlv_tt_change)]
6. "tt_vlan" is implicitly used from offset 4 [tt_data->ttvn]

result:

* tt_vlan = tt_data + 4
* tt_change = tt_data + 4 + num_vlan * 8
* num_entries = (tvlv_value_len - (4 + num_vlan * 8)) / 12

Sounds at least at the moment like a plausible fix. I have already queued it 
up for batadv/net but will leave it for a moment to allow others to review it 
too.

Thanks,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] batman-adv: Fix incorrect offset in batadv_tt_tvlv_ogm_handler_v1()

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

On Tuesday, 28 January 2025 23:18:06 GMT+1 Sven Eckelmann wrote:
> "tt_vlan" is implicitly used from offset  4 [tt_data->ttvn]

tt_data->vlan_data not tt_data->ttvn

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] batman-adv: Fix incorrect offset in batadv_tt_tvlv_ogm_handler_v1()

[Remi Pommarel]

On Tue, Jan 28, 2025 at 11:18:06PM +0100, Sven Eckelmann wrote:
> On Tuesday, 28 January 2025 16:11:06 GMT+1 Remi Pommarel wrote:
> > Since commit 4436df478860 ("batman-adv: Add flex array to struct
> > batadv_tvlv_tt_data"), the introduction of batadv_tvlv_tt_data's flex
> > array member in batadv_tt_tvlv_ogm_handler_v1() put tt_changes at
> > invalid offset. Those TT changes are supposed to be filled from the end
> > of batadv_tvlv_tt_data structure (including vlan_data flexible array),
> > but only the flex array size is taken into account missing completely
> > the size of the fixed part of the structure itself.
> > 
> > Fix the tt_change offset computation by using struct_size() instead of
> > flex_array_size() so both flex array member and its container structure
> > sizes are taken into account.
> > 
> > Fixes: 4436df478860 ("batman-adv: Add flex array to struct batadv_tvlv_tt_data")
> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> 
> Thanks for the patch. I just wanted to dump my notes here (because it is 
> getting a little late).
> 
> 
> Original calculation was:
> 
> 1. tvlv_value_len -= 4 [sizeof(*tt_data)]
> 2. check if tvlv_value_len contains at least num_vlan * 8 bytes [sizeof(*tt_vlan)]
> 3. tt_vlan = vlan section at offset 4 [sizeof(*tt_data)]
> 4. tt_change = change section at offset offset 4 [sizeof(*tt_data)] + num_vlan * 8 bytes [sizeof(*tt_vlan)]
> 5. tvlv_value_len was reduced by num_vlan * 8 bytes [sizeof(*tt_vlan)]
> 6. num_entries was calculated using tvlv_value_len / 12 [sizeof(batadv_tvlv_tt_change)]
> 
> result:
> 
> * tt_vlan = tt_data + 4
> * tt_change = tt_data + 4 + num_vlan * 8
> * num_entries = (tvlv_value_len - (4 + num_vlan * 8)) / 12
> 
> 
> After Erick's change
> 
> 1. tvlv_value_len -= 4 [sizeof(*tt_data)]
> 2. calculation of the flexible (vlan) part as num_vlan * 8 [sizeof(tt_data->vlan_data)]
> 3. check if tvlv_value_len contains at the flexible (vlan) part
> 4. tt_change = change section at offset num_vlan * 8 bytes [sizeof(*tt_vlan)]
>    (which is wrong by 4 bytes)
> 5. tvlv_value_len was reduced by num_vlan * 8 bytes [sizeof(*tt_vlan)]
> 6. num_entries was calculated using tvlv_value_len / 12 [sizeof(batadv_tvlv_tt_change)]
> 7. "tt_vlan" is implicitly used from offset  4 [tt_data->ttvn]
> 
> result:
> 
> * tt_vlan = tt_data + 4
> * tt_change = tt_data + num_vlan * 8
> * num_entries = (tvlv_value_len - (4 + num_vlan * 8)) / 12
> 
> 
> The broken part of the change was basically following:
> 
> -       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;
> 
> 
> if the line
> 
> +       tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data
> +                                                    + flex_size);
> 
> would have been replaced with
> 
> +       tt_change = (struct batadv_tvlv_tt_change *)((void *)tt_data->vlan_data
> +                                                    + flex_size);
> 
> then it should also have worked.

Erick's initial patch was almost doing that but Kees emitted concern
that this could bother the compiler bound checker and suggest the
current flawed logic [0] (hence him in CC). I wasn't sure the (void *)
cast would prevent the bound checker to complain here, so I tried to
also follow the "addressing from the base pointer" strategy Kees
mentioned.

On a side note, I am all about hardening and memory safety stuff but
if that means impacting readability and spending more time trying to
please the tool than thinking about the correctness of the code change,
that's where we end up converting a perfectly fine code into a
logically flawed one.

[0]: https://lore.kernel.org/lkml/202404291030.F760C26@keescook/

-- 
Remi