Date: Fri, 2 Aug 2013 17:22:04 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Hiroki Sato <hrs@FreeBSD.org> Cc: svn-src-head@freebsd.org, sjg@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r253887 - head/sys/dev/filemon Message-ID: <20130802152204.GA1880@stack.nl> In-Reply-To: <201308021444.r72EiBk2059771@svn.freebsd.org> References: <201308021444.r72EiBk2059771@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130802152204.GA1880>