Fix some filemon path logging issues.

- Properly handle snprintf return value for truncation and avoid
  overflowing the later write with the bogus length.
- Increase the msgbufr size to handle a rename of 2 full files.

The larger allocation causes a slight performance hit which will be mitigated
in the future.  A rewrite with sbufs will likely be done as well.

Reported by:	Ilja Van Sprundel <ivansprundel@ioactive.com>
MFC after:	2 weeks
Approved by:	so (gtetlow)
Reviewed by:	kib
Sponsored by:	Dell EMC
Differential Revision:	https://reviews.freebsd.org/D16098

2 files changed, 36 insertions, 49 deletions

diff --git a/sys/dev/filemon/filemon.c b/sys/dev/filemon/filemon.c
index 80c3c28ff51a..cd48c68d8b04 100644
--- a/sys/dev/filemon/filemon.c
+++ b/sys/dev/filemon/filemon.c
@@ -88,7 +88,7 @@ struct filemon {
 	struct ucred	*cred;		/* Credential of tracer. */
 	char		fname1[MAXPATHLEN]; /* Temporary filename buffer. */
 	char		fname2[MAXPATHLEN]; /* Temporary filename buffer. */
-	char		msgbufr[1024];	/* Output message buffer. */
+	char		msgbufr[2*MAXPATHLEN + 100];	/* Output message buffer. */
 	int		error;		/* Log write error, returned on close(2). */
 	u_int		refcnt;		/* Pointer reference count. */
 	u_int		proccnt;	/* Process count. */
@@ -200,8 +200,8 @@ filemon_write_header(struct filemon *filemon)
 	    "# filemon version %d\n# Target pid %d\n# Start %ju.%06ju\nV %d\n",
 	    FILEMON_VERSION, curproc->p_pid, (uintmax_t)now.tv_sec,
 	    (uintmax_t)now.tv_usec, FILEMON_VERSION);
