Hi,
I found a m_pullup() down in ah input. As it may free or change
the mbuf, the caller must be careful. All callers do not use the
mbuf, so we are safe. Nevertheless I would like to use a common
pattern to handle this. Pass down an mbuf pointer mp and let
m_pullup() update the pointer in all callers.
It looks like the tcp_signature functions should not be called.
Avoid an mbuf leak and return an error.
ok?
bluhm
Index: net/if_bridge.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.356
diff -u -p -r1.356 if_bridge.c
--- net/if_bridge.c 7 Jul 2021 20:19:01 -0000 1.356
+++ net/if_bridge.c 22 Oct 2021 16:20:53 -0000
@@ -1575,7 +1575,7 @@ bridge_ipsec(struct ifnet *ifp, struct e
tdb->tdb_soft_first_use);
}
- (*(tdb->tdb_xform->xf_input))(m, tdb, hlen, off);
+ (*(tdb->tdb_xform->xf_input))(&m, tdb, hlen, off);
return (1);
} else {
skiplookup:
Index: netinet/ip_ah.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ah.c,v
retrieving revision 1.158
diff -u -p -r1.158 ip_ah.c
--- netinet/ip_ah.c 22 Oct 2021 15:44:20 -0000 1.158
+++ netinet/ip_ah.c 22 Oct 2021 18:16:36 -0000
@@ -197,9 +197,9 @@ ah_zeroize(struct tdb *tdbp)
* Massage IPv4/IPv6 headers for AH processing.
*/
int
-ah_massage_headers(struct mbuf **m0, int af, int skip, int alg, int out)
+ah_massage_headers(struct mbuf **mp, int af, int skip, int alg, int out)
{
- struct mbuf *m = *m0;
+ struct mbuf *m = *mp;
unsigned char *ptr;
int off, count;
struct ip *ip;
@@ -216,11 +216,12 @@ ah_massage_headers(struct mbuf **m0, int
* and option processing -- just make sure they're in
* contiguous memory.
*/
- *m0 = m = m_pullup(m, skip);
+ m = *mp = m_pullup(m, skip);
if (m == NULL) {
DPRINTF("m_pullup() failed");
ahstat_inc(ahs_hdrops);
- return ENOBUFS;
+ error = ENOBUFS;
+ goto drop;
}
/* Fix the IP header */
@@ -240,8 +241,8 @@ ah_massage_headers(struct mbuf **m0, int
"for option %d",
ptr[off]);
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EINVAL;
+ error = EINVAL;
+ goto drop;
}
switch (ptr[off]) {
@@ -264,8 +265,8 @@ ah_massage_headers(struct mbuf **m0, int
"for option %d",
ptr[off]);
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EINVAL;
+ error = EINVAL;
+ goto drop;
}
off += ptr[off + 1];
@@ -279,8 +280,8 @@ ah_massage_headers(struct mbuf **m0, int
"for option %d",
ptr[off]);
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EINVAL;
+ error = EINVAL;
+ goto drop;
}
/*
@@ -307,8 +308,8 @@ ah_massage_headers(struct mbuf **m0, int
"for option %d",
ptr[off]);
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EINVAL;
+ error = EINVAL;
+ goto drop;
}
/* Zeroize all other options. */
@@ -322,8 +323,8 @@ ah_massage_headers(struct mbuf **m0, int
if (off > skip) {
DPRINTF("malformed IPv4 options header");
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EINVAL;
+ error = EINVAL;
+ goto drop;
}
}
@@ -338,8 +339,8 @@ ah_massage_headers(struct mbuf **m0, int
if (ip6.ip6_plen == 0) {
DPRINTF("unsupported IPv6 jumbogram");
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EMSGSIZE;
+ error = EMSGSIZE;
+ goto drop;
}
ip6.ip6_flow = 0;
@@ -359,8 +360,7 @@ ah_massage_headers(struct mbuf **m0, int
if (error) {
DPRINTF("m_copyback no memory");
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return error;
+ goto drop;
}
/* Let's deal with the remaining headers (if any). */
@@ -372,8 +372,8 @@ ah_massage_headers(struct mbuf **m0, int
DPRINTF("failed to allocate "
"memory for IPv6 headers");
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return ENOBUFS;
+ error = ENOBUFS;
+ goto drop;
}
/*
@@ -478,8 +478,7 @@ ah_massage_headers(struct mbuf **m0, int
if (alloc)
free(ptr, M_XDATA, 0);
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return error;
+ goto drop;
}
rh0->ip6r0_segleft = 0;
}
@@ -492,8 +491,8 @@ error6:
if (alloc)
free(ptr, M_XDATA, 0);
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return EINVAL;
+ error = EINVAL;
+ goto drop;
}
/* Advance. */
@@ -508,8 +507,7 @@ error6:
free(ptr, M_XDATA, 0);
if (error) {
ahstat_inc(ahs_hdrops);
- m_freem(m);
- return error;
+ goto drop;
}
}
@@ -518,6 +516,10 @@ error6:
}
return 0;
+
+ drop:
+ m_freemp(mp);
+ return error;
}
/*
@@ -525,9 +527,10 @@ error6:
* passes authentication.
*/
int
-ah_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
+ah_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
{
const struct auth_hash *ahx = tdb->tdb_authalgxform;
+ struct mbuf *m = *mp;
struct tdb_crypto *tc = NULL;
u_int32_t btsx, esn;
u_int8_t hl;
@@ -674,13 +677,12 @@ ah_input(struct mbuf *m, struct tdb *tdb
m_copyback(m, skip + rplen, ahx->authsize, ipseczeroes, M_NOWAIT);
/* "Massage" the packet headers for crypto processing. */
- error = ah_massage_headers(&m, tdb->tdb_dst.sa.sa_family, skip,
+ error = ah_massage_headers(mp, tdb->tdb_dst.sa.sa_family, skip,
ahx->type, 0);
- if (error) {
- /* mbuf was freed by callee. */
- m = NULL;
+ /* callee may change or free mbuf */
+ m = *mp;
+ if (error)
goto drop;
- }
/* Crypto operation descriptor. */
crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */
@@ -699,11 +701,14 @@ ah_input(struct mbuf *m, struct tdb *tdb
memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union));
tc->tc_rpl = tdb->tdb_rpl;
+ /* Now the crypto layer owns the mbuf */
+ *mp = NULL;
+
crypto_dispatch(crp);
return 0;
drop:
- m_freem(m);
+ m_freemp(mp);
crypto_freereq(crp);
free(tc, M_XDATA, 0);
return error;
Index: netinet/ip_esp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_esp.c,v
retrieving revision 1.177
diff -u -p -r1.177 ip_esp.c
--- netinet/ip_esp.c 22 Oct 2021 15:44:20 -0000 1.177
+++ netinet/ip_esp.c 22 Oct 2021 18:16:55 -0000
@@ -340,10 +340,11 @@ esp_zeroize(struct tdb *tdbp)
* ESP input processing, called (eventually) through the protocol switch.
*/
int
-esp_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
+esp_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
{
const struct auth_hash *esph = tdb->tdb_authalgxform;
const struct enc_xform *espx = tdb->tdb_encalgxform;
+ struct mbuf *m = *mp;
struct cryptodesc *crde = NULL, *crda = NULL;
struct cryptop *crp = NULL;
struct tdb_crypto *tc = NULL;
@@ -526,11 +527,14 @@ esp_input(struct mbuf *m, struct tdb *td
crde->crd_len = m->m_pkthdr.len - (skip + hlen + alen);
}
+ /* Now the crypto layer owns the mbuf */
+ *mp = NULL;
+
crypto_dispatch(crp);
return 0;
drop:
- m_freem(m);
+ m_freemp(mp);
crypto_freereq(crp);
free(tc, M_XDATA, 0);
return error;
Index: netinet/ip_ipcomp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipcomp.c,v
retrieving revision 1.79
diff -u -p -r1.79 ip_ipcomp.c
--- netinet/ip_ipcomp.c 22 Oct 2021 15:44:20 -0000 1.79
+++ netinet/ip_ipcomp.c 22 Oct 2021 18:16:18 -0000
@@ -131,11 +131,12 @@ ipcomp_zeroize(struct tdb *tdbp)
* ipcomp_input() gets called to uncompress an input packet
*/
int
-ipcomp_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff)
+ipcomp_input(struct mbuf **mp, struct tdb *tdb, int skip, int protoff)
{
const struct comp_algo *ipcompx = tdb->tdb_compalgxform;
+ struct mbuf *m = *mp;
struct tdb_crypto *tc;
- int hlen;
+ int error, hlen;
struct cryptodesc *crdc = NULL;
struct cryptop *crp;
@@ -145,19 +146,18 @@ ipcomp_input(struct mbuf *m, struct tdb
/* Get crypto descriptors */
crp = crypto_getreq(1);
if (crp == NULL) {
- m_freem(m);
DPRINTF("failed to acquire crypto descriptors");
ipcompstat_inc(ipcomps_crypto);
- return ENOBUFS;
+ error = ENOBUFS;
+ goto drop;
}
/* Get IPsec-specific opaque pointer */
tc = malloc(sizeof(*tc), M_XDATA, M_NOWAIT | M_ZERO);
if (tc == NULL) {
- m_freem(m);
- crypto_freereq(crp);
DPRINTF("failed to allocate tdb_crypto");
ipcompstat_inc(ipcomps_crypto);
- return ENOBUFS;
+ error = ENOBUFS;
+ goto drop;
}
crdc = &crp->crp_desc[0];
@@ -184,8 +184,16 @@ ipcomp_input(struct mbuf *m, struct tdb
tc->tc_rdomain = tdb->tdb_rdomain;
tc->tc_dst = tdb->tdb_dst;
+ /* Now the crypto layer owns the mbuf */
+ *mp = NULL;
+
crypto_dispatch(crp);
return 0;
+
+ drop:
+ m_freemp(mp);
+ crypto_freereq(crp);
+ return error;
}
int
Index: netinet/ip_ipip.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipip.c,v
retrieving revision 1.96
diff -u -p -r1.96 ip_ipip.c
--- netinet/ip_ipip.c 22 Oct 2021 15:44:20 -0000 1.96
+++ netinet/ip_ipip.c 22 Oct 2021 16:42:19 -0000
@@ -557,11 +557,11 @@ ipe4_zeroize(struct tdb *tdbp)
}
int
-ipe4_input(struct mbuf *m, struct tdb *tdb, int hlen, int proto)
+ipe4_input(struct mbuf **mp, struct tdb *tdb, int hlen, int proto)
{
/* This is a rather serious mistake, so no conditional printing. */
printf("%s: should never be called\n", __func__);
- m_freem(m);
+ m_freemp(mp);
return EINVAL;
}
#endif /* IPSEC */
Index: netinet/ip_ipsp.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.210
diff -u -p -r1.210 ip_ipsp.h
--- netinet/ip_ipsp.h 13 Oct 2021 14:36:31 -0000 1.210
+++ netinet/ip_ipsp.h 22 Oct 2021 16:23:24 -0000
@@ -479,7 +479,7 @@ struct xformsw {
int (*xf_init)(struct tdb *, const struct xformsw *,
struct ipsecinit *);
int (*xf_zeroize)(struct tdb *); /* termination */
- int (*xf_input)(struct mbuf *, struct tdb *, int, int);
+ int (*xf_input)(struct mbuf **, struct tdb *, int, int);
int (*xf_output)(struct mbuf *, struct tdb *, int, int);
};
@@ -564,13 +564,13 @@ int tdb_walk(u_int, int (*)(struct tdb *
int ipe4_attach(void);
int ipe4_init(struct tdb *, const struct xformsw *, struct ipsecinit *);
int ipe4_zeroize(struct tdb *);
-int ipe4_input(struct mbuf *, struct tdb *, int, int);
+int ipe4_input(struct mbuf **, struct tdb *, int, int);
/* XF_AH */
int ah_attach(void);
int ah_init(struct tdb *, const struct xformsw *, struct ipsecinit *);
int ah_zeroize(struct tdb *);
-int ah_input(struct mbuf *, struct tdb *, int, int);
+int ah_input(struct mbuf **, struct tdb *, int, int);
int ah_input_cb(struct tdb *, struct tdb_crypto *, struct mbuf *, int);
int ah_output(struct mbuf *, struct tdb *, int, int);
int ah_output_cb(struct tdb *, struct tdb_crypto *, struct mbuf *, int,
@@ -589,7 +589,7 @@ int ah6_input(struct mbuf **, int *, int
int esp_attach(void);
int esp_init(struct tdb *, const struct xformsw *, struct ipsecinit *);
int esp_zeroize(struct tdb *);
-int esp_input(struct mbuf *, struct tdb *, int, int);
+int esp_input(struct mbuf **, struct tdb *, int, int);
int esp_input_cb(struct tdb *, struct tdb_crypto *, struct mbuf *, int);
int esp_output(struct mbuf *, struct tdb *, int, int);
int esp_output_cb(struct tdb *, struct tdb_crypto *, struct mbuf *, int,
@@ -607,7 +607,7 @@ int esp6_input(struct mbuf **, int *, in
int ipcomp_attach(void);
int ipcomp_init(struct tdb *, const struct xformsw *, struct ipsecinit *);
int ipcomp_zeroize(struct tdb *);
-int ipcomp_input(struct mbuf *, struct tdb *, int, int);
+int ipcomp_input(struct mbuf **, struct tdb *, int, int);
int ipcomp_input_cb(struct tdb *, struct tdb_crypto *, struct mbuf *, int);
int ipcomp_output(struct mbuf *, struct tdb *, int, int);
int ipcomp_output_cb(struct tdb *, struct tdb_crypto *, struct mbuf *, int,
@@ -623,7 +623,7 @@ int tcp_signature_tdb_attach(void);
int tcp_signature_tdb_init(struct tdb *, const struct xformsw *,
struct ipsecinit *);
int tcp_signature_tdb_zeroize(struct tdb *);
-int tcp_signature_tdb_input(struct mbuf *, struct tdb *, int, int);
+int tcp_signature_tdb_input(struct mbuf **, struct tdb *, int, int);
int tcp_signature_tdb_output(struct mbuf *, struct tdb *, int, int);
/* Replay window */
@@ -647,7 +647,7 @@ void ipsp_ids_free(struct ipsec_ids *);
void ipsp_init(void);
void ipsec_init(void);
int ipsec_sysctl(int *, u_int, void *, size_t *, void *, size_t);
-int ipsec_common_input(struct mbuf *, int, int, int, int, int);
+int ipsec_common_input(struct mbuf **, int, int, int, int, int);
void ipsec_input_cb(struct cryptop *);
void ipsec_output_cb(struct cryptop *);
int ipsec_common_input_cb(struct mbuf *, struct tdb *, int, int);
Index: netinet/ipsec_input.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v
retrieving revision 1.185
diff -u -p -r1.185 ipsec_input.c
--- netinet/ipsec_input.c 22 Oct 2021 15:44:20 -0000 1.185
+++ netinet/ipsec_input.c 22 Oct 2021 16:20:08 -0000
@@ -176,7 +176,7 @@ ipsec_init(void)
* filtering).
*/
int
-ipsec_common_input(struct mbuf *m, int skip, int protoff, int af, int sproto,
+ipsec_common_input(struct mbuf **mp, int skip, int protoff, int af, int sproto,
int udpencap)
{
#define IPSEC_ISTAT(x,y,z) do { \
@@ -188,6 +188,7 @@ ipsec_common_input(struct mbuf *m, int s
ipcompstat_inc(z); \
} while (0)
+ struct mbuf *m = *mp;
union sockaddr_union dst_address;
struct tdb *tdbp = NULL;
struct ifnet *encif;
@@ -351,7 +352,7 @@ ipsec_common_input(struct mbuf *m, int s
* Call appropriate transform and return -- callback takes care of
* everything else.
*/
- error = (*(tdbp->tdb_xform->xf_input))(m, tdbp, skip, protoff);
+ error = (*(tdbp->tdb_xform->xf_input))(mp, tdbp, skip, protoff);
if (error) {
ipsecstat_inc(ipsec_idrops);
tdbp->tdb_idrops++;
@@ -359,7 +360,7 @@ ipsec_common_input(struct mbuf *m, int s
return error;
drop:
- m_freem(m);
+ m_freemp(mp);
ipsecstat_inc(ipsec_idrops);
if (tdbp != NULL)
tdbp->tdb_idrops++;
@@ -873,7 +874,7 @@ ah4_input(struct mbuf **mp, int *offp, i
!ah_enable)
return rip_input(mp, offp, proto, af);
- ipsec_common_input(*mp, *offp, offsetof(struct ip, ip_p), AF_INET,
+ ipsec_common_input(mp, *offp, offsetof(struct ip, ip_p), AF_INET,
proto, 0);
return IPPROTO_DONE;
}
@@ -899,7 +900,7 @@ esp4_input(struct mbuf **mp, int *offp,
!esp_enable)
return rip_input(mp, offp, proto, af);
- ipsec_common_input(*mp, *offp, offsetof(struct ip, ip_p), AF_INET,
+ ipsec_common_input(mp, *offp, offsetof(struct ip, ip_p), AF_INET,
proto, 0);
return IPPROTO_DONE;
}
@@ -915,7 +916,7 @@ ipcomp4_input(struct mbuf **mp, int *off
!ipcomp_enable)
return rip_input(mp, offp, proto, af);
- ipsec_common_input(*mp, *offp, offsetof(struct ip, ip_p), AF_INET,
+ ipsec_common_input(mp, *offp, offsetof(struct ip, ip_p), AF_INET,
proto, 0);
return IPPROTO_DONE;
}
@@ -1092,7 +1093,7 @@ ah6_input(struct mbuf **mp, int *offp, i
}
protoff += offsetof(struct ip6_ext, ip6e_nxt);
}
- ipsec_common_input(*mp, *offp, protoff, AF_INET6, proto, 0);
+ ipsec_common_input(mp, *offp, protoff, AF_INET6, proto, 0);
return IPPROTO_DONE;
}
@@ -1149,7 +1150,7 @@ esp6_input(struct mbuf **mp, int *offp,
}
protoff += offsetof(struct ip6_ext, ip6e_nxt);
}
- ipsec_common_input(*mp, *offp, protoff, AF_INET6, proto, 0);
+ ipsec_common_input(mp, *offp, protoff, AF_INET6, proto, 0);
return IPPROTO_DONE;
}
@@ -1207,7 +1208,7 @@ ipcomp6_input(struct mbuf **mp, int *off
protoff += offsetof(struct ip6_ext, ip6e_nxt);
}
- ipsec_common_input(*mp, *offp, protoff, AF_INET6, proto, 0);
+ ipsec_common_input(mp, *offp, protoff, AF_INET6, proto, 0);
return IPPROTO_DONE;
}
#endif /* INET6 */
Index: netinet/tcp_subr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.180
diff -u -p -r1.180 tcp_subr.c
--- netinet/tcp_subr.c 13 Oct 2021 14:36:31 -0000 1.180
+++ netinet/tcp_subr.c 22 Oct 2021 18:12:48 -0000
@@ -960,15 +960,18 @@ tcp_signature_tdb_zeroize(struct tdb *td
}
int
-tcp_signature_tdb_input(struct mbuf *m, struct tdb *tdbp, int skip, int
protoff)
+tcp_signature_tdb_input(struct mbuf **mp, struct tdb *tdbp, int skip,
+ int protoff)
{
- return (0);
+ m_freemp(mp);
+ return (EINVAL);
}
int
tcp_signature_tdb_output(struct mbuf *m, struct tdb *tdbp, int skip,
int protoff)
{
+ m_freem(m);
return (EINVAL);
}
Index: netinet/udp_usrreq.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/udp_usrreq.c,v
retrieving revision 1.262
diff -u -p -r1.262 udp_usrreq.c
--- netinet/udp_usrreq.c 22 Aug 2020 17:54:57 -0000 1.262
+++ netinet/udp_usrreq.c 22 Oct 2021 16:42:49 -0000
@@ -305,7 +305,7 @@ udp_input(struct mbuf **mp, int *offp, i
espstat_inc(esps_udpencin);
protoff = af == AF_INET ? offsetof(struct ip, ip_p) :
offsetof(struct ip6_hdr, ip6_nxt);
- ipsec_common_input(m, skip, protoff,
+ ipsec_common_input(mp, skip, protoff,
af, IPPROTO_ESP, 1);
return IPPROTO_DONE;
}
