| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2593315.VLH7GnMWUR@sven-l14>
Date: Tue, 28 Jan 2025 23:18:06 +0100
From: Sven Eckelmann <sven@...fation.org>
To: b.a.t.m.a.n@...ts.open-mesh.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Remi Pommarel <repk@...plefau.lt>
Cc: Marek Lindner <marek.lindner@...lbox.org>,
Simon Wunderlich <sw@...onwunderlich.de>,
Erick Archer <erick.archer@...look.com>, Kees Cook <kees@...nel.org>,
Remi Pommarel <repk@...plefau.lt>
Subject:
Re: [PATCH] batman-adv: Fix incorrect offset in
batadv_tt_tvlv_ogm_handler_v1()
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@...plefau.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
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists