Date: Fri, 1 May 2026 09:11:17 +0100 From: Oliver Pinter <oliver.pntr@gmail.com> To: Mark Johnston <markj@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org>, Mariusz Zaborski <oshogbo@freebsd.org> Subject: Re: git: f5ea3dce2cbe - main - libnv: switch fd_wait() from select(2) to poll(2) Message-ID: <CAPjTQNGzzY4sgCdKy0wrO2ipyYA1yefLhbbDE0RV2qFDCy06Hg@mail.gmail.com> In-Reply-To: <69f219fc.3d583.73783562@gitrepo.freebsd.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Wednesday, April 29, 2026, Mark Johnston <markj@freebsd.org> wrote: > The branch main has been updated by markj: > > URL: https://cgit.FreeBSD.org/src/commit/?id= > f5ea3dce2cbe1ee2068c5e5c11bb066f5789685b > > commit f5ea3dce2cbe1ee2068c5e5c11bb066f5789685b > Author: Mariusz Zaborski <oshogbo@FreeBSD.org> > AuthorDate: 2026-04-28 14:35:10 +0000 > Commit: Mark Johnston <markj@FreeBSD.org> > CommitDate: 2026-04-29 14:39:28 +0000 > > libnv: switch fd_wait() from select(2) to poll(2) > > The previous implementation used FD_SET() on a stack-allocated fd_set, > which is an out-of-bounds write whenever the socket fd is >= FD_SETSIZE > (1024). This problem seems like a more generic problem, after looking into sys/select.h What about adding a check for the FD_SET about the variable is on the stack or not? And enforce the FD_SETSIZE limit almost the same way as it is already done with fortify source. Maybe the https://lists.llvm.org/pipermail/llvm-dev/2013-October/066294.html can be used (or abused) to implement this check. Feel free to correct me, if I'm wrong. > Approved by: so > Security: FreeBSD-SA-26:16.libnv > Security: CVE-2026-39457 > Reported by: Joshua Rogers of AISLE Research Team ( > https://aisle.com/) > Reviewed by: markj > Differential Revision: https://reviews.freebsd.org/D56689 > --- > lib/libnv/msgio.c | 12 +++---- > lib/libnv/tests/nvlist_send_recv_test.c | 56 > +++++++++++++++++++++++++++++++++ > 2 files changed, 62 insertions(+), 6 deletions(-) > > diff --git a/lib/libnv/msgio.c b/lib/libnv/msgio.c > index f6f75241ff06..de2994e47fef 100644 > --- a/lib/libnv/msgio.c > +++ b/lib/libnv/msgio.c > @@ -32,10 +32,10 @@ > > #include <sys/param.h> > #include <sys/socket.h> > -#include <sys/select.h> > > #include <errno.h> > #include <fcntl.h> > +#include <poll.h> > #include <stdbool.h> > #include <stdint.h> > #include <stdlib.h> > @@ -86,14 +86,14 @@ msghdr_add_fd(struct cmsghdr *cmsg, int fd) > static void > fd_wait(int fd, bool doread) > { > - fd_set fds; > + struct pollfd pfd; > > PJDLOG_ASSERT(fd >= 0); > > - FD_ZERO(&fds); > - FD_SET(fd, &fds); > - (void)select(fd + 1, doread ? &fds : NULL, doread ? NULL : &fds, > - NULL, NULL); > + pfd.fd = fd; > + pfd.events = doread ? POLLIN : POLLOUT; > + pfd.revents = 0; > + (void)poll(&pfd, 1, -1); > } > > static int > diff --git a/lib/libnv/tests/nvlist_send_recv_test.c > b/lib/libnv/tests/nvlist_send_recv_test.c > index 4a5c10df656d..d655a26a7362 100644 > --- a/lib/libnv/tests/nvlist_send_recv_test.c > +++ b/lib/libnv/tests/nvlist_send_recv_test.c > @@ -27,6 +27,8 @@ > */ > > #include <sys/param.h> > +#include <sys/resource.h> > +#include <sys/select.h> > #include <sys/socket.h> > #include <sys/sysctl.h> > #include <sys/wait.h> > @@ -531,6 +533,59 @@ ATF_TC_BODY(nvlist_send_recv__send_nvlist__stream, > tc) > nvlist_send_recv__send_nvlist(SOCK_STREAM); > } > > +/* > + * Regression test for fd_wait(): the previous select(2)-based > implementation > + * called FD_SET() unconditionally, which is an out-of-bounds stack write > when > + * the socket fd is >= FD_SETSIZE. Force the socketpair fds above > FD_SETSIZE > + * and verify a full nvlist round-trip still works. > + */ > +ATF_TC_WITHOUT_HEAD(nvlist_send_recv__highfd); > +ATF_TC_BODY(nvlist_send_recv__highfd, tc) > +{ > + struct rlimit rl; > + nvlist_t *nvl; > + int socks[2], hi_send, hi_recv, status; > + pid_t pid; > + > + hi_send = FD_SETSIZE + 5; > + hi_recv = FD_SETSIZE + 6; > + > + rl.rlim_cur = rl.rlim_max = hi_recv + 1; > + if (setrlimit(RLIMIT_NOFILE, &rl) != 0) > + atf_tc_skip("cannot raise RLIMIT_NOFILE: %s", > strerror(errno)); > + > + ATF_REQUIRE(socketpair(PF_UNIX, SOCK_STREAM, 0, socks) == 0); > + ATF_REQUIRE(dup2(socks[0], hi_recv) == hi_recv); > + ATF_REQUIRE(dup2(socks[1], hi_send) == hi_send); > + (void)close(socks[0]); > + (void)close(socks[1]); > + > + pid = fork(); > + ATF_REQUIRE(pid >= 0); > + if (pid == 0) { > + /* Child: send. */ > + (void)close(hi_recv); > + nvl = nvlist_create(0); > + nvlist_add_string(nvl, "key", "value"); > + if (nvlist_send(hi_send, nvl) != 0) > + err(EXIT_FAILURE, "nvlist_send"); > + nvlist_destroy(nvl); > + _exit(0); > + } > + > + (void)close(hi_send); > + nvl = nvlist_recv(hi_recv, 0); > + ATF_REQUIRE(nvl != NULL); > + ATF_REQUIRE(nvlist_error(nvl) == 0); > + ATF_REQUIRE(nvlist_exists_string(nvl, "key")); > + ATF_REQUIRE(strcmp(nvlist_get_string(nvl, "key"), "value") == 0); > + nvlist_destroy(nvl); > + > + ATF_REQUIRE(waitpid(pid, &status, 0) == pid); > + ATF_REQUIRE(status == 0); > + (void)close(hi_recv); > +} > + > ATF_TC_WITHOUT_HEAD(nvlist_send_recv__send_closed_fd__dgram); > ATF_TC_BODY(nvlist_send_recv__send_closed_fd__dgram, tc) > { > @@ -734,6 +789,7 @@ ATF_TP_ADD_TCS(tp) > > ATF_TP_ADD_TC(tp, nvlist_send_recv__send_nvlist__dgram); > ATF_TP_ADD_TC(tp, nvlist_send_recv__send_nvlist__stream); > + ATF_TP_ADD_TC(tp, nvlist_send_recv__highfd); > ATF_TP_ADD_TC(tp, nvlist_send_recv__send_closed_fd__dgram); > ATF_TP_ADD_TC(tp, nvlist_send_recv__send_closed_fd__stream); > ATF_TP_ADD_TC(tp, nvlist_send_recv__send_many_fds__dgram); > > [-- Attachment #2 --] <br><br>On Wednesday, April 29, 2026, Mark Johnston <<a href="mailto:markj@freebsd.org">markj@freebsd.org</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The branch main has been updated by markj:<br> <br> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=f5ea3dce2cbe1ee2068c5e5c11bb066f5789685b" target="_blank">https://cgit.FreeBSD.org/src/<wbr>commit/?id=<wbr>f5ea3dce2cbe1ee2068c5e5c11bb06<wbr>6f5789685b</a><br>; <br> commit f5ea3dce2cbe1ee2068c5e5c11bb06<wbr>6f5789685b<br> Author: Mariusz Zaborski <oshogbo@FreeBSD.org><br> AuthorDate: 2026-04-28 14:35:10 +0000<br> Commit: Mark Johnston <markj@FreeBSD.org><br> CommitDate: 2026-04-29 14:39:28 +0000<br> <br> libnv: switch fd_wait() from select(2) to poll(2)<br> <br> The previous implementation used FD_SET() on a stack-allocated fd_set,<br> which is an out-of-bounds write whenever the socket fd is >= FD_SETSIZE<br> (1024).</blockquote><div><br></div><div>This problem seems like a more generic problem, after looking into sys/select.h</div><div><br></div><div>What about adding a check for the FD_SET about the variable is on the stack or not? And enforce the FD_SETSIZE limit almost the same way as it is already done with fortify source.</div><div><br></div><div>Maybe the <a href="https://lists.llvm.org/pipermail/llvm-dev/2013-October/066294.html">https://lists.llvm.org/pipermail/llvm-dev/2013-October/066294.html</a> can be used (or abused) to implement this check.</div><div> </div><div>Feel free to correct me, if I'm wrong.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Approved by: so<br> Security: FreeBSD-SA-26:16.libnv<br> Security: CVE-2026-39457<br> Reported by: Joshua Rogers of AISLE Research Team (<a href="https://aisle.com/" target="_blank">https://aisle.com/</a>)<br>; Reviewed by: markj<br> Differential Revision: <a href="https://reviews.freebsd.org/D56689" target="_blank">https://reviews.freebsd.org/<wbr>D56689</a><br>; ---<br> lib/libnv/msgio.c | 12 +++----<br> lib/libnv/tests/nvlist_send_<wbr>recv_test.c | 56 ++++++++++++++++++++++++++++++<wbr>+++<br> 2 files changed, 62 insertions(+), 6 deletions(-)<br> <br> diff --git a/lib/libnv/msgio.c b/lib/libnv/msgio.c<br> index f6f75241ff06..de2994e47fef 100644<br> --- a/lib/libnv/msgio.c<br> +++ b/lib/libnv/msgio.c<br> @@ -32,10 +32,10 @@<br> <br> #include <sys/param.h><br> #include <sys/socket.h><br> -#include <sys/select.h><br> <br> #include <errno.h><br> #include <fcntl.h><br> +#include <poll.h><br> #include <stdbool.h><br> #include <stdint.h><br> #include <stdlib.h><br> @@ -86,14 +86,14 @@ msghdr_add_fd(struct cmsghdr *cmsg, int fd)<br> static void<br> fd_wait(int fd, bool doread)<br> {<br> - fd_set fds;<br> + struct pollfd pfd;<br> <br> PJDLOG_ASSERT(fd >= 0);<br> <br> - FD_ZERO(&fds);<br> - FD_SET(fd, &fds);<br> - (void)select(fd + 1, doread ? &fds : NULL, doread ? NULL : &fds,<br> - NULL, NULL);<br> + pfd.fd = fd;<br> + pfd.events = doread ? POLLIN : POLLOUT;<br> + pfd.revents = 0;<br> + (void)poll(&pfd, 1, -1);<br> }<br> <br> static int<br> diff --git a/lib/libnv/tests/nvlist_send_<wbr>recv_test.c b/lib/libnv/tests/nvlist_send_<wbr>recv_test.c<br> index 4a5c10df656d..d655a26a7362 100644<br> --- a/lib/libnv/tests/nvlist_send_<wbr>recv_test.c<br> +++ b/lib/libnv/tests/nvlist_send_<wbr>recv_test.c<br> @@ -27,6 +27,8 @@<br> */<br> <br> #include <sys/param.h><br> +#include <sys/resource.h><br> +#include <sys/select.h><br> #include <sys/socket.h><br> #include <sys/sysctl.h><br> #include <sys/wait.h><br> @@ -531,6 +533,59 @@ ATF_TC_BODY(nvlist_send_recv__<wbr>send_nvlist__stream, tc)<br> nvlist_send_recv__send_nvlist(<wbr>SOCK_STREAM);<br> }<br> <br> +/*<br> + * Regression test for fd_wait(): the previous select(2)-based implementation<br> + * called FD_SET() unconditionally, which is an out-of-bounds stack write when<br> + * the socket fd is >= FD_SETSIZE. Force the socketpair fds above FD_SETSIZE<br> + * and verify a full nvlist round-trip still works.<br> + */<br> +ATF_TC_WITHOUT_HEAD(nvlist_<wbr>send_recv__highfd);<br> +ATF_TC_BODY(nvlist_send_recv_<wbr>_highfd, tc)<br> +{<br> + struct rlimit rl;<br> + nvlist_t *nvl;<br> + int socks[2], hi_send, hi_recv, status;<br> + pid_t pid;<br> +<br> + hi_send = FD_SETSIZE + 5;<br> + hi_recv = FD_SETSIZE + 6;<br> +<br> + rl.rlim_cur = rl.rlim_max = hi_recv + 1;<br> + if (setrlimit(RLIMIT_NOFILE, &rl) != 0)<br> + atf_tc_skip("cannot raise RLIMIT_NOFILE: %s", strerror(errno));<br> +<br> + ATF_REQUIRE(socketpair(PF_<wbr>UNIX, SOCK_STREAM, 0, socks) == 0);<br> + ATF_REQUIRE(dup2(socks[0], hi_recv) == hi_recv);<br> + ATF_REQUIRE(dup2(socks[1], hi_send) == hi_send);<br> + (void)close(socks[0]);<br> + (void)close(socks[1]);<br> +<br> + pid = fork();<br> + ATF_REQUIRE(pid >= 0);<br> + if (pid == 0) {<br> + /* Child: send. */<br> + (void)close(hi_recv);<br> + nvl = nvlist_create(0);<br> + nvlist_add_string(nvl, "key", "value");<br> + if (nvlist_send(hi_send, nvl) != 0)<br> + err(EXIT_FAILURE, "nvlist_send");<br> + nvlist_destroy(nvl);<br> + _exit(0);<br> + }<br> +<br> + (void)close(hi_send);<br> + nvl = nvlist_recv(hi_recv, 0);<br> + ATF_REQUIRE(nvl != NULL);<br> + ATF_REQUIRE(nvlist_error(nvl) == 0);<br> + ATF_REQUIRE(nvlist_exists_<wbr>string(nvl, "key"));<br> + ATF_REQUIRE(strcmp(nvlist_get_<wbr>string(nvl, "key"), "value") == 0);<br> + nvlist_destroy(nvl);<br> +<br> + ATF_REQUIRE(waitpid(pid, &status, 0) == pid);<br> + ATF_REQUIRE(status == 0);<br> + (void)close(hi_recv);<br> +}<br> +<br> ATF_TC_WITHOUT_HEAD(nvlist_<wbr>send_recv__send_closed_fd__<wbr>dgram);<br> ATF_TC_BODY(nvlist_send_recv__<wbr>send_closed_fd__dgram, tc)<br> {<br> @@ -734,6 +789,7 @@ ATF_TP_ADD_TCS(tp)<br> <br> ATF_TP_ADD_TC(tp, nvlist_send_recv__send_nvlist_<wbr>_dgram);<br> ATF_TP_ADD_TC(tp, nvlist_send_recv__send_nvlist_<wbr>_stream);<br> + ATF_TP_ADD_TC(tp, nvlist_send_recv__highfd);<br> ATF_TP_ADD_TC(tp, nvlist_send_recv__send_closed_<wbr>fd__dgram);<br> ATF_TP_ADD_TC(tp, nvlist_send_recv__send_closed_<wbr>fd__stream);<br> ATF_TP_ADD_TC(tp, nvlist_send_recv__send_many_<wbr>fds__dgram);<br> <br> </blockquote>home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPjTQNGzzY4sgCdKy0wrO2ipyYA1yefLhbbDE0RV2qFDCy06Hg>