Use struct refcnt for interface address reference counting.

There was a crash due to use after free of the ifa although it is
ref counted.  As ifa_refcnt was a simple integer increment, there
may be a path where multiple CPUs access it concurrently.  So change
to struct refcnt which is MP safe and provides dt(4) leak debugging.
Link level address for IPsec enc(4) and various MPLS interfaces is
special.  There ifa is part of struct sc.  Use refcount anyway and
add a panic to detect use after free.
bug report stsp@; OK mvs@

diff --git a/sys/dev/dt/dt_prov_static.c b/sys/dev/dt/dt_prov_static.c
index d608373..611269f 100644 (file)
--- a/sys/dev/dt/dt_prov_static.c
+++ b/sys/dev/dt/dt_prov_static.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: dt_prov_static.c,v 1.14 2022/06/28 09:32:27 bluhm Exp $ */
+/*     $OpenBSD: dt_prov_static.c,v 1.15 2022/08/29 07:51:45 bluhm Exp $ */
 
 /*
  * Copyright (c) 2019 Martin Pieuchot <mpi@openbsd.org>
@@ -88,9 +88,10 @@ DT_STATIC_PROBE0(smr, wakeup);
 DT_STATIC_PROBE2(smr, thread, "uint64_t", "uint64_t");
 
 /*
- * reference counting
+ * reference counting, keep in sync with sys/refcnt.h
  */
 DT_STATIC_PROBE0(refcnt, none);
+DT_STATIC_PROBE3(refcnt, ifaddr, "void *", "int", "int");
 DT_STATIC_PROBE3(refcnt, inpcb, "void *", "int", "int");
 DT_STATIC_PROBE3(refcnt, tdb, "void *", "int", "int");
 
@@ -135,6 +136,7 @@ struct dt_probe *const dtps_static[] = {
        &_DT_STATIC_P(smr, thread),
        /* refcnt */
        &_DT_STATIC_P(refcnt, none),
+       &_DT_STATIC_P(refcnt, ifaddr),
        &_DT_STATIC_P(refcnt, inpcb),
        &_DT_STATIC_P(refcnt, tdb),
 };

diff --git a/sys/net/if_enc.c b/sys/net/if_enc.c
index 4645f3a..e835a8b 100644 (file)
--- a/sys/net/if_enc.c
+++ b/sys/net/if_enc.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_enc.c,v 1.78 2020/12/28 14:28:50 kn Exp $  */
+/*     $OpenBSD: if_enc.c,v 1.79 2022/08/29 07:51:45 bluhm Exp $       */
 
 /*
  * Copyright (c) 2010 Reyk Floeter <reyk@vantronix.net>
@@ -100,6 +100,7 @@ enc_clone_create(struct if_clone *ifc, int unit)
         * and empty ifa of type AF_LINK for this purpose.
         */
        if_alloc_sadl(ifp);
+       refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR);
        sc->sc_ifa.ifa_ifp = ifp;
        sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl);
        sc->sc_ifa.ifa_netmask = NULL;
@@ -152,6 +153,10 @@ enc_clone_destroy(struct ifnet *ifp)
        NET_UNLOCK();
 
        if_detach(ifp);
+       if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) {
+               panic("%s: ifa refcnt has %u refs", __func__,
+                   sc->sc_ifa.ifa_refcnt.r_refs);
+       }
        free(sc, M_DEVBUF, sizeof(*sc));
 
        return (0);

