Hi,
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.
> Hi,
>
> 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 */
>
