svn commit: r253887 - head/sys/dev/filemon

[Jilles Tjoelker]

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