> On 22 Oct 2021, at 21:38, Alexander Bluhm <[email protected]> wrote:
>
> 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?
>
ok mvs@
> 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;
> }
>
