Bug #354
Updated by Sven Eckelmann over 6 years ago
There is the possibility that batadv_tt_prepare_tvlv_global_data/batadv_tt_prepare_tvlv_local_data and any function operating on the buffer is causing a an buffer overflow Here some details from IRC <pre> <ecsv_> hm, isn't the batadv_tt_prepare_tvlv_global_data buggy? <ecsv_> it traverses the vlan_list twice in hope that it returns the same number of entries <ecsv_> this doesn't have to be true because it is not locked with a mutex/spinlock <ecsv_> let us assume that something adds a vlan to vlan_list while batadv_tt_prepare_tvlv_global_data is running <ecsv_> and this happens on a core so that the current core first sees the new entry in the list after the kmalloc <ecsv_> the second loop will traverse the list again and then at the end write an entry more than it is expected to write to the buffer <ecsv_> or is tt_buff_lock also locked around the vlan/tt entry del/add functions? <ecsv_> ah, no. it isn't. for example the non-full table doesn't use it in batadv_send_other_tt_response </pre> Potential fix is: <pre> <ecsv_> oh noes, it is even worse. the buffer is also modified outside these functions. for example in batadv_tt_tvlv_container_update or batadv_tt_tvlv_generate. so it is not enough to just take the bat_priv->softif_vlan_list_lock (or orig_node->vlan_list_lock) diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 0225616d..7fa3a0a0 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -862,7 +862,7 @@ batadv_tt_prepare_tvlv_global_data(struct batadv_orig_node *orig_node, struct batadv_orig_node_vlan *vlan; u8 *tt_change_ptr; - rcu_read_lock(); + spin_lock_bh(&orig_node->vlan_list_lock); hlist_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) { num_vlan++; num_entries += atomic_read(&vlan->tt.num_entries); @@ -900,7 +900,7 @@ batadv_tt_prepare_tvlv_global_data(struct batadv_orig_node *orig_node, *tt_change = (struct batadv_tvlv_tt_change *)tt_change_ptr; out: - rcu_read_unlock(); + spin_unlock_bh(&orig_node->vlan_list_lock); return tvlv_len; } @@ -936,7 +936,7 @@ batadv_tt_prepare_tvlv_local_data(struct batadv_priv *bat_priv, u8 *tt_change_ptr; int change_offset; - rcu_read_lock(); + spin_lock_bh(&bat_priv->softif_vlan_list_lock); hlist_for_each_entry_rcu(vlan, &bat_priv->softif_vlan_list, list) { num_vlan++; num_entries += atomic_read(&vlan->tt.num_entries); @@ -974,7 +974,7 @@ batadv_tt_prepare_tvlv_local_data(struct batadv_priv *bat_priv, *tt_change = (struct batadv_tvlv_tt_change *)tt_change_ptr; out: - rcu_read_unlock(); + spin_unlock_bh(&bat_priv->softif_vlan_list_lock); return tvlv_len; } </pre>