diff --git a/sys/net/if_mpe.c b/sys/net/if_mpe.c
index 8232b2f..7b84d24 100644 (file)
--- a/sys/net/if_mpe.c
+++ b/sys/net/if_mpe.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: if_mpe.c,v 1.101 2021/11/08 04:50:54 dlg Exp $ */
+/* $OpenBSD: if_mpe.c,v 1.102 2022/08/29 07:51:45 bluhm Exp $ */
 
 /*
  * Copyright (c) 2008 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -128,6 +128,7 @@ mpe_clone_create(struct if_clone *ifc, int unit)
        sc->sc_txhprio = 0;
        sc->sc_rxhprio = IF_HDRPRIO_PACKET;
        sc->sc_rdomain = 0;
+       refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR);
        sc->sc_ifa.ifa_ifp = ifp;
        sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl);
        sc->sc_smpls.smpls_len = sizeof(sc->sc_smpls);
@@ -154,6 +155,10 @@ mpe_clone_destroy(struct ifnet *ifp)
        ifq_barrier(&ifp->if_snd);
 
        if_detach(ifp);
+       if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) {
+               panic("%s: ifa refcnt has %u refs", __func__,
+                   sc->sc_ifa.ifa_refcnt.r_refs);
+       }
        free(sc, M_DEVBUF, sizeof *sc);
        return (0);
 }

diff --git a/sys/net/if_mpip.c b/sys/net/if_mpip.c
index a8daeee..4fefa75 100644 (file)
--- a/sys/net/if_mpip.c
+++ b/sys/net/if_mpip.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_mpip.c,v 1.15 2021/03/26 19:00:21 kn Exp $ */
+/*     $OpenBSD: if_mpip.c,v 1.16 2022/08/29 07:51:45 bluhm Exp $ */
 
 /*
  * Copyright (c) 2015 Rafael Zalamena <rzalamena@openbsd.org>
@@ -128,6 +128,7 @@ mpip_clone_create(struct if_clone *ifc, int unit)
        bpfattach(&ifp->if_bpf, ifp, DLT_LOOP, sizeof(uint32_t));
 #endif
 
+       refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR);
        sc->sc_ifa.ifa_ifp = ifp;
        sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl);
 
@@ -152,7 +153,10 @@ mpip_clone_destroy(struct ifnet *ifp)
        ifq_barrier(&ifp->if_snd);
 
        if_detach(ifp);
-
+       if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) {
+               panic("%s: ifa refcnt has %u refs", __func__,
+                   sc->sc_ifa.ifa_refcnt.r_refs);
+       }
        free(sc->sc_neighbor, M_DEVBUF, sizeof(*sc->sc_neighbor));
        free(sc, M_DEVBUF, sizeof(*sc));
 

diff --git a/sys/net/if_mpw.c b/sys/net/if_mpw.c
index f17693d..2106c3e 100644 (file)
--- a/sys/net/if_mpw.c
+++ b/sys/net/if_mpw.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_mpw.c,v 1.62 2021/03/26 19:00:21 kn Exp $ */
+/*     $OpenBSD: if_mpw.c,v 1.63 2022/08/29 07:51:45 bluhm Exp $ */
 
 /*
  * Copyright (c) 2015 Rafael Zalamena <rzalamena@openbsd.org>
@@ -122,6 +122,7 @@ mpw_clone_create(struct if_clone *ifc, int unit)
        sc->sc_txhprio = 0;
        sc->sc_rxhprio = IF_HDRPRIO_PACKET;
        sc->sc_rdomain = 0;
+       refcnt_init_trace(&sc->sc_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR);
        sc->sc_ifa.ifa_ifp = ifp;
        sc->sc_ifa.ifa_addr = sdltosa(ifp->if_sadl);
        sc->sc_smpls.smpls_len = sizeof(sc->sc_smpls);
@@ -149,7 +150,10 @@ mpw_clone_destroy(struct ifnet *ifp)
 
        ether_ifdetach(ifp);
        if_detach(ifp);
-
+       if (refcnt_rele(&sc->sc_ifa.ifa_refcnt) == 0) {
+               panic("%s: ifa refcnt has %u refs", __func__,
+                   sc->sc_ifa.ifa_refcnt.r_refs);
+       }
        free(sc->sc_neighbor, M_DEVBUF, sizeof(*sc->sc_neighbor));
        free(sc, M_DEVBUF, sizeof(*sc));
 

diff --git a/sys/net/if_pppx.c b/sys/net/if_pppx.c
index fd099d8..953524b 100644 (file)
--- a/sys/net/if_pppx.c
+++ b/sys/net/if_pppx.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_pppx.c,v 1.121 2022/07/25 08:29:26 mvs Exp $ */
+/*     $OpenBSD: if_pppx.c,v 1.122 2022/08/29 07:51:45 bluhm Exp $ */
 
 /*
  * Copyright (c) 2010 Claudio Jeker <claudio@openbsd.org>
@@ -659,6 +659,7 @@ pppx_add_session(struct pppx_dev *pxd, struct pipex_session_req *req)
        ifaddr.sin_addr = req->pr_ip_srcaddr;
 
        ia = malloc(sizeof (*ia), M_IFADDR, M_WAITOK | M_ZERO);
+       refcnt_init_trace(&ia->ia_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR);
 
        ia->ia_addr.sin_family = AF_INET;
        ia->ia_addr.sin_len = sizeof(struct sockaddr_in);

diff --git a/sys/net/if_var.h b/sys/net/if_var.h
index 400afd3..394398c 100644 (file)
--- a/sys/net/if_var.h
+++ b/sys/net/if_var.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_var.h,v 1.114 2021/02/20 04:55:52 dlg Exp $        */
+/*     $OpenBSD: if_var.h,v 1.115 2022/08/29 07:51:45 bluhm Exp $      */
 /*     $NetBSD: if.h,v 1.23 1996/05/07 02:40:27 thorpej Exp $  */
 
 /*
@@ -242,7 +242,7 @@ struct ifaddr {
        struct  ifnet *ifa_ifp;         /* back-pointer to interface */
        TAILQ_ENTRY(ifaddr) ifa_list;   /* list of addresses for interface */
        u_int   ifa_flags;              /* interface flags, see below */
