From owner-svn-src-all@FreeBSD.ORG Fri Aug 2 15:22:23 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 7D53EB83; Fri, 2 Aug 2013 15:22:23 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (unknown [IPv6:2001:610:1108:5012::107]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 3ECDC2C30; Fri, 2 Aug 2013 15:22:23 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 0E1C41203CA; Fri, 2 Aug 2013 17:22:05 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id E399B28494; Fri, 2 Aug 2013 17:22:04 +0200 (CEST) Date: Fri, 2 Aug 2013 17:22:04 +0200 From: Jilles Tjoelker To: Hiroki Sato Subject: Re: svn commit: r253887 - head/sys/dev/filemon Message-ID: <20130802152204.GA1880@stack.nl> References: <201308021444.r72EiBk2059771@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201308021444.r72EiBk2059771@svn.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, sjg@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 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, 02 Aug 2013 15:22:23 -0000 On Fri, Aug 02, 2013 at 02:44:11PM +0000, Hiroki Sato wrote: > Author: hrs > Date: Fri Aug 2 14:44:11 2013 > New Revision: 253887 > URL: http://svnweb.freebsd.org/changeset/base/253887 > Log: > Add p_candebug() check to FILEMON_SET_PID ioctl. > Discussed with: sjg > MFC after: 3 days > Modified: > head/sys/dev/filemon/filemon.c > Modified: head/sys/dev/filemon/filemon.c > ============================================================================== > --- head/sys/dev/filemon/filemon.c Fri Aug 2 14:14:23 2013 (r253886) > +++ head/sys/dev/filemon/filemon.c Fri Aug 2 14:44:11 2013 (r253887) > @@ -150,6 +150,7 @@ filemon_ioctl(struct cdev *dev, u_long c > { > int error = 0; > struct filemon *filemon; > + struct proc *p; > > devfs_get_cdevpriv((void **) &filemon); > > @@ -163,7 +164,13 @@ filemon_ioctl(struct cdev *dev, u_long c > > /* Set the monitored process ID. */ > case FILEMON_SET_PID: > - filemon->pid = *((pid_t *)data); > + p = pfind(*((pid_t *)data)); > + if (p == NULL) > + return (EINVAL); > + error = p_candebug(curthread, p); > + if (error == 0) > + filemon->pid = p->p_pid; > + PROC_UNLOCK(p); > break; > > default: You can simplify the code using the fairly new pget(). This will also fix the incorrect errno when the process does not exist (should be [ESRCH]). This change is a step in the right direction but is incomplete. Although the check protects currently running processes, I do not see how it prevents tracing a process that gets the same PID again after the original target process has exited. This not only leaks sensitive information but may also prevent tracing by the legitimate owner of the process (because only one filemon will write events for a process). This could be fixed by setting filemon->pid = -1 in filemon_wrapper_sys_exit() and not allowing P_WEXIT and zombies in FILEMON_SET_PID (PGET_NOTWEXIT disallows both). An [EBUSY] when there is already a filemon monitoring the process may also be useful (or writing copies of the events to all attached filemons). -- Jilles Tjoelker