From 36c0109eae52b8c24faab57b6f949fb2dd622cb5 Mon Sep 17 00:00:00 2001 From: bluhm Date: Mon, 19 Jun 2017 17:58:49 +0000 Subject: [PATCH] When dealing with mbuf pointers passed down as function parameters, bugs could easily result in use-after-free or double free. Introduce m_freemp() which automatically resets the pointer before freeing it. So we have less dangling pointers in the kernel. OK krw@ mpi@ claudio@ --- share/man/man9/mbuf.9 | 12 ++++++++++-- sys/net/bsd-comp.c | 5 ++--- sys/net/if_gif.c | 4 ++-- sys/net/pf_norm.c | 5 ++--- sys/net/ppp-deflate.c | 5 ++--- sys/netinet/igmp.c | 4 ++-- sys/netinet/ip_carp.c | 6 +++--- sys/netinet/ip_ether.c | 6 +++--- sys/netinet/ip_icmp.c | 4 ++-- sys/netinet/ip_input.c | 4 ++-- sys/netinet/ip_ipip.c | 7 +++---- sys/netinet/ipsec_input.c | 20 +++++++------------- sys/netinet6/ip6_input.c | 4 ++-- sys/nfs/nfs_socket.c | 13 +++++-------- sys/sys/mbuf.h | 11 ++++++++++- 15 files changed, 57 insertions(+), 53 deletions(-) diff --git a/share/man/man9/mbuf.9 b/share/man/man9/mbuf.9 index dba1344dc1b..49b39222370 100644 --- a/share/man/man9/mbuf.9 +++ b/share/man/man9/mbuf.9 @@ -1,4 +1,4 @@ -.\" $OpenBSD: mbuf.9,v 1.107 2017/05/19 05:59:27 jmc Exp $ +.\" $OpenBSD: mbuf.9,v 1.108 2017/06/19 17:58:49 bluhm Exp $ .\" .\" Copyright (c) 2001 Jean-Jacques Bernard-Gundol .\" All rights reserved. @@ -25,7 +25,7 @@ .\" (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF .\" THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. .\" -.Dd $Mdocdate: May 19 2017 $ +.Dd $Mdocdate: June 19 2017 $ .Dt MGET 9 .Os .Sh NAME @@ -48,6 +48,7 @@ .Nm m_copyback , .Nm m_defrag , .Nm m_freem , +.Nm m_freemp , .Nm m_purge , .Nm m_reclaim , .Nm m_copydata , @@ -103,6 +104,8 @@ .Fn m_defrag "struct mbuf *m" "int wait" .Ft struct mbuf * .Fn m_freem "struct mbuf *m" +.Ft struct mbuf * +.Fn m_freem "struct mbuf **mp" .Ft void .Fn m_purge "struct mbuf *m" .Ft void @@ -637,6 +640,11 @@ is a pointer, no action occurs and .Dv NULL is returned. +.It Fn m_freemp "struct mbuf **mp" +Set the input mbuf pointer to +.Dv NULL +and call +.Fn m_freem . .It Fn m_purge "struct mbuf *m" Free the list of mbufs linked by m_nextpkt that is pointed to by .Fa m . diff --git a/sys/net/bsd-comp.c b/sys/net/bsd-comp.c index 3816a4faf88..d087a63417d 100644 --- a/sys/net/bsd-comp.c +++ b/sys/net/bsd-comp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: bsd-comp.c,v 1.13 2015/11/24 13:37:16 mpi Exp $ */ +/* $OpenBSD: bsd-comp.c,v 1.14 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: bsd-comp.c,v 1.6 1996/10/13 02:10:58 christos Exp $ */ /* Because this code is derived from the 4.3BSD compress source: @@ -661,8 +661,7 @@ bsd_compress(state, mret, mp, slen, maxolen) ++db->uncomp_count; if (olen + PPP_HDRLEN + BSD_OVHD > maxolen) { /* throw away the compressed stuff if it is longer than uncompressed */ - m_freem(*mret); - *mret = NULL; + m_freemp(mret); ++db->incomp_count; db->incomp_bytes += ilen; diff --git a/sys/net/if_gif.c b/sys/net/if_gif.c index 446ccc9ccac..051cbcf3309 100644 --- a/sys/net/if_gif.c +++ b/sys/net/if_gif.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if_gif.c,v 1.96 2017/05/18 10:56:45 bluhm Exp $ */ +/* $OpenBSD: if_gif.c,v 1.97 2017/06/19 17:58:49 bluhm Exp $ */ /* $KAME: if_gif.c,v 1.43 2001/02/20 08:51:07 itojun Exp $ */ /* @@ -277,7 +277,7 @@ gif_encap(struct ifnet *ifp, struct mbuf **mp, sa_family_t af) break; #endif default: - m_freem(*mp); + m_freemp(mp); error = EAFNOSUPPORT; break; } diff --git a/sys/net/pf_norm.c b/sys/net/pf_norm.c index e694b55f9b4..c1f8d5e3e1d 100644 --- a/sys/net/pf_norm.c +++ b/sys/net/pf_norm.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_norm.c,v 1.205 2017/06/05 22:18:28 sashan Exp $ */ +/* $OpenBSD: pf_norm.c,v 1.206 2017/06/19 17:58:49 bluhm Exp $ */ /* * Copyright 2001 Niels Provos @@ -773,8 +773,7 @@ pf_refragment6(struct mbuf **m0, struct m_tag *mtag, struct sockaddr_in6 *dst, (*m0)->m_nextpkt = NULL; if (error == 0) { /* The first mbuf contains the unfragmented packet */ - m_freem(*m0); - *m0 = NULL; + m_freemp(m0); action = PF_PASS; } else { /* Drop expects an mbuf to free */ diff --git a/sys/net/ppp-deflate.c b/sys/net/ppp-deflate.c index 98fe3110f36..cd75e9c8b26 100644 --- a/sys/net/ppp-deflate.c +++ b/sys/net/ppp-deflate.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ppp-deflate.c,v 1.12 2015/07/15 22:16:42 deraadt Exp $ */ +/* $OpenBSD: ppp-deflate.c,v 1.13 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: ppp-deflate.c,v 1.1 1996/03/15 02:28:09 paulus Exp $ */ /* @@ -342,8 +342,7 @@ z_compress(arg, mret, mp, orig_len, maxolen) state->stats.comp_bytes += olen; state->stats.comp_packets++; } else { - m_freem(*mret); - *mret = NULL; + m_freemp(mret); state->stats.inc_bytes += orig_len; state->stats.inc_packets++; diff --git a/sys/netinet/igmp.c b/sys/netinet/igmp.c index ed7bba46a7d..b4b23884d47 100644 --- a/sys/netinet/igmp.c +++ b/sys/netinet/igmp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: igmp.c,v 1.68 2017/05/16 12:24:01 mpi Exp $ */ +/* $OpenBSD: igmp.c,v 1.69 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: igmp.c,v 1.15 1996/02/13 23:41:25 christos Exp $ */ /* @@ -217,7 +217,7 @@ igmp_input(struct mbuf **mp, int *offp, int proto, int af) ifp = if_get((*mp)->m_pkthdr.ph_ifidx); if (ifp == NULL) { - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } diff --git a/sys/netinet/ip_carp.c b/sys/netinet/ip_carp.c index 97af0d33f0d..fc6f5a1b0bf 100644 --- a/sys/netinet/ip_carp.c +++ b/sys/netinet/ip_carp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_carp.c,v 1.312 2017/05/30 12:09:27 friehm Exp $ */ +/* $OpenBSD: ip_carp.c,v 1.313 2017/06/19 17:58:49 bluhm Exp $ */ /* * Copyright (c) 2002 Michael Shalayeff. All rights reserved. @@ -421,7 +421,7 @@ carp_proto_input(struct mbuf **mp, int *offp, int proto, int af) ifp = if_get((*mp)->m_pkthdr.ph_ifidx); if (ifp == NULL) { - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } @@ -517,7 +517,7 @@ carp6_proto_input(struct mbuf **mp, int *offp, int proto, int af) ifp = if_get((*mp)->m_pkthdr.ph_ifidx); if (ifp == NULL) { - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } diff --git a/sys/netinet/ip_ether.c b/sys/netinet/ip_ether.c index 2fe409e08e7..c90b61441c0 100644 --- a/sys/netinet/ip_ether.c +++ b/sys/netinet/ip_ether.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_ether.c,v 1.85 2017/04/14 20:46:31 bluhm Exp $ */ +/* $OpenBSD: ip_ether.c,v 1.86 2017/06/19 17:58:49 bluhm Exp $ */ /* * The author of this code is Angelos D. Keromytis (kermit@adk.gr) * @@ -97,7 +97,7 @@ etherip_input(struct mbuf **mp, int *offp, int proto, int af) if (!etherip_allow && ((*mp)->m_flags & (M_AUTH|M_CONF)) == 0) { DPRINTF(("etherip_input(): dropped due to policy\n")); etheripstat.etherips_pdrops++; - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } etherip_decap(*mp, *offp); @@ -111,7 +111,7 @@ etherip_input(struct mbuf **mp, int *offp, int proto, int af) default: DPRINTF(("etherip_input(): dropped, unhandled protocol\n")); etheripstat.etherips_pdrops++; - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } } diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c index 1c321375fa7..a30c96d7253 100644 --- a/sys/netinet/ip_icmp.c +++ b/sys/netinet/ip_icmp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_icmp.c,v 1.169 2017/05/30 12:09:27 friehm Exp $ */ +/* $OpenBSD: ip_icmp.c,v 1.170 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: ip_icmp.c,v 1.19 1996/02/13 23:42:22 christos Exp $ */ /* @@ -312,7 +312,7 @@ icmp_input(struct mbuf **mp, int *offp, int proto, int af) ifp = if_get((*mp)->m_pkthdr.ph_ifidx); if (ifp == NULL) { - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c index c4bbed9d015..b24f37858c4 100644 --- a/sys/netinet/ip_input.c +++ b/sys/netinet/ip_input.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_input.c,v 1.311 2017/06/19 17:00:16 bluhm Exp $ */ +/* $OpenBSD: ip_input.c,v 1.312 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: ip_input.c,v 1.30 1996/03/16 23:53:58 christos Exp $ */ /* @@ -605,7 +605,7 @@ ip_deliver(struct mbuf **mp, int *offp, int nxt, int af) #ifdef IPSEC bad: #endif - m_freem(*mp); + m_freemp(mp); } int diff --git a/sys/netinet/ip_ipip.c b/sys/netinet/ip_ipip.c index c1a839dddc6..214d4c31b16 100644 --- a/sys/netinet/ip_ipip.c +++ b/sys/netinet/ip_ipip.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_ipip.c,v 1.83 2017/06/11 19:59:57 bluhm Exp $ */ +/* $OpenBSD: ip_ipip.c,v 1.84 2017/06/19 17:58:49 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -100,7 +100,7 @@ ipip_input(struct mbuf **mp, int *offp, int proto, int af) if (!ipip_allow && ((*mp)->m_flags & (M_AUTH|M_CONF)) == 0) { DPRINTF(("%s: dropped due to policy\n", __func__)); ipipstat_inc(ipips_pdrops); - m_freem(*mp); + m_freemp(mp); return IPPROTO_DONE; } @@ -324,8 +324,7 @@ ipip_input_gif(struct mbuf **mp, int *offp, int proto, int oaf, #endif } bad: - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } diff --git a/sys/netinet/ipsec_input.c b/sys/netinet/ipsec_input.c index a67539c31e1..05c0ae4ba13 100644 --- a/sys/netinet/ipsec_input.c +++ b/sys/netinet/ipsec_input.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ipsec_input.c,v 1.154 2017/05/28 09:25:51 bluhm Exp $ */ +/* $OpenBSD: ipsec_input.c,v 1.155 2017/06/19 17:58:49 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -867,8 +867,7 @@ ah6_input(struct mbuf **mp, int *offp, int proto, int af) if (*offp < sizeof(struct ip6_hdr)) { DPRINTF(("ah6_input(): bad offset\n")); ahstat.ahs_hdrops++; - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } else if (*offp == sizeof(struct ip6_hdr)) { protoff = offsetof(struct ip6_hdr, ip6_nxt); @@ -898,8 +897,7 @@ ah6_input(struct mbuf **mp, int *offp, int proto, int af) if (protoff + l != *offp) { DPRINTF(("ah6_input(): bad packet header chain\n")); ahstat.ahs_hdrops++; - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } protoff += offsetof(struct ip6_ext, ip6e_nxt); @@ -919,8 +917,7 @@ esp6_input(struct mbuf **mp, int *offp, int proto, int af) if (*offp < sizeof(struct ip6_hdr)) { DPRINTF(("esp6_input(): bad offset\n")); espstat.esps_hdrops++; - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } else if (*offp == sizeof(struct ip6_hdr)) { protoff = offsetof(struct ip6_hdr, ip6_nxt); @@ -950,8 +947,7 @@ esp6_input(struct mbuf **mp, int *offp, int proto, int af) if (protoff + l != *offp) { DPRINTF(("esp6_input(): bad packet header chain\n")); espstat.esps_hdrops++; - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } protoff += offsetof(struct ip6_ext, ip6e_nxt); @@ -972,8 +968,7 @@ ipcomp6_input(struct mbuf **mp, int *offp, int proto, int af) if (*offp < sizeof(struct ip6_hdr)) { DPRINTF(("ipcomp6_input(): bad offset\n")); ipcompstat.ipcomps_hdrops++; - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } else if (*offp == sizeof(struct ip6_hdr)) { protoff = offsetof(struct ip6_hdr, ip6_nxt); @@ -1002,8 +997,7 @@ ipcomp6_input(struct mbuf **mp, int *offp, int proto, int af) if (protoff + l != *offp) { DPRINTF(("ipcomp6_input(): bad packet header chain\n")); ipcompstat.ipcomps_hdrops++; - m_freem(*mp); - *mp = NULL; + m_freemp(mp); return IPPROTO_DONE; } diff --git a/sys/netinet6/ip6_input.c b/sys/netinet6/ip6_input.c index b763c59e562..53dfe484017 100644 --- a/sys/netinet6/ip6_input.c +++ b/sys/netinet6/ip6_input.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip6_input.c,v 1.195 2017/06/19 17:00:16 bluhm Exp $ */ +/* $OpenBSD: ip6_input.c,v 1.196 2017/06/19 17:58:49 bluhm Exp $ */ /* $KAME: ip6_input.c,v 1.188 2001/03/29 05:34:31 itojun Exp $ */ /* @@ -579,7 +579,7 @@ ip6_deliver(struct mbuf **mp, int *offp, int nxt, int af) } return; bad: - m_freem(*mp); + m_freemp(mp); } int diff --git a/sys/nfs/nfs_socket.c b/sys/nfs/nfs_socket.c index 619951ba1d5..b94cbeb7b90 100644 --- a/sys/nfs/nfs_socket.c +++ b/sys/nfs/nfs_socket.c @@ -1,4 +1,4 @@ -/* $OpenBSD: nfs_socket.c,v 1.116 2017/05/17 08:59:05 mpi Exp $ */ +/* $OpenBSD: nfs_socket.c,v 1.117 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: nfs_socket.c,v 1.27 1996/04/15 20:20:00 thorpej Exp $ */ /* @@ -673,8 +673,7 @@ tryagain: } errout: if (error && error != EINTR && error != ERESTART) { - m_freem(*mp); - *mp = NULL; + m_freemp(mp); if (error != EPIPE) log(LOG_INFO, "receive error %d from nfs server %s\n", @@ -707,10 +706,8 @@ errout: } while (error == EWOULDBLOCK); len -= auio.uio_resid; } - if (error) { - m_freem(*mp); - *mp = NULL; - } + if (error) + m_freemp(mp); /* * Search for any mbufs that are not a multiple of 4 bytes long * or with m_data not longword aligned. @@ -1418,7 +1415,7 @@ nfs_realign(struct mbuf **pm, int hsiz) off += m->m_len; m = m->m_next; } - m_freem(*pm); + m_freemp(pm); *pm = n; } } diff --git a/sys/sys/mbuf.h b/sys/sys/mbuf.h index f6607a2eead..7f184049c81 100644 --- a/sys/sys/mbuf.h +++ b/sys/sys/mbuf.h @@ -1,4 +1,4 @@ -/* $OpenBSD: mbuf.h,v 1.229 2017/05/30 12:09:27 friehm Exp $ */ +/* $OpenBSD: mbuf.h,v 1.230 2017/06/19 17:58:49 bluhm Exp $ */ /* $NetBSD: mbuf.h,v 1.19 1996/02/09 18:25:14 christos Exp $ */ /* @@ -463,6 +463,15 @@ int m_apply(struct mbuf *, int, int, struct mbuf *m_dup_pkt(struct mbuf *, unsigned int, int); int m_dup_pkthdr(struct mbuf *, struct mbuf *, int); +static inline struct mbuf * +m_freemp(struct mbuf **mp) +{ + struct mbuf *m = *mp; + + *mp = NULL; + return m_freem(m); +} + /* Packet tag routines */ struct m_tag *m_tag_get(int, int, int); void m_tag_prepend(struct mbuf *, struct m_tag *); -- 2.20.1