vrrpd: search all vr's for mvl_ifp to null

Previous patch was not sufficient to make sure the pointers get nulled.
Reason being is that vrrp_lookup_by_if_mvl() follows the link_ifindex on
the provided interface to acquire the base ifp, which is then used to
lookup the VR. However, because FRR's interface management is straight
up insane, it's possible that we delete the base interface before its
children. When this happens, link_ifindex is still valid for the macvlan
device but we have no interface corresponding to that ifindex, so our
lookup will fail. Consequently vrrp_lookup_by_if_mvl() can't be used if
we are handling deletion of any sort. Instead we have to loop through
every VR and check the pointers.

Also, there's no null check on the mvl_ifp pointer in vrrp_shutdown(),
and even though we log that we're returning early from it, we actually
don't. Do both of these things.

Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>

1 files changed, 44 insertions, 32 deletions

diff --git a/vrrpd/vrrp.c b/vrrpd/vrrp.c
index 431ebf662..3ef9fd90a 100644
--- a/vrrpd/vrrp.c
+++ b/vrrpd/vrrp.c
@@ -185,6 +185,11 @@ static bool vrrp_ifp_has_vrrp_mac(struct interface *ifp)
  * is used to look up any existing instances that match the interface. It does
  * not matter whether the instance is already bound to the interface or not.
  *
+ * Note that the interface linkages must be correct for this to work. In other
+ * words, the macvlan must have a valid VRRP MAC, and its link_ifindex must be
+ * be equal to the ifindex of another interface in the interface RB trees (its
+ * parent). If these conditions aren't satisfied we won't find the VR.
+ *
  * mvl_ifp
  *    Interface pointer to use to lookup. Should be a macvlan device.
  *
@@ -1646,7 +1651,7 @@ static int vrrp_shutdown(struct vrrp_router *r)
 		       r->vr->vrid, family2str(r->family),
 		       vrrp_event_names[VRRP_EVENT_SHUTDOWN],
 		       vrrp_state_names[VRRP_STATE_INITIALIZE]);
-		break;
+		return 0;
 	}
 
 	/* Cancel all timers */
@@ -2214,52 +2219,59 @@ void vrrp_if_del(struct interface *ifp)
 {
 	struct listnode *ln;
 	struct vrrp_vrouter *vr;
-	struct list *vrs = vrrp_lookup_by_if_any(ifp);
 
 	vrrp_if_down(ifp);
 
+	/*
+	 * You think we'd be able use vrrp_lookup_by_if_any to find interfaces?
+	 * Nah. FRR's interface management is insane. There are no ordering
+	 * guarantees about what interfaces are deleted when. Maybe this is a
+	 * macvlan and its parent was already deleted, in which case its
+	 * ifindex is now IFINDEX_INTERNAL, so ifp->link_ifindex - while still
+	 * valid - doesn't match any interface on the system, meaning we can't
+	 * use any of the vrrp_lookup* functions since they rely on finding the
+	 * base interface of what they're given by following link_ifindex.
+	 *
+	 * Since we need to actually NULL out pointers in this function to
+	 * avoid a UAF - since the caller will (might) free ifp after we return
+	 * - we need to look up based on pointers.
+	 */
+	struct list *vrs = hash_to_list(vrrp_vrouters_hash);
+
 	for (ALL_LIST_ELEMENTS_RO(vrs, ln, vr)) {
-		if (vr->v4->mvl_ifp == ifp || vr->ifp == ifp) {
+		if (ifp == vr->ifp) {
+			vrrp_event(vr->v4, VRRP_EVENT_SHUTDOWN);
+			vrrp_event(vr->v6, VRRP_EVENT_SHUTDOWN);
 			/*
-			 * If either of our interfaces goes away, we need to
-			 * down the session
+			 * Stands to reason if the base was deleted, so were
+			 * (or will be) its children
 			 */
-			if (vr->v4->fsm.state != VRRP_STATE_INITIALIZE)
-				vrrp_event(vr->v4, VRRP_EVENT_SHUTDOWN);
-
+			vr->v4->mvl_ifp = NULL;
+			vr->v6->mvl_ifp = NULL;
 			/*
-			 * If it was the macvlan, then it wasn't explicitly
-			 * configured and will be deleted when we return from
-			 * this function, so we need to lose the reference
+			 * We shouldn't need to lose the reference if it's the
+			 * primary interface, because that was configured
+			 * explicitly in our config, and thus will be kept as a
+			 * stub; to avoid stupid bugs, double check that
 			 */
-			if (ifp == vr->v4->mvl_ifp)
-				vr->v4->mvl_ifp = NULL;
-
-		} else if (vr->v6->mvl_ifp == ifp || vr->ifp == ifp) {
+			assert(ifp->configured);
+		} else if (ifp == vr->v4->mvl_ifp) {
+			vrrp_event(vr->v4, VRRP_EVENT_SHUTDOWN);
 			/*
-			 * If either of our interfaces goes away, we need to
-			 * down the session
+			 * If this is a macvlan, then it wasn't explicitly
+			 * configured and will be deleted when we return from
+			 * this function, so we need to lose the reference
 			 */
-			if (vr->v6->fsm.state != VRRP_STATE_INITIALIZE)
-				vrrp_event(vr->v6, VRRP_EVENT_SHUTDOWN);
-
+			vr->v4->mvl_ifp = NULL;
+		} else if (ifp == vr->v6->mvl_ifp) {
+			vrrp_event(vr->v6, VRRP_EVENT_SHUTDOWN);
 			/*
-			 * If it was the macvlan, then it wasn't explicitly
+			 * If this is a macvlan, then it wasn't explicitly
 			 * configured and will be deleted when we return from
 			 * this function, so we need to lose the reference
 			 */
-			if (ifp == vr->v6->mvl_ifp)
-				vr->v6->mvl_ifp = NULL;
+			vr->v6->mvl_ifp = NULL;
 		}
-
-		/*
-		 * We shouldn't need to lose the reference if it's the primary
-		 * interface, because that was configured explicitly in our
-		 * config, and thus will be kept as a stub; to avoid stupid
-		 * bugs, double check that
-		 */
-		if (ifp == vr->ifp)
-			assert(ifp->configured);
 	}
 
 	list_delete(&vrs);