Date: Tue, 13 Apr 2010 12:28:22 -0700 From: Xin LI <delphij@delphij.net> To: freebsd-net@freebsd.org Cc: John Baldwin <jhb@freebsd.org>, bschmidt@FreeBSD.ORG Subject: [PATCH FOR REVIEW] Fix SIOCGIFDESCR when buffer is too small Message-ID: <4BC4C5D6.9040605@delphij.net>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, Here is a patch that addressed the issue, where when SIOCGIFDESCR is fed with a smaller buffer. As reported by Bernhard, this would cause an infinite loop in ifconfig(8). The previous implementation claims that the 'length' field would be set to the number of length returned, and an error is returned. However, our ioctl(2) system call will not do copyout if there is errno being set, as discussed on -arch@ and thus the API needs to be tweaked. To minimize impact on ABI I have choose to use buffer as an indicator that the buffer length from userland is not sufficient, instead of returning ENAMETOOLONG. I'll also submit a patch for libpcap if this proposed change is considered be a good one. The libpcap in contrib/libpcap is not affected since it doesn't support dynamic length description. Cheers, - -- Xin LI <delphij@delphij.net> http://www.delphij.net/ FreeBSD - The Power to Serve! Live free or die -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (FreeBSD) iQEcBAEBAgAGBQJLxMXWAAoJEATO+BI/yjfBWc4H/jO7i2Rm+GqeYXX2eNWUjE2W 5dpNFq0kxqQWpLTr8qPskQ7o/ZDIl8ASbNJPdr/G+U1mYGVwNWVa6z0TR3huZZCB gPnR+84a+C/8rwtJjhOuyFKt/fdZfD4kI+rnWB+9Cq/uLX4aqziY1YO7SIAtb/1b RrjyM6rgYsMcnrqJKrmAQQEU1k6Yqkcy5PEEzU6MTSsHYL4wuKujZzmIYdZRg4rI OLSdLQEWq+u4PuOnrRMrvrrZZCObOURCWpjnJiP1yyMBE/ZW6itfMp6BE6k29vUz vZcDtqUFj3j1tVvaA4MzuX+isMUqnO8DvcnIawjwefs9Rq0mWY796kGSEjZYxuQ= =lyPJ -----END PGP SIGNATURE----- [-- Attachment #2 --] Index: sbin/ifconfig/ifconfig.c =================================================================== --- sbin/ifconfig/ifconfig.c (revision 206558) +++ sbin/ifconfig/ifconfig.c (working copy) @@ -922,19 +922,21 @@ ifr.ifr_buffer.buffer = descr; ifr.ifr_buffer.length = descrlen; if (ioctl(s, SIOCGIFDESCR, &ifr) == 0) { - if (strlen(descr) > 0) - printf("\tdescription: %s\n", descr); - break; - } else if (errno == ENAMETOOLONG) - descrlen = ifr.ifr_buffer.length; - else - break; - } else { + if (ifr.ifr_buffer.buffer == descr) { + if (strlen(descr) > 0) + printf("\tdescription: %s\n", + descr); + break; + } else if (ifr.ifr_buffer.length > descrlen) { + descrlen = ifr.ifr_buffer.length; + continue; + } + } + } else warn("unable to allocate memory for interface" "description"); - break; - } - }; + break; + } if (ioctl(s, SIOCGIFCAP, (caddr_t)&ifr) == 0) { if (ifr.ifr_curcap != 0) { Index: share/man/man4/netintro.4 =================================================================== --- share/man/man4/netintro.4 (revision 206558) +++ share/man/man4/netintro.4 (working copy) @@ -292,8 +292,11 @@ struct passed in as parameter, and the length would include the terminating nul character. If there is not enough space to hold the interface length, -no copy would be done and an -error would be returned. +no copy would be done and the +.Va buffer +field of +.Va ifru_buffer +would be set to NULL. The kernel will store the buffer length in the .Va length field upon return, regardless whether the buffer itself is Index: sys/net/if.c =================================================================== --- sys/net/if.c (revision 206558) +++ sys/net/if.c (working copy) @@ -2049,14 +2049,13 @@ case SIOCGIFDESCR: error = 0; sx_slock(&ifdescr_sx); - if (ifp->if_description == NULL) { - ifr->ifr_buffer.length = 0; + if (ifp->if_description == NULL) error = ENOMSG; - } else { + else { /* space for terminating nul */ descrlen = strlen(ifp->if_description) + 1; if (ifr->ifr_buffer.length < descrlen) - error = ENAMETOOLONG; + ifr->ifr_buffer.buffer = NULL; else error = copyout(ifp->if_description, ifr->ifr_buffer.buffer, descrlen);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4BC4C5D6.9040605>