| From 6f68cd634856f8ca93bafd623ba5357e0f648c68 Mon Sep 17 00:00:00 2001 |
| From: Pavel Skripkin <paskripkin@gmail.com> |
| Date: Sun, 24 Oct 2021 16:13:56 +0300 |
| Subject: net: batman-adv: fix error handling |
| |
| From: Pavel Skripkin <paskripkin@gmail.com> |
| |
| commit 6f68cd634856f8ca93bafd623ba5357e0f648c68 upstream. |
| |
| Syzbot reported ODEBUG warning in batadv_nc_mesh_free(). The problem was |
| in wrong error handling in batadv_mesh_init(). |
| |
| Before this patch batadv_mesh_init() was calling batadv_mesh_free() in case |
| of any batadv_*_init() calls failure. This approach may work well, when |
| there is some kind of indicator, which can tell which parts of batadv are |
| initialized; but there isn't any. |
| |
| All written above lead to cleaning up uninitialized fields. Even if we hide |
| ODEBUG warning by initializing bat_priv->nc.work, syzbot was able to hit |
| GPF in batadv_nc_purge_paths(), because hash pointer in still NULL. [1] |
| |
| To fix these bugs we can unwind batadv_*_init() calls one by one. |
| It is good approach for 2 reasons: 1) It fixes bugs on error handling |
| path 2) It improves the performance, since we won't call unneeded |
| batadv_*_free() functions. |
| |
| So, this patch makes all batadv_*_init() clean up all allocated memory |
| before returning with an error to no call correspoing batadv_*_free() |
| and open-codes batadv_mesh_free() with proper order to avoid touching |
| uninitialized fields. |
| |
| Link: https://lore.kernel.org/netdev/000000000000c87fbd05cef6bcb0@google.com/ [1] |
| Reported-and-tested-by: syzbot+28b0702ada0bf7381f58@syzkaller.appspotmail.com |
| Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol") |
| Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> |
| Acked-by: Sven Eckelmann <sven@narfation.org> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| net/batman-adv/bridge_loop_avoidance.c | 8 +++- |
| net/batman-adv/main.c | 56 +++++++++++++++++++++++---------- |
| net/batman-adv/network-coding.c | 4 +- |
| net/batman-adv/translation-table.c | 4 +- |
| 4 files changed, 52 insertions(+), 20 deletions(-) |
| |
| --- a/net/batman-adv/bridge_loop_avoidance.c |
| +++ b/net/batman-adv/bridge_loop_avoidance.c |
| @@ -1561,10 +1561,14 @@ int batadv_bla_init(struct batadv_priv * |
| return 0; |
| |
| bat_priv->bla.claim_hash = batadv_hash_new(128); |
| - bat_priv->bla.backbone_hash = batadv_hash_new(32); |
| + if (!bat_priv->bla.claim_hash) |
| + return -ENOMEM; |
| |
| - if (!bat_priv->bla.claim_hash || !bat_priv->bla.backbone_hash) |
| + bat_priv->bla.backbone_hash = batadv_hash_new(32); |
| + if (!bat_priv->bla.backbone_hash) { |
| + batadv_hash_destroy(bat_priv->bla.claim_hash); |
| return -ENOMEM; |
| + } |
| |
| batadv_hash_set_lock_class(bat_priv->bla.claim_hash, |
| &batadv_claim_hash_lock_class_key); |
| --- a/net/batman-adv/main.c |
| +++ b/net/batman-adv/main.c |
| @@ -197,29 +197,41 @@ int batadv_mesh_init(struct net_device * |
| |
| bat_priv->gw.generation = 0; |
| |
| - ret = batadv_v_mesh_init(bat_priv); |
| - if (ret < 0) |
| - goto err; |
| - |
| ret = batadv_originator_init(bat_priv); |
| - if (ret < 0) |
| - goto err; |
| + if (ret < 0) { |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_DEACTIVATING); |
| + goto err_orig; |
| + } |
| |
| ret = batadv_tt_init(bat_priv); |
| - if (ret < 0) |
| - goto err; |
| + if (ret < 0) { |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_DEACTIVATING); |
| + goto err_tt; |
| + } |
| + |
| + ret = batadv_v_mesh_init(bat_priv); |
| + if (ret < 0) { |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_DEACTIVATING); |
| + goto err_v; |
| + } |
| |
| ret = batadv_bla_init(bat_priv); |
| - if (ret < 0) |
| - goto err; |
| + if (ret < 0) { |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_DEACTIVATING); |
| + goto err_bla; |
| + } |
| |
| ret = batadv_dat_init(bat_priv); |
| - if (ret < 0) |
| - goto err; |
| + if (ret < 0) { |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_DEACTIVATING); |
| + goto err_dat; |
| + } |
| |
| ret = batadv_nc_mesh_init(bat_priv); |
| - if (ret < 0) |
| - goto err; |
| + if (ret < 0) { |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_DEACTIVATING); |
| + goto err_nc; |
| + } |
| |
| batadv_gw_init(bat_priv); |
| batadv_mcast_init(bat_priv); |
| @@ -229,8 +241,20 @@ int batadv_mesh_init(struct net_device * |
| |
| return 0; |
| |
| -err: |
| - batadv_mesh_free(soft_iface); |
| +err_nc: |
| + batadv_dat_free(bat_priv); |
| +err_dat: |
| + batadv_bla_free(bat_priv); |
| +err_bla: |
| + batadv_v_mesh_free(bat_priv); |
| +err_v: |
| + batadv_tt_free(bat_priv); |
| +err_tt: |
| + batadv_originator_free(bat_priv); |
| +err_orig: |
| + batadv_purge_outstanding_packets(bat_priv, NULL); |
| + atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE); |
| + |
| return ret; |
| } |
| |
| --- a/net/batman-adv/network-coding.c |
| +++ b/net/batman-adv/network-coding.c |
| @@ -155,8 +155,10 @@ int batadv_nc_mesh_init(struct batadv_pr |
| &batadv_nc_coding_hash_lock_class_key); |
| |
| bat_priv->nc.decoding_hash = batadv_hash_new(128); |
| - if (!bat_priv->nc.decoding_hash) |
| + if (!bat_priv->nc.decoding_hash) { |
| + batadv_hash_destroy(bat_priv->nc.coding_hash); |
| goto err; |
| + } |
| |
| batadv_hash_set_lock_class(bat_priv->nc.decoding_hash, |
| &batadv_nc_decoding_hash_lock_class_key); |
| --- a/net/batman-adv/translation-table.c |
| +++ b/net/batman-adv/translation-table.c |
| @@ -4405,8 +4405,10 @@ int batadv_tt_init(struct batadv_priv *b |
| return ret; |
| |
| ret = batadv_tt_global_init(bat_priv); |
| - if (ret < 0) |
| + if (ret < 0) { |
| + batadv_tt_local_table_free(bat_priv); |
| return ret; |
| + } |
| |
| batadv_tvlv_handler_register(bat_priv, batadv_tt_tvlv_ogm_handler_v1, |
| batadv_tt_tvlv_unicast_handler_v1, |