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>

5 files changed, 59 insertions, 58 deletions

diff --git a/isisd/isis_adjacency.c b/isisd/isis_adjacency.c
index 9beed206e..4e0ee4448 100644
--- 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 93583fc12..8f3d63c29 100644
--- 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 68be9c1a9..2ff5979d1 100644
--- 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 915351262..e8a0ba02e 100644
--- 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 2bb028f5f..298629e24 100644
--- 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");
 			}
 		}