svn commit: r332657 - in head: etc/mtree sys/kern sys/sys tests/sys tests/sys/capsicum
John Baldwin
jhb at FreeBSD.org
Tue Apr 17 18:07:41 UTC 2018
Author: jhb
Date: Tue Apr 17 18:07:40 2018
New Revision: 332657
URL: https://svnweb.freebsd.org/changeset/base/332657
Log:
Properly do a deep copy of the ioctls capability array for fget_cap().
fget_cap() tries to do a cheaper snapshot of a file descriptor without
holding the file descriptor lock. This snapshot does not do a deep
copy of the ioctls capability array, but instead uses a different
return value to inform the caller to retry the copy with the lock
held. However, filecaps_copy() was returning 1 to indicate that a
retry was required, and fget_cap() was checking for 0 (actually
'!filecaps_copy()'). As a result, fget_cap() did not do a deep copy
of the ioctls array and just reused the original pointer. This cause
multiple file descriptor entries to think they owned the same pointer
and eventually resulted in duplicate frees.
The only code path that I'm aware of that triggers this is to create a
listen socket that has a restricted list of ioctls and then call
accept() which calls fget_cap() with a valid filecaps structure from
getsock_cap().
To fix, change the return value of filecaps_copy() to return true if
it succeeds in copying the caps and false if it fails because the lock
is required. I find this more intuitive than fixing the caller in
this case. While here, change the return type from 'int' to 'bool'.
Finally, make filecaps_copy() more robust in the failure case by not
copying any of the source filecaps structure over. This avoids the
possibility of leaking a pointer into a structure if a similar future
caller doesn't properly handle the return value from filecaps_copy()
at the expense of one more branch.
I also added a test case that panics before this change and now passes.
Reviewed by: kib
Discussed with: mjg (not a fan of the extra branch)
MFC after: 1 week
Differential Revision: https://reviews.freebsd.org/D15047
Added:
head/tests/sys/capsicum/
head/tests/sys/capsicum/Makefile (contents, props changed)
head/tests/sys/capsicum/ioctls_test.c (contents, props changed)
Modified:
head/etc/mtree/BSD.tests.dist
head/sys/kern/kern_descrip.c
head/sys/sys/filedesc.h
head/tests/sys/Makefile
Modified: head/etc/mtree/BSD.tests.dist
==============================================================================
--- head/etc/mtree/BSD.tests.dist Tue Apr 17 18:04:28 2018 (r332656)
+++ head/etc/mtree/BSD.tests.dist Tue Apr 17 18:07:40 2018 (r332657)
@@ -430,6 +430,8 @@
..
aio
..
+ capsicum
+ ..
cddl
zfs
bin
Modified: head/sys/kern/kern_descrip.c
==============================================================================
--- head/sys/kern/kern_descrip.c Tue Apr 17 18:04:28 2018 (r332656)
+++ head/sys/kern/kern_descrip.c Tue Apr 17 18:07:40 2018 (r332657)
@@ -1492,16 +1492,16 @@ filecaps_init(struct filecaps *fcaps)
* Note that if the table was not locked, the caller has to check the relevant
* sequence counter to determine whether the operation was successful.
*/
-int
+bool
filecaps_copy(const struct filecaps *src, struct filecaps *dst, bool locked)
{
size_t size;
+ if (src->fc_ioctls != NULL && !locked)
+ return (false);
*dst = *src;
if (src->fc_ioctls == NULL)
- return (0);
- if (!locked)
- return (1);
+ return (true);
KASSERT(src->fc_nioctls > 0,
("fc_ioctls != NULL, but fc_nioctls=%hd", src->fc_nioctls));
@@ -1509,7 +1509,7 @@ filecaps_copy(const struct filecaps *src, struct filec
size = sizeof(src->fc_ioctls[0]) * src->fc_nioctls;
dst->fc_ioctls = malloc(size, M_FILECAPS, M_WAITOK);
bcopy(src->fc_ioctls, dst->fc_ioctls, size);
- return (0);
+ return (true);
}
static u_long *
Modified: head/sys/sys/filedesc.h
==============================================================================
--- head/sys/sys/filedesc.h Tue Apr 17 18:04:28 2018 (r332656)
+++ head/sys/sys/filedesc.h Tue Apr 17 18:07:40 2018 (r332657)
@@ -155,7 +155,7 @@ enum {
struct thread;
void filecaps_init(struct filecaps *fcaps);
-int filecaps_copy(const struct filecaps *src, struct filecaps *dst,
+bool filecaps_copy(const struct filecaps *src, struct filecaps *dst,
bool locked);
void filecaps_move(struct filecaps *src, struct filecaps *dst);
void filecaps_free(struct filecaps *fcaps);
Modified: head/tests/sys/Makefile
==============================================================================
--- head/tests/sys/Makefile Tue Apr 17 18:04:28 2018 (r332656)
+++ head/tests/sys/Makefile Tue Apr 17 18:07:40 2018 (r332657)
@@ -6,6 +6,7 @@ TESTSDIR= ${TESTSBASE}/sys
TESTS_SUBDIRS+= acl
TESTS_SUBDIRS+= aio
+TESTS_SUBDIRS+= capsicum
TESTS_SUBDIRS+= ${_cddl}
TESTS_SUBDIRS+= fifo
TESTS_SUBDIRS+= file
Added: head/tests/sys/capsicum/Makefile
==============================================================================
--- /dev/null 00:00:00 1970 (empty, because file is newly added)
+++ head/tests/sys/capsicum/Makefile Tue Apr 17 18:07:40 2018 (r332657)
@@ -0,0 +1,9 @@
+# $FreeBSD$
+
+TESTSDIR= ${TESTSBASE}/sys/capsicum
+
+ATF_TESTS_C+= ioctls_test
+
+WARNS?= 6
+
+.include <bsd.test.mk>
Added: head/tests/sys/capsicum/ioctls_test.c
==============================================================================
--- /dev/null 00:00:00 1970 (empty, because file is newly added)
+++ head/tests/sys/capsicum/ioctls_test.c Tue Apr 17 18:07:40 2018 (r332657)
@@ -0,0 +1,127 @@
+/*-
+ * Copyright (c) 2018 John Baldwin <jhb at FreeBSD.org>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <sys/cdefs.h>
+__FBSDID("$FreeBSD$");
+
+#include <sys/capsicum.h>
+#include <sys/filio.h>
+#include <sys/socket.h>
+#include <sys/wait.h>
+#include <netinet/in.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <atf-c.h>
+
+/*
+ * A variant of ATF_REQUIRE that is suitable for use in child
+ * processes. This only works if the parent process is tripped up by
+ * the early exit and fails some requirement itself.
+ */
+#define CHILD_REQUIRE(exp) do { \
+ if (!(exp)) \
+ child_fail_require(__FILE__, __LINE__, \
+ #exp " not met"); \
+ } while (0)
+
+static __dead2 void
+child_fail_require(const char *file, int line, const char *str)
+{
+ char buf[128];
+
+ snprintf(buf, sizeof(buf), "%s:%d: %s\n", file, line, str);
+ write(2, buf, strlen(buf));
+ _exit(32);
+}
+
+/*
+ * Exercise the edge case of a custom ioctl list being copied from a
+ * listen socket to an accepted socket.
+ */
+ATF_TC_WITHOUT_HEAD(cap_ioctls__listen_copy);
+ATF_TC_BODY(cap_ioctls__listen_copy, tc)
+{
+ struct sockaddr_in sin;
+ cap_rights_t rights;
+ u_long cmds[] = { FIONREAD };
+ socklen_t len;
+ pid_t pid;
+ char dummy;
+ int s[2], status;
+
+ s[0] = socket(AF_INET, SOCK_STREAM, 0);
+ ATF_REQUIRE(s[0] > 0);
+
+ /* Bind to an arbitrary unused port. */
+ memset(&sin, 0, sizeof(sin));
+ sin.sin_len = sizeof(sin);
+ sin.sin_family = AF_INET;
+ sin.sin_port = 0;
+ sin.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+ ATF_REQUIRE(bind(s[0], (struct sockaddr *)&sin, sizeof(sin)) == 0);
+
+ CHILD_REQUIRE(listen(s[0], 1) == 0);
+
+ len = sizeof(sin);
+ ATF_REQUIRE(getsockname(s[0], (struct sockaddr *)&sin, &len) == 0);
+ ATF_REQUIRE(len == sizeof(sin));
+
+ cap_rights_init(&rights, CAP_ACCEPT, CAP_IOCTL);
+ ATF_REQUIRE(cap_rights_limit(s[0], &rights) == 0);
+ ATF_REQUIRE(cap_ioctls_limit(s[0], cmds, nitems(cmds)) == 0);
+
+ pid = fork();
+ if (pid == 0) {
+ s[1] = accept(s[0], NULL, NULL);
+ CHILD_REQUIRE(s[1] > 0);
+
+ /* Close both sockets during exit(). */
+ exit(0);
+ }
+
+ ATF_REQUIRE(pid > 0);
+
+ ATF_REQUIRE(close(s[0]) == 0);
+ s[1] = socket(AF_INET, SOCK_STREAM, 0);
+ ATF_REQUIRE(s[1] > 0);
+ ATF_REQUIRE(connect(s[1], (struct sockaddr *)&sin, sizeof(sin)) == 0);
+ ATF_REQUIRE(read(s[1], &dummy, sizeof(dummy)) == 0);
+ ATF_REQUIRE(close(s[1]) == 0);
+
+ ATF_REQUIRE(wait(&status) == pid);
+ ATF_REQUIRE(WIFEXITED(status));
+ ATF_REQUIRE(WEXITSTATUS(status) == 0);
+}
+
+ATF_TP_ADD_TCS(tp)
+{
+
+ ATF_TP_ADD_TC(tp, cap_ioctls__listen_copy);
+
+ return (atf_no_error());
+}
More information about the svn-src-all
mailing list