isisd: Prevent use after free for isis_adj_state_change

When we call isis_adj_state_change with ISIS_ADJ_DOWN
we free the pointer, but we were still using the pointer
after it was freed.  Cleanup the api to prevent this.

Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>

diff --git a/isisd/isis_adjacency.c b/isisd/isis_adjacency.c
index 9beed206e8050c03c33551bf9898c1b77facfe53..4e0ee4448be14a033f2ba0038f52465838a832e3 100644 (file)
--- a/isisd/isis_adjacency.c
+++ b/isisd/isis_adjacency.c
@@ -201,13 +201,14 @@ void isis_adj_process_threeway(struct isis_adjacency *adj,
                fabricd_initial_sync_hello(adj->circuit);
 
        if (next_tw_state == ISIS_THREEWAY_DOWN) {
-               isis_adj_state_change(adj, ISIS_ADJ_DOWN, "Neighbor restarted");
+               isis_adj_state_change(&adj, ISIS_ADJ_DOWN,
+                                     "Neighbor restarted");
                return;
        }
 
        if (next_tw_state == ISIS_THREEWAY_UP) {
                if (adj->adj_state != ISIS_ADJ_UP) {
-                       isis_adj_state_change(adj, ISIS_ADJ_UP, NULL);
+                       isis_adj_state_change(&adj, ISIS_ADJ_UP, NULL);
                        adj->adj_usage = adj_usage;
                }
        }
@@ -219,12 +220,13 @@ void isis_adj_process_threeway(struct isis_adjacency *adj,
        adj->threeway_state = next_tw_state;
 }
 
-void isis_adj_state_change(struct isis_adjacency *adj,
+void isis_adj_state_change(struct isis_adjacency **padj,
                           enum isis_adj_state new_state, const char *reason)
 {
+       struct isis_adjacency *adj = *padj;
        enum isis_adj_state old_state = adj->adj_state;
        struct isis_circuit *circuit = adj->circuit;
-       bool del;
+       bool del = false;
 
        adj->adj_state = new_state;
        if (new_state != old_state) {
@@ -262,7 +264,6 @@ void isis_adj_state_change(struct isis_adjacency *adj,
 #endif /* ifndef FABRICD */
 
        if (circuit->circ_type == CIRCUIT_T_BROADCAST) {
-               del = false;
                for (int level = IS_LEVEL_1; level <= IS_LEVEL_2; level++) {
                        if ((adj->level & level) == 0)
                                continue;
@@ -299,11 +300,7 @@ void isis_adj_state_change(struct isis_adjacency *adj,
                                lsp_regenerate_schedule_pseudo(circuit, level);
                }
 
-               if (del)
-                       isis_delete_adj(adj);
-
        } else if (circuit->circ_type == CIRCUIT_T_P2P) {
-               del = false;
                for (int level = IS_LEVEL_1; level <= IS_LEVEL_2; level++) {
                        if ((adj->level & level) == 0)
                                continue;
@@ -336,9 +333,11 @@ void isis_adj_state_change(struct isis_adjacency *adj,
                                del = true;
                        }
                }
+       }
 
-               if (del)
-                       isis_delete_adj(adj);
+       if (del) {
+               isis_delete_adj(adj);
+               *padj = NULL;
        }
 }
 
@@ -402,7 +401,7 @@ int isis_adj_expire(struct thread *thread)
        adj->t_expire = NULL;
 
        /* trigger the adj expire event */
-       isis_adj_state_change(adj, ISIS_ADJ_DOWN, "holding time expired");
+       isis_adj_state_change(&adj, ISIS_ADJ_DOWN, "holding time expired");
 
        return 0;
 }

diff --git a/isisd/isis_adjacency.h b/isisd/isis_adjacency.h
index 93583fc1224372a31518af7c42911a1fc3f121ec..8f3d63c297b663f8abdacd40ab829dcd67e7f7f8 100644 (file)
--- a/isisd/isis_adjacency.h
+++ b/isisd/isis_adjacency.h
@@ -118,7 +118,7 @@ void isis_adj_process_threeway(struct isis_adjacency *adj,
                               struct isis_threeway_adj *tw_adj,
                               enum isis_adj_usage adj_usage);
 DECLARE_HOOK(isis_adj_state_change_hook, (struct isis_adjacency *adj), (adj))
-void isis_adj_state_change(struct isis_adjacency *adj,
+void isis_adj_state_change(struct isis_adjacency **adj,
                           enum isis_adj_state state, const char *reason);
 void isis_adj_print(struct isis_adjacency *adj);
 const char *isis_adj_yang_state(enum isis_adj_state state);

diff --git a/isisd/isis_bfd.c b/isisd/isis_bfd.c
index 68be9c1a995660d1222145f96367485a932cd8ec..2ff5979d141bc625e0bbe2a2a26d6676cc95fae8 100644 (file)
--- a/isisd/isis_bfd.c
+++ b/isisd/isis_bfd.c
@@ -138,7 +138,7 @@ static void bfd_adj_event(struct isis_adjacency *adj, struct prefix *dst,
                return;
        }
 
-       isis_adj_state_change(adj, ISIS_ADJ_DOWN, "bfd session went down");
+       isis_adj_state_change(&adj, ISIS_ADJ_DOWN, "bfd session went down");
 }
 
 static int isis_bfd_interface_dest_update(ZAPI_CALLBACK_ARGS)

diff --git a/isisd/isis_pdu.c b/isisd/isis_pdu.c
index 9153512623c69a6f0b05366fc9210b55dc145552..e8a0ba02e906ea16be23b5d3fb49b0facd4c6830 100644 (file)
--- a/isisd/isis_pdu.c
+++ b/isisd/isis_pdu.c
@@ -164,7 +164,7 @@ static int process_p2p_hello(struct iih_info *iih)
                if (memcmp(iih->sys_id, adj->sysid, ISIS_SYS_ID_LEN)) {
                        zlog_debug(
                                "hello source and adjacency do not match, set adj down\n");
-                       isis_adj_state_change(adj, ISIS_ADJ_DOWN,
+                       isis_adj_state_change(&adj, ISIS_ADJ_DOWN,
                                              "adj do not exist");
                        return ISIS_OK;
                }
@@ -184,7 +184,7 @@ static int process_p2p_hello(struct iih_info *iih)
                 * adjacency entry getting added to the lsp tlv neighbor list.
                 */
                adj->circuit_t = iih->circ_type;
-               isis_adj_state_change(adj, ISIS_ADJ_INITIALIZING, NULL);
+               isis_adj_state_change(&adj, ISIS_ADJ_INITIALIZING, NULL);
                adj->sys_type = ISIS_SYSTYPE_UNKNOWN;
        }
 
@@ -233,7 +233,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                        return ISIS_WARNING;
                                } else if (adj->adj_usage == ISIS_ADJ_LEVEL1) {
                                        /* (6) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -254,7 +254,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                           || (adj->adj_usage
                                               == ISIS_ADJ_LEVEL2)) {
                                        /* (8) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -268,7 +268,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                           || (adj->adj_usage
                                               == ISIS_ADJ_LEVEL1AND2)) {
                                        /* (8) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -282,7 +282,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                           || (adj->adj_usage
                                               == ISIS_ADJ_LEVEL2)) {
                                        /* (8) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -304,7 +304,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                           || (adj->adj_usage
                                               == ISIS_ADJ_LEVEL2)) {
                                        /* (6) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -318,7 +318,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                } else if (adj->adj_usage
                                           == ISIS_ADJ_LEVEL1AND2) {
                                        /* (6) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -331,11 +331,11 @@ static int process_p2p_hello(struct iih_info *iih)
                if (iih->circuit->area->is_type == IS_LEVEL_1) {
                        /* 8.2.5.2 b) 1) is_type L1 and adj is not up */
                        if (adj->adj_state != ISIS_ADJ_UP) {
-                               isis_adj_state_change(adj, ISIS_ADJ_DOWN,
+                               isis_adj_state_change(&adj, ISIS_ADJ_DOWN,
                                                      "Area Mismatch");
                                /* 8.2.5.2 b) 2)is_type L1 and adj is up */
                        } else {
-                               isis_adj_state_change(adj, ISIS_ADJ_DOWN,
+                               isis_adj_state_change(&adj, ISIS_ADJ_DOWN,
                                                      "Down - Area Mismatch");
                        }
                }
@@ -349,7 +349,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                        return ISIS_WARNING;
                                } else if (adj->adj_usage == ISIS_ADJ_LEVEL1) {
                                        /* (7) down - area mismatch */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Area Mismatch");
 
@@ -358,7 +358,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                           || (adj->adj_usage
                                               == ISIS_ADJ_LEVEL2)) {
                                        /* (7) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                }
@@ -371,7 +371,7 @@ static int process_p2p_hello(struct iih_info *iih)
                                                                  ISIS_ADJ_LEVEL2);
                                } else if (adj->adj_usage == ISIS_ADJ_LEVEL1) {
                                        /* (7) down - wrong system */
-                                       isis_adj_state_change(adj,
+                                       isis_adj_state_change(&adj,
                                                              ISIS_ADJ_DOWN,
                                                              "Wrong System");
                                } else if (adj->adj_usage
@@ -379,12 +379,12 @@ static int process_p2p_hello(struct iih_info *iih)
                                        if (iih->circ_type == IS_LEVEL_2) {
                                                /* (7) down - wrong system */
                                                isis_adj_state_change(
-                                                       adj, ISIS_ADJ_DOWN,
+                                                       &adj, ISIS_ADJ_DOWN,
                                                        "Wrong System");
                                        } else {
                                                /* (7) down - area mismatch */
                                                isis_adj_state_change(
-                                                       adj, ISIS_ADJ_DOWN,
+                                                       &adj, ISIS_ADJ_DOWN,
                                                        "Area Mismatch");
                                        }
                                }
@@ -393,34 +393,36 @@ static int process_p2p_hello(struct iih_info *iih)
                }
        } else {
                /* down - area mismatch */
-               isis_adj_state_change(adj, ISIS_ADJ_DOWN, "Area Mismatch");
+               isis_adj_state_change(&adj, ISIS_ADJ_DOWN, "Area Mismatch");
        }
 
-       if (adj->adj_state == ISIS_ADJ_UP && changed) {
-               lsp_regenerate_schedule(adj->circuit->area,
-                                       isis_adj_usage2levels(adj->adj_usage),
-                                       0);
-       }
+       if (adj) {
+               if (adj->adj_state == ISIS_ADJ_UP && changed) {
+                       lsp_regenerate_schedule(
+                               adj->circuit->area,
+                               isis_adj_usage2levels(adj->adj_usage), 0);
+               }
 
-       /* 8.2.5.2 c) if the action was up - comparing circuit IDs */
-       /* FIXME - Missing parts */
+               /* 8.2.5.2 c) if the action was up - comparing circuit IDs */
+               /* FIXME - Missing parts */
 
-       /* some of my own understanding of the ISO, why the heck does
-        * it not say what should I change the system_type to...
-        */
-       switch (adj->adj_usage) {
-       case ISIS_ADJ_LEVEL1:
-               adj->sys_type = ISIS_SYSTYPE_L1_IS;
-               break;
-       case ISIS_ADJ_LEVEL2:
-               adj->sys_type = ISIS_SYSTYPE_L2_IS;
-               break;
-       case ISIS_ADJ_LEVEL1AND2:
-               adj->sys_type = ISIS_SYSTYPE_L2_IS;
-               break;
-       case ISIS_ADJ_NONE:
-               adj->sys_type = ISIS_SYSTYPE_UNKNOWN;
-               break;
+               /* some of my own understanding of the ISO, why the heck does
+                * it not say what should I change the system_type to...
+                */
+               switch (adj->adj_usage) {
+               case ISIS_ADJ_LEVEL1:
+                       adj->sys_type = ISIS_SYSTYPE_L1_IS;
+                       break;
+               case ISIS_ADJ_LEVEL2:
+                       adj->sys_type = ISIS_SYSTYPE_L2_IS;
+                       break;
+               case ISIS_ADJ_LEVEL1AND2:
+                       adj->sys_type = ISIS_SYSTYPE_L2_IS;
+                       break;
+               case ISIS_ADJ_NONE:
+                       adj->sys_type = ISIS_SYSTYPE_UNKNOWN;
+                       break;
+               }
        }
 
        if (isis->debugs & DEBUG_ADJ_PACKETS) {
@@ -455,7 +457,7 @@ static int process_lan_hello(struct iih_info *iih)
                        }
                        adj->level = iih->level;
                }
-               isis_adj_state_change(adj, ISIS_ADJ_INITIALIZING, NULL);
+               isis_adj_state_change(&adj, ISIS_ADJ_INITIALIZING, NULL);
 
                if (iih->level == IS_LEVEL_1)
                        adj->sys_type = ISIS_SYSTYPE_L1_IS;
@@ -506,13 +508,13 @@ static int process_lan_hello(struct iih_info *iih)
        if (adj->adj_state != ISIS_ADJ_UP) {
                if (own_snpa_found) {
                        isis_adj_state_change(
-                               adj, ISIS_ADJ_UP,
+                               &adj, ISIS_ADJ_UP,
                                "own SNPA found in LAN Neighbours TLV");
                }
        } else {
                if (!own_snpa_found) {
                        isis_adj_state_change(
-                               adj, ISIS_ADJ_INITIALIZING,
+                               &adj, ISIS_ADJ_INITIALIZING,
                                "own SNPA not found in LAN Neighbours TLV");
                }
        }

diff --git a/isisd/isisd.c b/isisd/isisd.c
index 2bb028f5ff5bff6e83059bf03c9a5d5dc975daa0..298629e246b331daab0985071bffa617f878b7aa 100644 (file)
--- a/isisd/isisd.c
+++ b/isisd/isisd.c
@@ -653,7 +653,7 @@ int clear_isis_neighbor_common(struct vty *vty, const char *id)
                                                                       sysid,
                                                                       ISIS_SYS_ID_LEN))
                                                                isis_adj_state_change(
-                                                                       adj,
+                                                                       &adj,
                                                                        ISIS_ADJ_DOWN,
                                                                        "clear user request");
                                        }
@@ -665,7 +665,7 @@ int clear_isis_neighbor_common(struct vty *vty, const char *id)
                                    || !memcmp(adj->sysid, sysid,
                                               ISIS_SYS_ID_LEN))
                                        isis_adj_state_change(
-                                               adj, ISIS_ADJ_DOWN,
+                                               &adj, ISIS_ADJ_DOWN,
                                                "clear user request");
                        }
                }