diff options
| -rw-r--r-- | vrrpd/vrrp.c | 76 |
1 files changed, 44 insertions, 32 deletions
diff --git a/vrrpd/vrrp.c b/vrrpd/vrrp.c index 431ebf6620..3ef9fd90aa 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); |