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

[Sven Eckelmann <sven@narfation.org>]

[-- 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 --]