Date: Mon, 16 Sep 2002 13:10:00 -0700 (PDT) From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 17585 for review Message-ID: <200209162010.g8GKA0SU060334@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://people.freebsd.org/~peter/p4db/chv.cgi?CH=17585 Change 17585 by rwatson@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@FreeBSD.org with "unsubscribe p4-projects" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200209162010.g8GKA0SU060334>