-       u_int   ifa_refcnt;             /* number of `rt_ifa` references */
+       struct  refcnt ifa_refcnt;      /* number of `rt_ifa` references */
        int     ifa_metric;             /* cost of going out this interface */
 };
 
@@ -333,6 +333,7 @@ int p2p_bpf_mtap(caddr_t, const struct mbuf *, u_int);
 struct ifaddr *ifa_ifwithaddr(struct sockaddr *, u_int);
 struct ifaddr *ifa_ifwithdstaddr(struct sockaddr *, u_int);
 struct ifaddr *ifaof_ifpforaddr(struct sockaddr *, struct ifnet *);
+struct ifaddr *ifaref(struct ifaddr *);
 void   ifafree(struct ifaddr *);
 
 int    if_isconnected(const struct ifnet *, unsigned int);

diff --git a/sys/net/route.c b/sys/net/route.c
index 7d1aeec..6f5cb08 100644 (file)
--- a/sys/net/route.c
+++ b/sys/net/route.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: route.c,v 1.413 2022/07/28 22:19:09 bluhm Exp $       */
+/*     $OpenBSD: route.c,v 1.414 2022/08/29 07:51:45 bluhm Exp $       */
 /*     $NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $      */
 
 /*
@@ -146,7 +146,6 @@ extern unsigned int rtmap_limit;
 
 struct cpumem *                rtcounters;
 int                    rttrash;        /* routes not in table but not freed */
-int                    ifatrash;       /* ifas not in ifp list but not free */
 
 struct pool    rtentry_pool;           /* pool for rtentry structures */
 struct pool    rttimer_pool;           /* pool for rttimer structures */
@@ -512,16 +511,19 @@ rtfree(struct rtentry *rt)
        pool_put(&rtentry_pool, rt);
 }
 
