| Message ID | 20211024131356.10699-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6f68cd634856f8ca93bafd623ba5357e0f648c68 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: batman-adv: fix error handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 115 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Sunday, 24 October 2021 15:13:56 CEST Pavel Skripkin wrote: > 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> Kind regards, Sven
On Sun, 24 Oct 2021 16:58:30 +0200 Sven Eckelmann wrote: > On Sunday, 24 October 2021 15:13:56 CEST Pavel Skripkin wrote: > > 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> FWIW I'm marking this as "Awaiting upstream" in netdev patchwork, please LMK if you prefer for it to be applied directly.
On Tuesday, 26 October 2021 02:49:50 CEST Jakub Kicinski wrote: [...] > > Acked-by: Sven Eckelmann <sven@narfation.org> > > FWIW I'm marking this as "Awaiting upstream" in netdev patchwork, > please LMK if you prefer for it to be applied directly. Please apply this directly. Thanks Kind regards, Sven
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Sun, 24 Oct 2021 16:13:56 +0300 you wrote: > 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. > > [...] Here is the summary with links: - net: batman-adv: fix error handling https://git.kernel.org/netdev/net/c/6f68cd634856 You are awesome, thank you!
diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c index 1669744304c5..17687848daec 100644 --- 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 3ddd66e4c29e..5207cd8d6ad8 100644 --- 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 9f06132e007d..0a7f1d36a6a8 100644 --- 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 e0b3dace2020..4b7ad6684bc4 100644 --- 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,
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> --- 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(-)