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