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
> > 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 */
> >
