net: batman-adv: fix error handling

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/
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>

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index 1669744..1768784 100644 (file)
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -1560,10 +1560,14 @@ int batadv_bla_init(struct batadv_priv *bat_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);

diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c
index 3ddd66e..5207cd8 100644 (file)
--- a/net/batman-adv/main.c
+++ b/net/batman-adv/main.c
@@ -190,29 +190,41 @@ int batadv_mesh_init(struct net_device *soft_iface)
 
        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);
@@ -222,8 +234,20 @@ int batadv_mesh_init(struct net_device *soft_iface)
 
        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;
 }
 

diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c
index 9f06132..0a7f1d3 100644 (file)
--- a/net/batman-adv/network-coding.c
+++ b/net/batman-adv/network-coding.c
@@ -152,8 +152,10 @@ int batadv_nc_mesh_init(struct batadv_priv *bat_priv)
                                   &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);

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index e0b3dac..4b7ad66 100644 (file)
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -4162,8 +4162,10 @@ int batadv_tt_init(struct batadv_priv *bat_priv)
                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,