-
-	filemon_output(filemon, filemon->msgbufr, len);
+	if (len < sizeof(filemon->msgbufr))
+		filemon_output(filemon, filemon->msgbufr, len);
 }
 
 /*
@@ -268,7 +268,8 @@ filemon_close_log(struct filemon *filemon)
 	    "# Stop %ju.%06ju\n# Bye bye\n",
 	    (uintmax_t)now.tv_sec, (uintmax_t)now.tv_usec);
 
-	filemon_output(filemon, filemon->msgbufr, len);
+	if (len < sizeof(filemon->msgbufr))
+		filemon_output(filemon, filemon->msgbufr, len);
 	fp = filemon->fp;
 	filemon->fp = NULL;
 
diff --git a/sys/dev/filemon/filemon_wrapper.c b/sys/dev/filemon/filemon_wrapper.c
index 2b742d0dab8b..54150777ed9f 100644
--- a/sys/dev/filemon/filemon_wrapper.c
+++ b/sys/dev/filemon/filemon_wrapper.c
@@ -39,6 +39,11 @@ __FBSDID("$FreeBSD$");
 #include <sys/sysent.h>
 #include <sys/vnode.h>
 
+#include <machine/stdarg.h>
+
+static void filemon_output_event(struct filemon *filemon, const char *fmt, ...)
+    __printflike(2, 3);
+
 static eventhandler_tag filemon_exec_tag;
 static eventhandler_tag filemon_exit_tag;
 static eventhandler_tag filemon_fork_tag;
@@ -71,11 +76,25 @@ filemon_output(struct filemon *filemon, char *msg, size_t len)
 		filemon->error = error;
 }
 
+static void
+filemon_output_event(struct filemon *filemon, const char *fmt, ...)
+{
+	va_list ap;
+	size_t len;
+
+	va_start(ap, fmt);
+	len = vsnprintf(filemon->msgbufr, sizeof(filemon->msgbufr), fmt, ap);
+	va_end(ap);
+	/* The event is truncated but still worth logging. */
+	if (len >= sizeof(filemon->msgbufr))
+		len = sizeof(filemon->msgbufr) - 1;
+	filemon_output(filemon, filemon->msgbufr, len);
+}
+
 static int
 filemon_wrapper_chdir(struct thread *td, struct chdir_args *uap)
 {
 	int error, ret;
-	size_t len;
 	struct filemon *filemon;
 
 	if ((ret = sys_chdir(td, uap)) == 0) {
@@ -86,11 +105,8 @@ filemon_wrapper_chdir(struct thread *td, struct chdir_args *uap)
 				goto copyfail;
 			}
 
-			len = snprintf(filemon->msgbufr,
-			    sizeof(filemon->msgbufr), "C %d %s\n",
+			filemon_output_event(filemon, "C %d %s\n",
 			    curproc->p_pid, filemon->fname1);
-
-			filemon_output(filemon, filemon->msgbufr, len);
 copyfail:
 			filemon_drop(filemon);
 		}
@@ -104,16 +120,12 @@ filemon_event_process_exec(void *arg __unused, struct proc *p,
     struct image_params *imgp)
 {
 	struct filemon *filemon;
-	size_t len;
 
 	if ((filemon = filemon_proc_get(p)) != NULL) {
-		len = snprintf(filemon->msgbufr,
-		    sizeof(filemon->msgbufr), "E %d %s\n",
+		filemon_output_event(filemon, "E %d %s\n",
 		    p->p_pid,
 		    imgp->execpath != NULL ? imgp->execpath : "<unknown>");
 
-		filemon_output(filemon, filemon->msgbufr, len);
-
 		/* If the credentials changed then cease tracing. */
 		if (imgp->newcred != NULL &&
 		    imgp->credential_setid &&
@@ -140,7 +152,6 @@ static void
 _filemon_wrapper_openat(struct thread *td, char *upath, int flags, int fd)
 {
 	int error;
-	size_t len;
 	struct file *fp;
 	struct filemon *filemon;
 	char *atpath, *freepath;
@@ -166,10 +177,8 @@ _filemon_wrapper_openat(struct thread *td, char *upath, int flags, int fd)
 			 * XXX: This may be able to come out with
 			 * the namecache lookup now.
 			 */
-			len = snprintf(filemon->msgbufr,
-			    sizeof(filemon->msgbufr), "A %d %s\n",
+			filemon_output_event(filemon, "A %d %s\n",
 			    curproc->p_pid, filemon->fname1);
-			filemon_output(filemon, filemon->msgbufr, len);
 			/*
 			 * Try to resolve the path from the vnode using the
 			 * namecache.  It may be inaccurate, but better
@@ -187,19 +196,15 @@ _filemon_wrapper_openat(struct thread *td, char *upath, int flags, int fd)
 			 * to also output an R to distinguish from
 			 * O_WRONLY.
 			 */
-			len = snprintf(filemon->msgbufr,
-			    sizeof(filemon->msgbufr), "R %d %s%s%s\n",
+			filemon_output_event(filemon, "R %d %s%s%s\n",
 			    curproc->p_pid, atpath,
 			    atpath[0] != '\0' ? "/" : "", filemon->fname1);
-			filemon_output(filemon, filemon->msgbufr, len);
 		}
 
-		len = snprintf(filemon->msgbufr,
-		    sizeof(filemon->msgbufr), "%c %d %s%s%s\n",
+		filemon_output_event(filemon, "%c %d %s%s%s\n",
 		    (flags & O_ACCMODE) ? 'W':'R',
 		    curproc->p_pid, atpath,
 		    atpath[0] != '\0' ? "/" : "", filemon->fname1);
-		filemon_output(filemon, filemon->msgbufr, len);
 copyfail:
 		filemon_drop(filemon);
 		if (fp != NULL)
@@ -234,7 +239,6 @@ static int
 filemon_wrapper_rename(struct thread *td, struct rename_args *uap)
 {
 	int error, ret;
-	size_t len;
 	struct filemon *filemon;
 
 	if ((ret = sys_rename(td, uap)) == 0) {
@@ -247,11 +251,8 @@ filemon_wrapper_rename(struct thread *td, struct rename_args *uap)
 				goto copyfail;
 			}
 
-			len = snprintf(filemon->msgbufr,
-			    sizeof(filemon->msgbufr), "M %d '%s' '%s'\n",
+			filemon_output_event(filemon, "M %d '%s' '%s'\n",
 			    curproc->p_pid, filemon->fname1, filemon->fname2);
-
-			filemon_output(filemon, filemon->msgbufr, len);
 copyfail:
 			filemon_drop(filemon);
 		}
@@ -264,7 +265,6 @@ static void
 _filemon_wrapper_link(struct thread *td, char *upath1, char *upath2)
 {
 	struct filemon *filemon;
-	size_t len;
 	int error;
 
 	if ((filemon = filemon_proc_get(curproc)) != NULL) {
@@ -276,11 +276,8 @@ _filemon_wrapper_link(struct thread *td, char *upath1, char *upath2)
 			goto copyfail;
 		}
 
-		len = snprintf(filemon->msgbufr,
-		    sizeof(filemon->msgbufr), "L %d '%s' '%s'\n",
+		filemon_output_event(filemon, "L %d '%s' '%s'\n",
 		    curproc->p_pid, filemon->fname1, filemon->fname2);
-
-		filemon_output(filemon, filemon->msgbufr, len);
 copyfail:
 		filemon_drop(filemon);
 	}
@@ -322,14 +319,11 @@ filemon_wrapper_linkat(struct thread *td, struct linkat_args *uap)
 static void
 filemon_event_process_exit(void *arg __unused, struct proc *p)
 {
-	size_t len;
 	struct filemon *filemon;
 
 	if ((filemon = filemon_proc_get(p)) != NULL) {
-		len = snprintf(filemon->msgbufr, sizeof(filemon->msgbufr),
-		    "X %d %d %d\n", p->p_pid, p->p_xexit, p->p_xsig);
-
-		filemon_output(filemon, filemon->msgbufr, len);
+		filemon_output_event(filemon, "X %d %d %d\n",
+		    p->p_pid, p->p_xexit, p->p_xsig);
 
 		/*
 		 * filemon_untrack_processes() may have dropped this p_filemon
@@ -350,7 +344,6 @@ static int
 filemon_wrapper_unlink(struct thread *td, struct unlink_args *uap)
 {
 	int error, ret;
-	size_t len;
 	struct filemon *filemon;
 
 	if ((ret = sys_unlink(td, uap)) == 0) {
@@ -361,11 +354,8 @@ filemon_wrapper_unlink(struct thread *td, struct unlink_args *uap)
 				goto copyfail;
 			}
 
-			len = snprintf(filemon->msgbufr,
-			    sizeof(filemon->msgbufr), "D %d %s\n",
+			filemon_output_event(filemon, "D %d %s\n",
 			    curproc->p_pid, filemon->fname1);
-
-			filemon_output(filemon, filemon->msgbufr, len);
 copyfail:
 			filemon_drop(filemon);
 		}
@@ -378,16 +368,12 @@ static void
 filemon_event_process_fork(void *arg __unused, struct proc *p1,
     struct proc *p2, int flags __unused)
 {
-	size_t len;
 	struct filemon *filemon;
 
 	if ((filemon = filemon_proc_get(p1)) != NULL) {
-		len = snprintf(filemon->msgbufr,
-		    sizeof(filemon->msgbufr), "F %d %d\n",
+		filemon_output_event(filemon, "F %d %d\n",
 		    p1->p_pid, p2->p_pid);
 
-		filemon_output(filemon, filemon->msgbufr, len);
-
 		/*
 		 * filemon_untrack_processes() or
 		 * filemon_ioctl(FILEMON_SET_PID) may have changed the parent's