Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 6 Apr 2007 10:00:25 GMT
From:      Mike Makonnen <mtm@FreeBSD.Org>
To:        freebsd-bugs@FreeBSD.org
Subject:   kern/99425: [ipv6] ping6 memory leak due to IPV6_RTHDR setsockopt b ehavior and inet6_rth_* functions.
Message-ID:  <200704061000.l36A0PfO030305@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/99425; it has been noted by GNATS.

From: Mike Makonnen <mtm@FreeBSD.Org>
To: bug-followup@FreeBSD.org, clemun@gmail.com
Cc: ume@freebsd.org, gnn@freebsd.org
Subject: kern/99425: [ipv6] ping6 memory leak due to IPV6_RTHDR setsockopt b
	ehavior and inet6_rth_* functions.
Date: Fri, 6 Apr 2007 13:02:50 +0300

 --9amGYk9869ThD9tj
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 
 [cc'ing our resident IPv6 gurus]
 
 Hi,
 
 Ok, it seems the problem is that there are 2 problems here:
  1. The static buffer that ping6(8) uses to hold the control data
     it gets from recvmsg(2) is too small.
  2. When it outputs the extra header information it doesn't
     confirm that its static buffer is indeed big enough to hold all
     the data that the headers tell it is there.
 
 The attached patch solves both problems.
 First, the buffer to hold the control data is increased to 10240 bytes
 because RFC3542 section 20.1 says that the buffer to hold the control
 data must be a minimum of 10420 bytes. Second, pr_ip6opt() and
 pr_rthdr() are modified to check that the size of the buffer, allocated
 in main(), to hold the control data is big enough to hold the data that
 the headers say is included. If it is NOT big enough then they make sure
 not to read outside of the allocated buffer. Third, a warning is printed
 if the buffer is not big enough to hold all the data (by checking if
 recvmsg(8) has appened MSG_CTRUNC to the msghdr flags).
 
 Also, as stated in the PR the inet6_rth_* functions do NOT check that
 the number of segments is within the limits set by RFC3542, but that is
 a separate issue and will be fixed separately.
 
 There is no problem when sending because the buffer to hold the
 control data is dynamically allocated to the correct size. It also
 correctly checks the return value of inet6_rth_space() to make sure
 there are no problems with routing header type or number. The problem
 is that inet6_rth_space() doesn't do the proper checking. But, as
 I stated in the previous paragraph that's a separate issue and I'll fix
 it in a separate commit (I'm working on the patch now).
 
 Please apply the attached patch. Let me know if it doesn't behave
 as advertised :-)
 
 Cheers.
 -- 
 Mike Makonnen         | GPG-KEY: http://people.freebsd.org/~mtm/mtm.asc
 mmakonnen @ gmail.com | AC7B 5672 2D11 F4D0 EBF8  5279 5359 2B82 7CD4 1F55
 mtm @ FreeBSD.Org     | FreeBSD - http://www.freebsd.org
 
 --9amGYk9869ThD9tj
 Content-Type: text/x-diff; charset=us-ascii
 Content-Disposition: attachment; filename="ping6.diff"
 
 Index: sbin/ping6/ping6.c
 ===================================================================
 RCS file: /home/ncvs/src/sbin/ping6/ping6.c,v
 retrieving revision 1.29
 diff -u -u -r1.29 ping6.c
 --- sbin/ping6/ping6.c	10 Feb 2005 09:19:32 -0000	1.29
 +++ sbin/ping6/ping6.c	5 Apr 2007 22:32:52 -0000
 @@ -150,6 +150,7 @@
  #define ICMP6ECHOLEN	8	/* icmp echo header len excluding time */
  #define ICMP6ECHOTMLEN sizeof(struct tv32)
  #define ICMP6_NIQLEN	(ICMP6ECHOLEN + 8)
 +# define CONTROLLEN	10240	/* ancillary data buffer size RFC3542 20.1 */
  /* FQDN case, 64 bits of nonce + 32 bits ttl */
  #define ICMP6_NIRLEN	(ICMP6ECHOLEN + 12)
  #define	EXTRA		256	/* for AH and various other headers. weird. */
 @@ -269,8 +270,8 @@
  	char *, size_t);
  void	 pr_pack(u_char *, int, struct msghdr *);
  void	 pr_exthdrs(struct msghdr *);
 -void	 pr_ip6opt(void *);
 -void	 pr_rthdr(void *);
 +void	 pr_ip6opt(void *, unsigned int);
 +void	 pr_rthdr(void *, unsigned int);
  int	 pr_bitrange(u_int32_t, int, int);
  void	 pr_retip(struct ip6_hdr *, u_char *);
  void	 summary(void);
 @@ -307,6 +308,7 @@
  	char *e, *target, *ifname = NULL, *gateway = NULL;
  	int ip6optlen = 0;
  	struct cmsghdr *scmsgp = NULL;
 +	struct cmsghdr *cm;
  #if defined(SO_SNDBUF) && defined(SO_RCVBUF)
  	u_long lsockbufsize;
  	int sockbufsize = 0;
 @@ -1057,10 +1059,13 @@
  	seeninfo = 0;
  #endif
  
 +	/* For control (ancillary) data received from recvmsg() */
 +	cm = (struct cmsghdr *)malloc(CONTROLLEN);
 +	if (cm == NULL)
 +		err(1, "malloc");
 +
  	for (;;) {
  		struct msghdr m;
 -		struct cmsghdr *cm;
 -		u_char buf[1024];
  		struct iovec iov[2];
  
  		/* signal handling */
 @@ -1127,9 +1132,9 @@
  		iov[0].iov_len = packlen;
  		m.msg_iov = iov;
  		m.msg_iovlen = 1;
 -		cm = (struct cmsghdr *)buf;
 -		m.msg_control = (caddr_t)buf;
 -		m.msg_controllen = sizeof(buf);
 +		memset(cm, 0, CONTROLLEN);
 +		m.msg_control = (void *)cm;
 +		m.msg_controllen = CONTROLLEN;
  
  		cc = recvmsg(s, &m, 0);
  		if (cc < 0) {
 @@ -1493,6 +1498,9 @@
  			    pr_addr(from, fromlen));
  		return;
  	}
 +	if (((mhdr->msg_flags & MSG_CTRUNC) != 0) &&
 +	    (options & F_VERBOSE) != 0)
 +		warnx("some control data discarded, insufficient buffer size");
  	icp = (struct icmp6_hdr *)buf;
  	ni = (struct icmp6_nodeinfo *)buf;
  	off = 0;
 @@ -1735,28 +1743,31 @@
  pr_exthdrs(mhdr)
  	struct msghdr *mhdr;
  {
 -	struct cmsghdr *cm;
 +	struct cmsghdr *cm, *cm1;
 +	unsigned int offset;
  
 -	for (cm = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr); cm;
 -	     cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) {
 +	offset = 0;
 +	cm1 = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr);
 +	for (cm = cm1; cm; cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) {
  		if (cm->cmsg_level != IPPROTO_IPV6)
  			continue;
  
 +		offset = (caddr_t)CMSG_DATA(cm) - (caddr_t)cm1;
  		switch (cm->cmsg_type) {
  		case IPV6_HOPOPTS:
  			printf("  HbH Options: ");
 -			pr_ip6opt(CMSG_DATA(cm));
 +			pr_ip6opt(CMSG_DATA(cm), offset);
  			break;
  		case IPV6_DSTOPTS:
  #ifdef IPV6_RTHDRDSTOPTS
  		case IPV6_RTHDRDSTOPTS:
  #endif
  			printf("  Dst Options: ");
 -			pr_ip6opt(CMSG_DATA(cm));
 +			pr_ip6opt(CMSG_DATA(cm), offset);
  			break;
  		case IPV6_RTHDR:
  			printf("  Routing: ");
 -			pr_rthdr(CMSG_DATA(cm));
 +			pr_rthdr(CMSG_DATA(cm), offset);
  			break;
  		}
  	}
 @@ -1764,12 +1775,12 @@
  
  #ifdef USE_RFC2292BIS
  void
 -pr_ip6opt(void *extbuf)
 +pr_ip6opt(void *extbuf, unsigned int cmoffset)
  {
  	struct ip6_hbh *ext;
  	int currentlen;
  	u_int8_t type;
 -	socklen_t extlen, len;
 +	socklen_t extlen, len, origextlen;
  	void *databuf;
  	size_t offset;
  	u_int16_t value2;
 @@ -1780,6 +1791,21 @@
  	printf("nxt %u, len %u (%lu bytes)\n", ext->ip6h_nxt,
  	    (unsigned int)ext->ip6h_len, (unsigned long)extlen);
  
 +	/*
 +	 * Bounds checking on the ancillary data buffer. When calculating
 +	 * the number of items to show keep in mind:
 +	 *	- The offset, in the ancillary data buffer, of this header
 +	 *	- The size of the cmsg structure
 +	 *	- The size of one unit (8 octets)
 +	 */
 +	if (CONTROLLEN < (extlen  + CMSG_SPACE(0) + offset)) {
 +		origextlen = extlen;
 +		extlen -= (extlen - (CONTROLLEN - offset - CMSG_SPACE(0))) / 8;
 +		warnx("options truncated, showing only %u (total=%u)",
 +		    (unsigned int)(extlen / 8 - 1),
 +		    (unsigned int)(ext->ip6h_len));
 +	}
 +
  	currentlen = 0;
  	while (1) {
  		currentlen = inet6_opt_next(extbuf, extlen, currentlen,
 @@ -1816,7 +1842,7 @@
  #else  /* !USE_RFC2292BIS */
  /* ARGSUSED */
  void
 -pr_ip6opt(void *extbuf)
 +pr_ip6opt(void *extbuf, unsigned int cmoffset __unused)
  {
  	putchar('\n');
  	return;
 @@ -1825,21 +1851,44 @@
  
  #ifdef USE_RFC2292BIS
  void
 -pr_rthdr(void *extbuf)
 +pr_rthdr(void *extbuf, unsigned int offset)
  {
  	struct in6_addr *in6;
  	char ntopbuf[INET6_ADDRSTRLEN];
  	struct ip6_rthdr *rh = (struct ip6_rthdr *)extbuf;
 -	int i, segments;
 +	int i, segments, origsegs, rthsize, size0, size1;
  
  	/* print fixed part of the header */
  	printf("nxt %u, len %u (%d bytes), type %u, ", rh->ip6r_nxt,
  	    rh->ip6r_len, (rh->ip6r_len + 1) << 3, rh->ip6r_type);
 -	if ((segments = inet6_rth_segments(extbuf)) >= 0)
 +	if ((segments = inet6_rth_segments(extbuf)) >= 0) {
  		printf("%d segments, ", segments);
 -	else
 +		printf("%d left\n", rh->ip6r_segleft);
 +	} else {
  		printf("segments unknown, ");
 -	printf("%d left\n", rh->ip6r_segleft);
 +		printf("%d left\n", rh->ip6r_segleft);
 +		return;
 +	}
 +
 +	/*
 +	 * Bounds checking on the ancillary data buffer. When calculating
 +	 * the number of items to show keep in mind:
 +	 *	- The offset, in the ancillary data buffer, of this header
 +	 *	- The size of the cmsg structure
 +	 *	- The size of one segment (the size of one IPv6 address)
 +	 *	- When dividing add a fudge factor of one in case the
 +	 *	  dividend is not evenly divisible by the divisor
 +	 */
 +	rthsize = (rh->ip6r_len + 1) * 8;
 +	if (CONTROLLEN < (rthsize + CMSG_SPACE(0) + offset)) {
 +		origsegs = segments;
 +		size0 = inet6_rth_space(IPV6_RTHDR_TYPE_0, 0);
 +		size1 = inet6_rth_space(IPV6_RTHDR_TYPE_0, 1);
 +		segments -= (rthsize - (CONTROLLEN - offset - CMSG_SPACE(0))) /
 +		    (size1 - size0) + 1;
 +		warnx("segments truncated, showing only %d (total=%d)",
 +		    segments, origsegs);
 +	}
  
  	for (i = 0; i < segments; i++) {
  		in6 = inet6_rth_getaddr(extbuf, i);
 @@ -1860,7 +1909,7 @@
  #else  /* !USE_RFC2292BIS */
  /* ARGSUSED */
  void
 -pr_rthdr(void *extbuf)
 +pr_rthdr(void *extbuf, unsigned int offset __unused)
  {
  	putchar('\n');
  	return;
 
 --9amGYk9869ThD9tj--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200704061000.l36A0PfO030305>