From owner-p4-projects Mon Sep 16 13:10:11 2002 Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id B594837B401; Mon, 16 Sep 2002 13:10:01 -0700 (PDT) Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2CD9737B400 for ; Mon, 16 Sep 2002 13:10:01 -0700 (PDT) Received: from freefall.freebsd.org (freefall.FreeBSD.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id CC2E643E6E for ; Mon, 16 Sep 2002 13:10:00 -0700 (PDT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from freefall.freebsd.org (perforce@localhost [127.0.0.1]) by freefall.freebsd.org (8.12.4/8.12.4) with ESMTP id g8GKA0JU060338 for ; Mon, 16 Sep 2002 13:10:00 -0700 (PDT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: (from perforce@localhost) by freefall.freebsd.org (8.12.4/8.12.4/Submit) id g8GKA0SU060334 for perforce@freebsd.org; Mon, 16 Sep 2002 13:10:00 -0700 (PDT) Date: Mon, 16 Sep 2002 13:10:00 -0700 (PDT) Message-Id: <200209162010.g8GKA0SU060334@freefall.freebsd.org> X-Authentication-Warning: freefall.freebsd.org: perforce set sender to bb+lists.freebsd.perforce@cyrus.watson.org using -f From: Robert Watson Subject: PERFORCE change 17585 for review To: Perforce Change Reviews Sender: owner-p4-projects@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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