PERFORCE change 17585 for review

[Robert Watson]

http://people.freebsd.org/~peter/p4db/chv.cgi?CH=17585

Change 17585 by rwatson at rwatson_tislabs on 2002/09/16 13:09:17

	Re-work pipes and locking a bit for the MAC framework:
	
	(1) In any MAC check call for the pipe code, assert that
	    the pipe mutex is held.  We use the pipe mutex to protect
	    the label when the pipe is active, so it must be held
	    if we want consistent access to the labe.
	
	(2) Check the mac_enforce_pipe flag for all pipe access
	    control checks.  This permits a high-level disabling
	    of pipe access control in the MAC framework.
	
	(3) In the pipe code, there were some situations where
	    the pipe mutex was not held (pipe_stat()) but we required
	    it to be held.  If we need it but the code didn't have it,
	    grab and release the mutex in the MAC-specific code.
	
	(4) Rather than selectively grabbing the pipe mutex depending
	    on the ioctl used on a pipe, always grab it, then release
	    it if not needed.  This is required for the MAC ioctl
	    check.  Note: it's not clear to me that all the existing
	    code here is correct with regards to the locking of the
	    pipe_sigio pointer, but if it's wrong, it was broken before
	    I got here.  I'm particularly concerned about the call to
	    fgetown() in TIOCGPGRP, where we pass the sigio pointer
	    by value rather than by reference.

Affected files ...

.. //depot/projects/trustedbsd/mac/sys/kern/kern_mac.c#263 edit
.. //depot/projects/trustedbsd/mac/sys/kern/sys_pipe.c#31 edit

Differences ...

==== //depot/projects/trustedbsd/mac/sys/kern/kern_mac.c#263 (text+ko) ====

@@ -2613,6 +2613,11 @@
 {
 	int error;
 
+	PIPE_LOCK_ASSERT(pipe, MA_OWNED);
+
+	if (!mac_enforce_pipe)
+		return (0);
+
 	MAC_CHECK(check_pipe_ioctl, cred, pipe, pipe->pipe_label, cmd, data);
 
 	return (error);
@@ -2623,6 +2628,11 @@
 {
 	int error;
 
+	PIPE_LOCK_ASSERT(pipe, MA_OWNED);
+
+	if (!mac_enforce_pipe)
+		return (0);
+
 	MAC_CHECK(check_pipe_poll, cred, pipe, pipe->pipe_label);
 
 	return (error);
@@ -2633,6 +2643,11 @@
 {
 	int error;
 
+	PIPE_LOCK_ASSERT(pipe, MA_OWNED);
+
+	if (!mac_enforce_pipe)
+		return (0);
+
 	MAC_CHECK(check_pipe_read, cred, pipe, pipe->pipe_label);
 
 	return (error);
@@ -2644,6 +2659,11 @@
 {
 	int error;
 
+	PIPE_LOCK_ASSERT(pipe, MA_OWNED);
+
+	if (!mac_enforce_pipe)
+		return (0);
+
 	MAC_CHECK(check_pipe_relabel, cred, pipe, pipe->pipe_label, newlabel);
 
 	return (error);
@@ -2654,6 +2674,11 @@
 {
 	int error;
 
+	PIPE_LOCK_ASSERT(pipe, MA_OWNED);
+
+	if (!mac_enforce_pipe)
+		return (0);
+
 	MAC_CHECK(check_pipe_stat, cred, pipe, pipe->pipe_label);
 
 	return (error);
@@ -2664,6 +2689,11 @@
 {
 	int error;
 
+	PIPE_LOCK_ASSERT(pipe, MA_OWNED);
+
+	if (!mac_enforce_pipe)
+		return (0);
+
 	MAC_CHECK(check_pipe_write, cred, pipe, pipe->pipe_label);
 
 	return (error);

==== //depot/projects/trustedbsd/mac/sys/kern/sys_pipe.c#31 (text+ko) ====

@@ -1165,8 +1165,11 @@
 	struct pipe *mpipe = (struct pipe *)fp->f_data;
 #ifdef MAC
 	int error;
+#endif
+
+	PIPE_LOCK(mpipe);
 
-	/* XXXMAC: Pipe should be locked for this check. */
+#ifdef MAC
 	error = mac_check_pipe_ioctl(active_cred, mpipe, cmd, data);
 	if (error)
 		return (error);
@@ -1175,10 +1178,10 @@
 	switch (cmd) {
 
 	case FIONBIO:
+		PIPE_UNLOCK(mpipe);
 		return (0);
 
 	case FIOASYNC:
-		PIPE_LOCK(mpipe);
 		if (*(int *)data) {
 			mpipe->pipe_state |= PIPE_ASYNC;
 		} else {
@@ -1188,7 +1191,6 @@
 		return (0);
 
 	case FIONREAD:
-		PIPE_LOCK(mpipe);
 		if (mpipe->pipe_state & PIPE_DIRECTW)
 			*(int *)data = mpipe->pipe_map.cnt;
 		else
@@ -1197,22 +1199,27 @@
 		return (0);
 
 	case FIOSETOWN:
+		PIPE_UNLOCK(mpipe);
 		return (fsetown(*(int *)data, &mpipe->pipe_sigio));
 
 	case FIOGETOWN:
+		PIPE_UNLOCK(mpipe);
 		*(int *)data = fgetown(mpipe->pipe_sigio);
 		return (0);
 
 	/* This is deprecated, FIOSETOWN should be used instead. */
 	case TIOCSPGRP:
+		PIPE_UNLOCK(mpipe);
 		return (fsetown(-(*(int *)data), &mpipe->pipe_sigio));
 
 	/* This is deprecated, FIOGETOWN should be used instead. */
 	case TIOCGPGRP:
+		PIPE_UNLOCK(mpipe);
 		*(int *)data = -fgetown(mpipe->pipe_sigio);
 		return (0);
 
 	}
+	PIPE_UNLOCK(mpipe);
 	return (ENOTTY);
 }
 
@@ -1288,8 +1295,9 @@
 #ifdef MAC
 	int error;
 
-	/* XXXMAC: Pipe should be locked for this check. */
+	PIPE_LOCK(pipe);
 	error = mac_check_pipe_stat(active_cred, pipe);
+	PIPE_UNLOCK(pipe);
 	if (error)
 		return (error);
 #endif
To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message