From owner-svn-src-all@freebsd.org Fri Apr 27 18:07:32 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2F8BDFAEC76; Fri, 27 Apr 2018 18:07:32 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CF64B7618C; Fri, 27 Apr 2018 18:07:31 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id C95CD1E79; Fri, 27 Apr 2018 18:07:31 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id w3RI7V5v046293; Fri, 27 Apr 2018 18:07:31 GMT (envelope-from jhb@FreeBSD.org) Received: (from jhb@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id w3RI7Vd8046289; Fri, 27 Apr 2018 18:07:31 GMT (envelope-from jhb@FreeBSD.org) Message-Id: <201804271807.w3RI7Vd8046289@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: jhb set sender to jhb@FreeBSD.org using -f From: John Baldwin Date: Fri, 27 Apr 2018 18:07:31 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r333069 - in stable/11: etc/mtree sys/kern sys/sys tests/sys tests/sys/capsicum X-SVN-Group: stable-11 X-SVN-Commit-Author: jhb X-SVN-Commit-Paths: in stable/11: etc/mtree sys/kern sys/sys tests/sys tests/sys/capsicum X-SVN-Commit-Revision: 333069 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Apr 2018 18:07:32 -0000 Author: jhb Date: Fri Apr 27 18:07:31 2018 New Revision: 333069 URL: https://svnweb.freebsd.org/changeset/base/333069 Log: MFC 332657: 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. Added: stable/11/tests/sys/capsicum/ - copied from r332657, head/tests/sys/capsicum/ Modified: stable/11/etc/mtree/BSD.tests.dist stable/11/sys/kern/kern_descrip.c stable/11/sys/sys/filedesc.h stable/11/tests/sys/Makefile Directory Properties: stable/11/ (props changed) Modified: stable/11/etc/mtree/BSD.tests.dist ============================================================================== --- stable/11/etc/mtree/BSD.tests.dist Fri Apr 27 17:20:23 2018 (r333068) +++ stable/11/etc/mtree/BSD.tests.dist Fri Apr 27 18:07:31 2018 (r333069) @@ -420,6 +420,8 @@ .. aio .. + capsicum + .. fifo .. file Modified: stable/11/sys/kern/kern_descrip.c ============================================================================== --- stable/11/sys/kern/kern_descrip.c Fri Apr 27 17:20:23 2018 (r333068) +++ stable/11/sys/kern/kern_descrip.c Fri Apr 27 18:07:31 2018 (r333069) @@ -1446,16 +1446,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)); @@ -1463,7 +1463,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); } /* Modified: stable/11/sys/sys/filedesc.h ============================================================================== --- stable/11/sys/sys/filedesc.h Fri Apr 27 17:20:23 2018 (r333068) +++ stable/11/sys/sys/filedesc.h Fri Apr 27 18:07:31 2018 (r333069) @@ -153,7 +153,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: stable/11/tests/sys/Makefile ============================================================================== --- stable/11/tests/sys/Makefile Fri Apr 27 17:20:23 2018 (r333068) +++ stable/11/tests/sys/Makefile Fri Apr 27 18:07:31 2018 (r333069) @@ -4,6 +4,7 @@ TESTSDIR= ${TESTSBASE}/sys TESTS_SUBDIRS+= acl TESTS_SUBDIRS+= aio +TESTS_SUBDIRS+= capsicum TESTS_SUBDIRS+= fifo TESTS_SUBDIRS+= file TESTS_SUBDIRS+= fs