On Thu, Oct 21, 2021 at 03:04:22PM +0300, Vitaliy Makkoveev wrote:
> With this diff the `tdb' dereference after tdb_free() is safe enough and
> I have no objections against your diff.
My diff did not make things worse, it counted the drops earlier.
But I decided to drop that part, I would like just do commit the
error cleanup.
Fixing the tdb destroy is another story. I do not see how the
NET_LOCK(); NET_UNLOCK() is a fix, it looks more like a hack.
ok to commit this error cleanup?
bluhm
Index: netinet/ip_ah.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
retrieving revision 1.157
diff -u -p -r1.157 ip_ah.c
--- netinet/ip_ah.c 21 Oct 2021 22:59:07 -0000 1.157
+++ netinet/ip_ah.c 22 Oct 2021 14:41:53 -0000
@@ -1164,6 +1164,7 @@ ah_output_cb(struct tdb *tdb, struct tdb
{
int skip = tc->tc_skip;
caddr_t ptr = (caddr_t) (tc + 1);
+ int error;
/*
* Copy original headers (with the new protocol number) back
@@ -1175,10 +1176,8 @@ ah_output_cb(struct tdb *tdb, struct tdb
free(tc, M_XDATA, 0);
/* Call the IPsec input callback. */
- if (ipsp_process_done(m, tdb)) {
+ error = ipsp_process_done(m, tdb);
+ if (error)
ahstat_inc(ahs_outfail);
- return -1;
- }
-
- return 0;
+ return error;
}
Index: netinet/ip_esp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_esp.c,v
retrieving revision 1.176
diff -u -p -r1.176 ip_esp.c
--- netinet/ip_esp.c 21 Oct 2021 22:59:07 -0000 1.176
+++ netinet/ip_esp.c 22 Oct 2021 14:42:27 -0000
@@ -1024,16 +1024,16 @@ int
esp_output_cb(struct tdb *tdb, struct tdb_crypto *tc, struct mbuf *m, int ilen,
int olen)
{
+ int error;
+
/* Release crypto descriptors. */
free(tc, M_XDATA, 0);
/* Call the IPsec input callback. */
- if (ipsp_process_done(m, tdb)) {
+ error = ipsp_process_done(m, tdb);
+ if (error)
espstat_inc(esps_outfail);
- return -1;
- }
-
- return 0;
+ return error;
}
#define SEEN_SIZE howmany(TDB_REPLAYMAX, 32)
Index: netinet/ip_ipcomp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipcomp.c,v
retrieving revision 1.78
diff -u -p -r1.78 ip_ipcomp.c
--- netinet/ip_ipcomp.c 22 Oct 2021 12:30:53 -0000 1.78
+++ netinet/ip_ipcomp.c 22 Oct 2021 14:42:41 -0000
@@ -505,6 +505,7 @@ ipcomp_output_cb(struct tdb *tdb, struct
#ifdef ENCDEBUG
char buf[INET6_ADDRSTRLEN];
#endif
+ int error;
skip = tc->tc_skip;
rlen = ilen - skip;
@@ -523,7 +524,8 @@ ipcomp_output_cb(struct tdb *tdb, struct
ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)),
ntohl(tdb->tdb_spi));
ipcompstat_inc(ipcomps_wrap);
- goto baddone;
+ error = ENOBUFS;
+ goto drop;
}
/* Initialize the IPCOMP header */
@@ -552,21 +554,21 @@ ipcomp_output_cb(struct tdb *tdb, struct
ipsp_address(&tdb->tdb_dst, buf, sizeof(buf)),
ntohl(tdb->tdb_spi));
ipcompstat_inc(ipcomps_nopf);
- goto baddone;
+ error = EPFNOSUPPORT;
+ goto drop;
}
skiphdr:
/* Release the crypto descriptor. */
free(tc, M_XDATA, 0);
- if (ipsp_process_done(m, tdb)) {
+ error = ipsp_process_done(m, tdb);
+ if (error)
ipcompstat_inc(ipcomps_outfail);
- return -1;
- }
- return 0;
+ return error;
- baddone:
+ drop:
m_freem(m);
free(tc, M_XDATA, 0);
- return -1;
+ return error;
}
Index: netinet/ip_ipip.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipip.c,v
retrieving revision 1.95
diff -u -p -r1.95 ip_ipip.c
--- netinet/ip_ipip.c 13 Oct 2021 14:36:31 -0000 1.95
+++ netinet/ip_ipip.c 22 Oct 2021 14:45:33 -0000
@@ -524,7 +524,7 @@ ipip_output(struct mbuf **mp, struct tdb
DPRINTF("unsupported protocol family %d",
tdb->tdb_dst.sa.sa_family);
ipipstat_inc(ipips_family);
- error = EAFNOSUPPORT;
+ error = EPFNOSUPPORT;
goto drop;
}
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 22 Oct 2021 14:58:54 -0000
@@ -359,10 +359,10 @@ ipsec_common_input(struct mbuf *m, int s
return error;
drop:
+ m_freem(m);
ipsecstat_inc(ipsec_idrops);
if (tdbp != NULL)
tdbp->tdb_idrops++;
- m_freem(m);
return error;
}
@@ -423,7 +423,6 @@ ipsec_input_cb(struct cryptop *crp)
panic("%s: unknown/unsupported security protocol %d",
__func__, tdb->tdb_sproto);
}
-
if (error) {
ipsecstat_inc(ipsec_idrops);
tdb->tdb_idrops++;
@@ -431,12 +430,12 @@ ipsec_input_cb(struct cryptop *crp)
return;
drop:
+ m_freem(m);
+ free(tc, M_XDATA, 0);
+ crypto_freereq(crp);
ipsecstat_inc(ipsec_idrops);
if (tdb != NULL)
tdb->tdb_idrops++;
- free(tc, M_XDATA, 0);
- m_freem(m);
- crypto_freereq(crp);
}
/*
@@ -448,19 +447,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 +472,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 +484,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 +497,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 +510,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 +518,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 +530,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 +541,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 +564,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 +582,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 +611,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 +690,24 @@ 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);
+ 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 22 Oct 2021 15:02:38 -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,10 +434,9 @@ 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) {
ipsecstat_inc(ipsec_odrops);
tdb->tdb_odrops++;
@@ -445,12 +444,12 @@ ipsec_output_cb(struct cryptop *crp)
return;
drop:
- if (tdb != NULL)
- tdb->tdb_odrops++;
m_freem(m);
free(tc, M_XDATA, 0);
crypto_freereq(crp);
ipsecstat_inc(ipsec_odrops);
+ if (tdb != NULL)
+ tdb->tdb_odrops++;
}
/*
@@ -494,7 +493,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 +547,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,18 +595,22 @@ 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. */
+ return error;
drop:
m_freem(m);