+struct ifaddr *
+ifaref(struct ifaddr *ifa)
+{
+       refcnt_take(&ifa->ifa_refcnt);
+       return ifa;
+}
+
 void
 ifafree(struct ifaddr *ifa)
 {
-       if (ifa == NULL)
-               panic("ifafree");
-       if (ifa->ifa_refcnt == 0) {
-               ifatrash--;
-               free(ifa, M_IFADDR, 0);
-       } else
-               ifa->ifa_refcnt--;
+       if (refcnt_rele(&ifa->ifa_refcnt) == 0)
+               return;
+       free(ifa, M_IFADDR, 0);
 }
 
 /*
@@ -901,8 +903,7 @@ rtrequest(int req, struct rt_addrinfo *info, u_int8_t prio,
                        rt_mpls_clear(rt);
 #endif
 
-               ifa->ifa_refcnt++;
-               rt->rt_ifa = ifa;
+               rt->rt_ifa = ifaref(ifa);
                rt->rt_ifidx = ifp->if_index;
                /*
                 * Copy metrics and a back pointer from the cloned
@@ -1857,8 +1858,8 @@ db_print_ifa(struct ifaddr *ifa)
        db_print_sa(ifa->ifa_dstaddr);
        db_printf("  ifa_mask=");
        db_print_sa(ifa->ifa_netmask);
-       db_printf("  flags=0x%x, refcnt=%d, metric=%d\n",
-           ifa->ifa_flags, ifa->ifa_refcnt, ifa->ifa_metric);
+       db_printf("  flags=0x%x, refcnt=%u, metric=%d\n",
+           ifa->ifa_flags, ifa->ifa_refcnt.r_refs, ifa->ifa_metric);
 }
 
 /*

diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
index 5e54c1c..c7476a5 100644 (file)
--- a/sys/net/rtsock.c
+++ b/sys/net/rtsock.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rtsock.c,v 1.346 2022/08/28 21:35:12 mvs Exp $        */
+/*     $OpenBSD: rtsock.c,v 1.347 2022/08/29 07:51:45 bluhm Exp $      */
 /*     $NetBSD: rtsock.c,v 1.18 1996/03/29 00:32:10 cgd Exp $  */
 
 /*
@@ -1145,8 +1145,7 @@ rtm_output(struct rt_msghdr *rtm, struct rtentry **prt,
                                ifp->if_rtrequest(ifp, RTM_DELETE, rt);
                                ifafree(rt->rt_ifa);
 
-                               ifa->ifa_refcnt++;
-                               rt->rt_ifa = ifa;
+                               rt->rt_ifa = ifaref(ifa);
                                rt->rt_ifidx = ifa->ifa_ifp->if_index;
                                /* recheck link state after ifp change */
                                rt_if_linkstate_change(rt, ifa->ifa_ifp,

diff --git a/sys/netinet/in.c b/sys/netinet/in.c
index 2ff41a5..70bfd7e 100644 (file)
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: in.c,v 1.175 2022/08/06 15:57:59 bluhm Exp $  */
+/*     $OpenBSD: in.c,v 1.176 2022/08/29 07:51:45 bluhm Exp $  */
 /*     $NetBSD: in.c,v 1.26 1996/02/13 23:41:39 christos Exp $ */
 
 /*
@@ -379,6 +379,7 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
        }
        if (ia == NULL) {
                ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
+               refcnt_init_trace(&ia->ia_ifa.ifa_refcnt, DT_REFCNT_IDX_IFADDR);
                ia->ia_addr.sin_family = AF_INET;
                ia->ia_addr.sin_len = sizeof(ia->ia_addr);
                ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr);
@@ -475,6 +476,8 @@ in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
 
                if (ia == NULL) {
                        ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
+                       refcnt_init_trace(&ia->ia_ifa.ifa_refcnt,
+                           DT_REFCNT_IDX_IFADDR);
                        ia->ia_addr.sin_family = AF_INET;
                        ia->ia_addr.sin_len = sizeof(ia->ia_addr);
                        ia->ia_ifa.ifa_addr = sintosa(&ia->ia_addr);
@@ -745,7 +748,6 @@ in_purgeaddr(struct ifaddr *ifa)
 {
        struct ifnet *ifp = ifa->ifa_ifp;
        struct in_ifaddr *ia = ifatoia(ifa);
-       extern int ifatrash;
 
        NET_ASSERT_LOCKED();
 
@@ -760,7 +762,6 @@ in_purgeaddr(struct ifaddr *ifa)
                ia->ia_allhosts = NULL;
        }
 
-       ifatrash++;
        ia->ia_ifp = NULL;
        ifafree(&ia->ia_ifa);
 }

diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c
index 20ff921..65ad168 100644 (file)
--- a/sys/netinet6/in6.c
+++ b/sys/netinet6/in6.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: in6.c,v 1.247 2022/08/06 15:57:59 bluhm Exp $ */
+/*     $OpenBSD: in6.c,v 1.248 2022/08/29 07:51:45 bluhm Exp $ */
 /*     $KAME: in6.c,v 1.372 2004/06/14 08:14:21 itojun Exp $   */
 
 /*
@@ -647,6 +647,8 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra,
        if (ia6 == NULL) {
                hostIsNew = 1;
                ia6 = malloc(sizeof(*ia6), M_IFADDR, M_WAITOK | M_ZERO);
+               refcnt_init_trace(&ia6->ia_ifa.ifa_refcnt,
+                   DT_REFCNT_IDX_IFADDR);
                LIST_INIT(&ia6->ia6_memberships);
                /* Initialize the address and masks, and put time stamp */
                ia6->ia_ifa.ifa_addr = sin6tosa(&ia6->ia_addr);
@@ -938,7 +940,6 @@ void
 in6_unlink_ifa(struct in6_ifaddr *ia6, struct ifnet *ifp)
 {
        struct ifaddr *ifa = &ia6->ia_ifa;
-       extern int ifatrash;
        int plen;
 
        NET_ASSERT_LOCKED();
@@ -953,7 +954,6 @@ in6_unlink_ifa(struct in6_ifaddr *ia6, struct ifnet *ifp)
        rt_ifa_purge(ifa);
        ifa_del(ifp, ifa);
 
-       ifatrash++;
        ia6->ia_ifp = NULL;
        ifafree(&ia6->ia_ifa);
 }

diff --git a/sys/netinet6/nd6_nbr.c b/sys/netinet6/nd6_nbr.c
index 57433b4..3701ae4 100644 (file)
--- a/sys/netinet6/nd6_nbr.c
+++ b/sys/netinet6/nd6_nbr.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: nd6_nbr.c,v 1.132 2022/08/08 17:47:59 kn Exp $        */
+/*     $OpenBSD: nd6_nbr.c,v 1.133 2022/08/29 07:51:45 bluhm Exp $     */
 /*     $KAME: nd6_nbr.c,v 1.61 2001/02/10 16:06:14 jinmei Exp $        */
 
 /*
@@ -1124,8 +1124,7 @@ nd6_dad_start(struct ifaddr *ifa)
         * first packet to be sent from the interface after interface
         * (re)initialization.
         */
-       dp->dad_ifa = ifa;
-       ifa->ifa_refcnt++;      /* just for safety */
+       dp->dad_ifa = ifaref(ifa);
        dp->dad_count = ip6_dad_count;
        dp->dad_ns_icount = dp->dad_na_icount = 0;
        dp->dad_ns_ocount = dp->dad_ns_tcount = 0;

diff --git a/sys/sys/refcnt.h b/sys/sys/refcnt.h
index bb6e968..c6a44d6 100644 (file)
--- a/sys/sys/refcnt.h
+++ b/sys/sys/refcnt.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: refcnt.h,v 1.7 2022/06/28 09:32:28 bluhm Exp $ */
+/*     $OpenBSD: refcnt.h,v 1.8 2022/08/29 07:51:45 bluhm Exp $ */
 
 /*
  * Copyright (c) 2015 David Gwynne <dlg@openbsd.org>
@@ -43,8 +43,10 @@ void refcnt_finalize(struct refcnt *, const char *);
 int    refcnt_shared(struct refcnt *);
 unsigned int   refcnt_read(struct refcnt *);
 
-#define DT_REFCNT_IDX_INPCB    1
-#define DT_REFCNT_IDX_TDB      2
+/* sorted alphabetically, keep in sync with dev/dt/dt_prov_static.c */
+#define DT_REFCNT_IDX_IFADDR   1
+#define DT_REFCNT_IDX_INPCB    2
+#define DT_REFCNT_IDX_TDB      3
 
 #endif /* _KERNEL */