kern/99425: [ipv6] ping6 memory leak due to IPV6_RTHDR setsockopt b
ehavior and inet6_rth_* functions.
Mike Makonnen
mtm at FreeBSD.Org
Fri Apr 6 10:00:25 UTC 2007
The following reply was made to PR kern/99425; it has been noted by GNATS.
From: Mike Makonnen <mtm at FreeBSD.Org>
To: bug-followup at FreeBSD.org, clemun at gmail.com
Cc: ume at freebsd.org, gnn at 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--
More information about the freebsd-bugs
mailing list