On Thu, Oct 21, 2021 at 12:08:57PM +0200, Alexander Bluhm wrote:
> On Sat, Oct 16, 2021 at 12:50:46AM +0300, Vitaliy Makkoveev wrote:
> > This time tdb_delete() doesn't kill passed `tdb' but schedules timeout
> > to do this. So this `tdb' is logically killed and should be not used,
> > but because the killer timeout is also serialized with netlock we don;t
> > catch use-after-free issue.
> >
> > This was the reason I reverted my "per-cpu counters" diff for `tdb'
> > which has exposed this.
> >
> > Unfortunately, your diff introduces access to `tdb' after it was killed
> > by tdb_delete(). I marked such places in your diff.
>
> This is not worse than before. Currently the use after free is
> after calling ipsp_process_packet(). I just pushed the problem
> down the callstack. And it is not exposed due to exclusive netlock.
>
> My goal is to port genua's tdb refcounting in small steps. It will
> free the reference basically where I do the counting with this diff.
>
> The refcouting has to be moved down the callstack due to the queueing
> in the crypto taks. I think in the long term the seperate crypto
> task should be avoided, but this is not my focus right now.
>
> As this diff is part on my way to solve the tdb lifetime, I would
> like to commit it.
>
> ok?
>
> Or do you see any new problems?
>
> bluhm
>
tdb_free() which "kills" passed `tdb' actually sets the new tdb_reaper()
handler to `tdb_timer_tmo' timeout(9) and schedules it to run.
tdb_reaper() thread is not serialized with netlock holding threads, so
hypothetically such use-after-free movement could change the probability
and this issue will be exposed.
844 tdb_free(struct tdb *tdbp)
845 {
...
891 timeout_del(&tdbp->tdb_stimer_tmo);
892 timeout_del(&tdbp->tdb_sfirst_tmo);
893
894 timeout_set_proc(&tdbp->tdb_timer_tmo, tdb_reaper, tdbp);
895 timeout_add(&tdbp->tdb_timer_tmo, 0);
896 }
899 tdb_reaper(void *xtdbp)
900 {
901 struct tdb *tdbp = xtdbp;
902
903 pool_put(&tdb_pool, tdbp);
904 }
But since tdb_free() unlinks passed `tdb' from the stack, it enough to
take netlock around or before pool_put(9) within tdb_reaper(). I like to
have this diff commited before your diff. It imlements some kind of
barrier to enforce tdb_reaper() thread wait before netlock holding
ipsec(4) thread finished.
With this diff the `tdb' dereference after tdb_free() is safe enough and
I have no objections against your diff.
Index: sys/netinet/ip_ipsp.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.246
diff -u -p -r1.246 ip_ipsp.c
--- sys/netinet/ip_ipsp.c 13 Oct 2021 14:36:31 -0000 1.246
+++ sys/netinet/ip_ipsp.c 21 Oct 2021 11:57:18 -0000
@@ -900,6 +900,13 @@ tdb_reaper(void *xtdbp)
{
struct tdb *tdbp = xtdbp;
+ /*
+ * XXXSMP: we should wait until tdb_free() called thread
+ * finished
+ */
+ NET_LOCK();
+ NET_UNLOCK();
+
pool_put(&tdb_pool, tdbp);
}
> > > Goal is to ref count the tdb in ipsec. For that the counters that
> > > access the tdb have to be pushed down the function hierarchie.
> > > While there adjust the error handling. Output functions should
> > > generate an errno.
> > >
> > > ok?
> > >
> > > bluhm
> > >
> > > Index: netinet/ip_ah.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
> > > [...]
> > >
> > > @@ -1152,6 +1153,7 @@ ah_output(struct mbuf *m, struct tdb *td
> > > m_freem(m);
> > > crypto_freereq(crp);
> > > free(tc, M_XDATA, 0);
> > > + tdb->tdb_odrops++;
> > `tdb' was killed by tdb_delete().
> >
> > > return error;
> > > }
> > >
> > > [...]
> > >
> > > Index: netinet/ip_esp.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_esp.c,v
> > > [...]
> > >
> > > @@ -1019,6 +1020,7 @@ esp_output(struct mbuf *m, struct tdb *t
> > > m_freem(m);
> > > crypto_freereq(crp);
> > > free(tc, M_XDATA, 0);
> > > + tdb->tdb_odrops++;
> > `tdb' was killed by tdb_delete().
> >
> > > return error;
> > > }
> > >
> > > [...]
> > >
> > > Index: netinet/ip_ipcomp.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipcomp.c,v
> > > [...]
> > >
> > > @@ -488,6 +489,7 @@ ipcomp_output(struct mbuf *m, struct tdb
> > > drop:
> > > m_freem(m);
> > > crypto_freereq(crp);
> > > + tdb->tdb_odrops++;
> > `tdb' was killed by tdb_delete().
> >
> > > return error;
> > > }
> > >
> > > [...]
> > >
> > > Index: netinet/ipsec_input.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v
> > > retrieving revision 1.184
> > > diff -u -p -r1.184 ipsec_input.c
> > > --- netinet/ipsec_input.c 13 Oct 2021 22:49:11 -0000 1.184
> > > +++ netinet/ipsec_input.c 15 Oct 2021 14:25:53 -0000
> > > @@ -352,17 +352,15 @@ ipsec_common_input(struct mbuf *m, int s
> > > * everything else.
> > > */
> > > error = (*(tdbp->tdb_xform->xf_input))(m, tdbp, skip, protoff);
> > > - if (error) {
> > > + if (error)
> > > ipsecstat_inc(ipsec_idrops);
> > > - tdbp->tdb_idrops++;
> > > - }
> > > return error;
> > >
> > > drop:
> > > - ipsecstat_inc(ipsec_idrops);
> > > + m_freem(m);
> > > if (tdbp != NULL)
> > > tdbp->tdb_idrops++;
> > tdbp->tdb_xform->xf_input() could kill passed `tdbp' by
> > tdb_delete()
> >
> > > - m_freem(m);
> > > + ipsecstat_inc(ipsec_idrops);
> > > return error;
> > > }
> > >
> > > @@ -423,20 +421,17 @@ ipsec_input_cb(struct cryptop *crp)
> > > panic("%s: unknown/unsupported security protocol %d",
> > > __func__, tdb->tdb_sproto);
> > > }
> > > -
> > > - if (error) {
> > > + if (error)
> > > ipsecstat_inc(ipsec_idrops);
> > > - tdb->tdb_idrops++;
> > > - }
> > > return;
> > >
> > > drop:
> > > - ipsecstat_inc(ipsec_idrops);
> > > - if (tdb != NULL)
> > > - tdb->tdb_idrops++;
> > > - free(tc, M_XDATA, 0);
> > > m_freem(m);
> > > + free(tc, M_XDATA, 0);
> > > crypto_freereq(crp);
> > > + if (tdb != NULL)
> > > + tdb->tdb_idrops++;
> > ipcomp_input_cb() could kill passed `tdb' by tdb_delete()
> >
> > > + ipsecstat_inc(ipsec_idrops);
> > > }
> > >
> > > /*
> > > @@ -448,19 +443,15 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > {
> > > int af, sproto;
> > > u_int8_t prot;
> > > -
> > > #if NBPFILTER > 0
> > > struct ifnet *encif;
> > > #endif
> > > -
> > > struct ip *ip, ipn;
> > > -
> > > #ifdef INET6
> > > struct ip6_hdr *ip6, ip6n;
> > > #endif /* INET6 */
> > > struct m_tag *mtag;
> > > struct tdb_ident *tdbi;
> > > -
> > > #ifdef ENCDEBUG
> > > char buf[INET6_ADDRSTRLEN];
> > > #endif
> > > @@ -477,7 +468,7 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > ipsp_address(&tdbp->tdb_dst, buf, sizeof(buf)),
> > > ntohl(tdbp->tdb_spi));
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops, ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > >
> > > ip = mtod(m, struct ip *);
> > > @@ -489,10 +480,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > /* IP-in-IP encapsulation */
> > > if (prot == IPPROTO_IPIP) {
> > > if (m->m_pkthdr.len - skip < sizeof(struct ip)) {
> > > - m_freem(m);
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> > > ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > /* ipn will now contain the inner IPv4 header */
> > > m_copydata(m, skip, sizeof(struct ip),
> > > @@ -503,10 +493,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > /* IPv6-in-IP encapsulation. */
> > > if (prot == IPPROTO_IPV6) {
> > > if (m->m_pkthdr.len - skip < sizeof(struct ip6_hdr)) {
> > > - m_freem(m);
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> > > ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > /* ip6n will now contain the inner IPv6 header. */
> > > m_copydata(m, skip, sizeof(struct ip6_hdr),
> > > @@ -517,8 +506,7 @@ ipsec_common_input_cb(struct mbuf *m, st
> > >
> > > #ifdef INET6
> > > /* Fix IPv6 header */
> > > - if (af == AF_INET6)
> > > - {
> > > + if (af == AF_INET6) {
> > > if (m->m_len < sizeof(struct ip6_hdr) &&
> > > (m = m_pullup(m, sizeof(struct ip6_hdr))) == NULL) {
> > >
> > > @@ -526,7 +514,7 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > ipsp_address(&tdbp->tdb_dst, buf, sizeof(buf)),
> > > ntohl(tdbp->tdb_spi));
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops, ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > >
> > > ip6 = mtod(m, struct ip6_hdr *);
> > > @@ -538,10 +526,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > /* IP-in-IP encapsulation */
> > > if (prot == IPPROTO_IPIP) {
> > > if (m->m_pkthdr.len - skip < sizeof(struct ip)) {
> > > - m_freem(m);
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> > > ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > /* ipn will now contain the inner IPv4 header */
> > > m_copydata(m, skip, sizeof(struct ip), (caddr_t) &ipn);
> > > @@ -550,10 +537,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > /* IPv6-in-IP encapsulation */
> > > if (prot == IPPROTO_IPV6) {
> > > if (m->m_pkthdr.len - skip < sizeof(struct ip6_hdr)) {
> > > - m_freem(m);
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> > > ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > /* ip6n will now contain the inner IPv6 header. */
> > > m_copydata(m, skip, sizeof(struct ip6_hdr),
> > > @@ -574,10 +560,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > switch (prot) {
> > > case IPPROTO_UDP:
> > > if (m->m_pkthdr.len < skip + sizeof(struct udphdr)) {
> > > - m_freem(m);
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> > > ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > cksum = 0;
> > > m_copyback(m, skip + offsetof(struct udphdr, uh_sum),
> > > @@ -593,10 +578,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > break;
> > > case IPPROTO_TCP:
> > > if (m->m_pkthdr.len < skip + sizeof(struct tcphdr)) {
> > > - m_freem(m);
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops,
> > > ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > cksum = 0;
> > > m_copyback(m, skip + offsetof(struct tcphdr, th_sum),
> > > @@ -623,10 +607,9 @@ ipsec_common_input_cb(struct mbuf *m, st
> > > mtag = m_tag_get(PACKET_TAG_IPSEC_IN_DONE,
> > > sizeof(struct tdb_ident), M_NOWAIT);
> > > if (mtag == NULL) {
> > > - m_freem(m);
> > > DPRINTF("failed to get tag");
> > > IPSEC_ISTAT(esps_hdrops, ahs_hdrops, ipcomps_hdrops);
> > > - return -1;
> > > + goto baddone;
> > > }
> > >
> > > tdbi = (struct tdb_ident *)(mtag + 1);
> > > @@ -703,22 +686,25 @@ ipsec_common_input_cb(struct mbuf *m, st
> > >
> > > /* This is the enc0 interface unless for ipcomp. */
> > > if ((ifp = if_get(m->m_pkthdr.ph_ifidx)) == NULL) {
> > > - m_freem(m);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > if (pf_test(af, PF_IN, ifp, &m) != PF_PASS) {
> > > if_put(ifp);
> > > - m_freem(m);
> > > - return -1;
> > > + goto baddone;
> > > }
> > > if_put(ifp);
> > > if (m == NULL)
> > > - return -1;
> > > + return 0;
> > > }
> > > #endif
> > > /* Call the appropriate IPsec transform callback. */
> > > ip_deliver(&m, &skip, prot, af);
> > > return 0;
> > > +
> > > + baddone:
> > > + m_freem(m);
> > > + tdbp->tdb_idrops++;
> > > + return -1;
> > > #undef IPSEC_ISTAT
> > > }
> > >
> > > Index: netinet/ipsec_output.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_output.c,v
> > > retrieving revision 1.89
> > > diff -u -p -r1.89 ipsec_output.c
> > > --- netinet/ipsec_output.c 13 Oct 2021 22:43:44 -0000 1.89
> > > +++ netinet/ipsec_output.c 15 Oct 2021 14:11:23 -0000
> > > @@ -130,7 +130,7 @@ ipsp_process_packet(struct mbuf *m, stru
> > > ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)),
> > > ntohl(tdb->tdb_spi), tdb->tdb_sproto,
> > > tdb->tdb_dst.sa.sa_family);
> > > - error = ENXIO;
> > > + error = EPFNOSUPPORT;
> > > goto drop;
> > > }
> > >
> > > @@ -348,7 +348,7 @@ ipsp_process_packet(struct mbuf *m, stru
> > > break;
> > > #endif /* INET6 */
> > > default:
> > > - error = EINVAL;
> > > + error = EPFNOSUPPORT;
> > > goto drop;
> > > }
> > >
> > > @@ -434,22 +434,19 @@ ipsec_output_cb(struct cryptop *crp)
> > > error = ipcomp_output_cb(tdb, tc, m, ilen, olen);
> > > break;
> > > default:
> > > - panic("%s: unknown/unsupported security protocol %d",
> > > + panic("%s: unhandled security protocol %d",
> > > __func__, tdb->tdb_sproto);
> > > }
> > > -
> > > - if (error) {
> > > + if (error)
> > > ipsecstat_inc(ipsec_odrops);
> > > - tdb->tdb_odrops++;
> > > - }
> > > return;
> > >
> > > drop:
> > > - if (tdb != NULL)
> > > - tdb->tdb_odrops++;
> > > m_freem(m);
> > > free(tc, M_XDATA, 0);
> > > crypto_freereq(crp);
> > > + if (tdb != NULL)
> > > + tdb->tdb_odrops++;
> > > ipsecstat_inc(ipsec_odrops);
> > > }
> > >
> > > @@ -494,7 +491,7 @@ ipsp_process_done(struct mbuf *m, struct
> > > default:
> > > DPRINTF("unknown protocol family (%d)",
> > > tdb->tdb_dst.sa.sa_family);
> > > - error = ENXIO;
> > > + error = EPFNOSUPPORT;
> > > goto drop;
> > > }
> > >
> > > @@ -548,7 +545,7 @@ ipsp_process_done(struct mbuf *m, struct
> > > default:
> > > DPRINTF("unknown protocol family (%d)",
> > > tdb->tdb_dst.sa.sa_family);
> > > - error = ENXIO;
> > > + error = EPFNOSUPPORT;
> > > goto drop;
> > > }
> > >
> > > @@ -596,21 +593,28 @@ ipsp_process_done(struct mbuf *m, struct
> > > */
> > > switch (tdb->tdb_dst.sa.sa_family) {
> > > case AF_INET:
> > > - return (ip_output(m, NULL, NULL, IP_RAWOUTPUT, NULL, NULL, 0));
> > > -
> > > + error = ip_output(m, NULL, NULL, IP_RAWOUTPUT, NULL, NULL, 0);
> > > + break;
> > > #ifdef INET6
> > > case AF_INET6:
> > > /*
> > > * We don't need massage, IPv6 header fields are always in
> > > * net endian.
> > > */
> > > - return (ip6_output(m, NULL, NULL, 0, NULL, NULL));
> > > + error = ip6_output(m, NULL, NULL, 0, NULL, NULL);
> > > + break;
> > > #endif /* INET6 */
> > > + default:
> > > + error = EPFNOSUPPORT;
> > > + break;
> > > }
> > > - error = EINVAL; /* Not reached. */
> > > + if (error)
> > > + tdb->tdb_odrops++;
> > > + return error;
> > >
> > > drop:
> > > m_freem(m);
> > > + tdb->tdb_odrops++;
> > > return error;
> > > }
> > >
> > > Index: netinet6/ip6_output.c
> > > ===================================================================
> > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_output.c,v
> > > retrieving revision 1.260
> > > diff -u -p -r1.260 ip6_output.c
> > > --- netinet6/ip6_output.c 27 Jul 2021 17:13:03 -0000 1.260
> > > +++ netinet6/ip6_output.c 15 Oct 2021 14:11:23 -0000
> > > @@ -2870,10 +2870,8 @@ ip6_output_ipsec_send(struct tdb *tdb, s
> > >
> > > /* Callee frees mbuf */
> > > error = ipsp_process_packet(m, tdb, AF_INET6, tunalready);
> > > - if (error) {
> > > + if (error)
> > > ipsecstat_inc(ipsec_odrops);
> > > - tdb->tdb_odrops++;
> > > - }
> > > return error;
> > > }
> > > #endif /* IPSEC */
> > >
