From: Donald Sharp <sharpd@cumulusnetworks.com>
Date: Sat, 18 Apr 2020 01:18:53 +0000 (-0400)
Subject: ospf6d: Prevent use after free
X-Git-Tag: base_7.4~66^2
X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=744ba5696921114b552a05cd848a4f7da5b449b6;p=matthieu%2Ffrr.git

ospf6d: Prevent use after free

ospf6_lsa_unlock may free the lsa data structure as such
we cannot use the passed in data structure after freeing it.

Provide a mechanism to know if the data has been freed
using the same usage patterns of other _unlock functions
in FRR.

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

diff --git a/ospf6d/ospf6_flood.c b/ospf6d/ospf6_flood.c
index 85d02c186b..b144c6804e 100644
--- a/ospf6d/ospf6_flood.c
+++ b/ospf6d/ospf6_flood.c
@@ -332,11 +332,12 @@ void ospf6_flood_interface(struct ospf6_neighbor *from, struct ospf6_lsa *lsa,
 					if (req == on->last_ls_req) {
 						/* sanity check refcount */
 						assert(req->lock >= 2);
-						ospf6_lsa_unlock(req);
+						req = ospf6_lsa_unlock(req);
 						on->last_ls_req = NULL;
 					}
-					ospf6_lsdb_remove(req,
-							  on->request_list);
+					if (req)
+						ospf6_lsdb_remove(
+							req, on->request_list);
 					ospf6_check_nbr_loading(on);
 					continue;
 				}
@@ -348,7 +349,7 @@ void ospf6_flood_interface(struct ospf6_neighbor *from, struct ospf6_lsa *lsa,
 						zlog_debug(
 							"Received is newer, remove requesting");
 					if (req == on->last_ls_req) {
-						ospf6_lsa_unlock(req);
+						req = ospf6_lsa_unlock(req);
 						on->last_ls_req = NULL;
 					}
 					if (req)
diff --git a/ospf6d/ospf6_lsa.c b/ospf6d/ospf6_lsa.c
index bcfd975879..aa32fae6ad 100644
--- a/ospf6d/ospf6_lsa.c
+++ b/ospf6d/ospf6_lsa.c
@@ -608,16 +608,17 @@ void ospf6_lsa_lock(struct ospf6_lsa *lsa)
 }
 
 /* decrement reference counter of struct ospf6_lsa */
-void ospf6_lsa_unlock(struct ospf6_lsa *lsa)
+struct ospf6_lsa *ospf6_lsa_unlock(struct ospf6_lsa *lsa)
 {
 	/* decrement reference counter */
 	assert(lsa->lock > 0);
 	lsa->lock--;
 
 	if (lsa->lock != 0)
-		return;
+		return lsa;
 
 	ospf6_lsa_delete(lsa);
+	return NULL;
 }
 
 
diff --git a/ospf6d/ospf6_lsa.h b/ospf6d/ospf6_lsa.h
index 02f9f9d26c..5519dd1b80 100644
--- a/ospf6d/ospf6_lsa.h
+++ b/ospf6d/ospf6_lsa.h
@@ -227,7 +227,7 @@ extern void ospf6_lsa_delete(struct ospf6_lsa *lsa);
 extern struct ospf6_lsa *ospf6_lsa_copy(struct ospf6_lsa *);
 
 extern void ospf6_lsa_lock(struct ospf6_lsa *);
-extern void ospf6_lsa_unlock(struct ospf6_lsa *);
+extern struct ospf6_lsa *ospf6_lsa_unlock(struct ospf6_lsa *);
 
 extern int ospf6_lsa_expire(struct thread *);
 extern int ospf6_lsa_refresh(struct thread *);
diff --git a/ospf6d/ospf6_lsdb.c b/ospf6d/ospf6_lsdb.c
index 0a9f1c6f7c..18fcec82c1 100644
--- a/ospf6d/ospf6_lsdb.c
+++ b/ospf6d/ospf6_lsdb.c
@@ -136,7 +136,7 @@ void ospf6_lsdb_add(struct ospf6_lsa *lsa, struct ospf6_lsdb *lsdb)
 		}
 		/* to free the lookup lock in node get*/
 		route_unlock_node(current);
-		ospf6_lsa_unlock(old);
+		old = ospf6_lsa_unlock(old);
 	}
 
 	ospf6_lsdb_count_assert(lsdb);
@@ -164,7 +164,7 @@ void ospf6_lsdb_remove(struct ospf6_lsa *lsa, struct ospf6_lsdb *lsdb)
 
 	route_unlock_node(node); /* to free the lookup lock */
 	route_unlock_node(node); /* to free the original lock */
-	ospf6_lsa_unlock(lsa);
+	lsa = ospf6_lsa_unlock(lsa);
 
 	ospf6_lsdb_count_assert(lsdb);
 }
@@ -279,7 +279,7 @@ struct ospf6_lsa *ospf6_lsdb_next(const struct route_node *iterend,
 {
 	struct route_node *node = lsa->rn;
 
-	ospf6_lsa_unlock(lsa);
+	lsa = ospf6_lsa_unlock(lsa);
 
 	do
 		node = route_next_until(node, iterend);
@@ -316,7 +316,7 @@ void ospf6_lsdb_lsa_unlock(struct ospf6_lsa *lsa)
 	if (lsa != NULL) {
 		if (lsa->rn != NULL)
 			route_unlock_node(lsa->rn);
-		ospf6_lsa_unlock(lsa);
+		lsa = ospf6_lsa_unlock(lsa);
 	}
 }
 
diff --git a/ospf6d/ospf6_message.c b/ospf6d/ospf6_message.c
index 21f9b0722c..31862a2298 100644
--- a/ospf6d/ospf6_message.c
+++ b/ospf6d/ospf6_message.c
@@ -1948,9 +1948,9 @@ int ospf6_lsreq_send(struct thread *thread)
 	}
 
 	if (last_req != NULL) {
-		if (on->last_ls_req != NULL) {
-			ospf6_lsa_unlock(on->last_ls_req);
-		}
+		if (on->last_ls_req != NULL)
+			on->last_ls_req = ospf6_lsa_unlock(on->last_ls_req);
+
 		ospf6_lsa_lock(last_req);
 		on->last_ls_req = last_req;
 	}