Skip to content

Commit

Permalink
net: batman-adv: fix error handling
Browse files Browse the repository at this point in the history
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: c6c8fea ("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>
  • Loading branch information
Pavel Skripkin authored and David S. Miller committed Oct 26, 2021
1 parent fa40d97 commit 6f68cd6
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 20 deletions.
8 changes: 6 additions & 2 deletions net/batman-adv/bridge_loop_avoidance.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
56 changes: 40 additions & 16 deletions net/batman-adv/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down
4 changes: 3 additions & 1 deletion net/batman-adv/network-coding.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion net/batman-adv/translation-table.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 6f68cd6

Please sign in to comment.