svn commit: r333069 - in stable/11: etc/mtree sys/kern sys/sys tests/sys tests/sys/capsicum

[John Baldwin]

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