Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 3 Sep 2015 20:32:10 +0000 (UTC)
From:      "Conrad E. Meyer" <cem@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r287442 - in head: lib/libprocstat lib/libutil share/man/man5 sys/kern sys/sys
Message-ID:  <201509032032.t83KWAtl043698@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: cem
Date: Thu Sep  3 20:32:10 2015
New Revision: 287442
URL: https://svnweb.freebsd.org/changeset/base/287442

Log:
  Detect badly behaved coredump note helpers
  
  Coredump notes depend on being able to invoke dump routines twice; once
  in a dry-run mode to get the size of the note, and another to actually
  emit the note to the corefile.
  
  When a note helper emits a different length section the second time
  around than the length it requested the first time, the kernel produces
  a corrupt coredump.
  
  NT_PROCSTAT_FILES output length, when packing kinfo structs, is tied to
  the length of filenames corresponding to vnodes in the process' fd table
  via vn_fullpath.  As vnodes may move around during dump, this is racy.
  
  So:
  
   - Detect badly behaved notes in putnote() and pad underfilled notes.
  
   - Add a fail point, debug.fail_point.fill_kinfo_vnode__random_path to
     exercise the NT_PROCSTAT_FILES corruption.  It simply picks random
     lengths to expand or truncate paths to in fo_fill_kinfo_vnode().
  
   - Add a sysctl, kern.coredump_pack_fileinfo, to allow users to
     disable kinfo packing for PROCSTAT_FILES notes.  This should avoid
     both FILES note corruption and truncation, even if filenames change,
     at the cost of about 1 kiB in padding bloat per open fd.  Document
     the new sysctl in core.5.
  
   - Fix note_procstat_files to self-limit in the 2nd pass.  Since
     sometimes this will result in a short write, pad up to our advertised
     size.  This addresses note corruption, at the risk of sometimes
     truncating the last several fd info entries.
  
   - Fix NT_PROCSTAT_FILES consumers libutil and libprocstat to grok the
     zero padding.
  
  With suggestions from:	bjk, jhb, kib, wblock
  Approved by:	markj (mentor)
  Relnotes:	yes
  Sponsored by:	EMC / Isilon Storage Division
  Differential Revision:	https://reviews.freebsd.org/D3548

Modified:
  head/lib/libprocstat/libprocstat.c
  head/lib/libutil/kinfo_getfile.c
  head/share/man/man5/core.5
  head/sys/kern/imgact_elf.c
  head/sys/kern/kern_descrip.c
  head/sys/kern/vfs_vnops.c
  head/sys/sys/user.h

Modified: head/lib/libprocstat/libprocstat.c
==============================================================================
--- head/lib/libprocstat/libprocstat.c	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/lib/libprocstat/libprocstat.c	Thu Sep  3 20:32:10 2015	(r287442)
@@ -767,6 +767,8 @@ kinfo_getfile_core(struct procstat_core 
 	eb = buf + len;
 	while (bp < eb) {
 		kf = (struct kinfo_file *)(uintptr_t)bp;
+		if (kf->kf_structsize == 0)
+			break;
 		bp += kf->kf_structsize;
 		cnt++;
 	}
@@ -782,6 +784,8 @@ kinfo_getfile_core(struct procstat_core 
 	/* Pass 2: unpack */
 	while (bp < eb) {
 		kf = (struct kinfo_file *)(uintptr_t)bp;
+		if (kf->kf_structsize == 0)
+			break;
 		/* Copy/expand into pre-zeroed buffer */
 		memcpy(kp, kf, kf->kf_structsize);
 		/* Advance to next packed record */

Modified: head/lib/libutil/kinfo_getfile.c
==============================================================================
--- head/lib/libutil/kinfo_getfile.c	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/lib/libutil/kinfo_getfile.c	Thu Sep  3 20:32:10 2015	(r287442)
@@ -44,6 +44,8 @@ kinfo_getfile(pid_t pid, int *cntp)
 	eb = buf + len;
 	while (bp < eb) {
 		kf = (struct kinfo_file *)(uintptr_t)bp;
+		if (kf->kf_structsize == 0)
+			break;
 		bp += kf->kf_structsize;
 		cnt++;
 	}
@@ -59,6 +61,8 @@ kinfo_getfile(pid_t pid, int *cntp)
 	/* Pass 2: unpack */
 	while (bp < eb) {
 		kf = (struct kinfo_file *)(uintptr_t)bp;
+		if (kf->kf_structsize == 0)
+			break;
 		/* Copy/expand into pre-zeroed buffer */
 		memcpy(kp, kf, kf->kf_structsize);
 		/* Advance to next packed record */

Modified: head/share/man/man5/core.5
==============================================================================
--- head/share/man/man5/core.5	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/share/man/man5/core.5	Thu Sep  3 20:32:10 2015	(r287442)
@@ -28,7 +28,7 @@
 .\"     @(#)core.5	8.3 (Berkeley) 12/11/93
 .\" $FreeBSD$
 .\"
-.Dd March 8, 2015
+.Dd September 2, 2015
 .Dt CORE 5
 .Os
 .Sh NAME
@@ -120,6 +120,16 @@ Compressed core files will have a suffix
 .Ql .gz
 appended to them.
 .El
+.Sh NOTES
+Corefiles are written with open file descriptor information as an ELF note.
+By default, file paths are packed to only use as much space as needed.
+However, file paths can change at any time, including during core dump,
+and this can result in truncated file descriptor data.
+.Pp
+All file descriptor information can be preserved by disabling packing.
+This potentially wastes up to PATH_MAX bytes per open fd.
+Packing is disabled with
+.Dl sysctl kern.coredump_pack_fileinfo=0 .
 .Sh EXAMPLES
 In order to store all core images in per-user private areas under
 .Pa /var/coredumps ,

Modified: head/sys/kern/imgact_elf.c
==============================================================================
--- head/sys/kern/imgact_elf.c	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/sys/kern/imgact_elf.c	Thu Sep  3 20:32:10 2015	(r287442)
@@ -1677,7 +1677,8 @@ static void
 __elfN(putnote)(struct note_info *ninfo, struct sbuf *sb)
 {
 	Elf_Note note;
-	ssize_t old_len;
+	ssize_t old_len, sect_len;
+	size_t new_len, descsz, i;
 
 	if (ninfo->type == -1) {
 		ninfo->outfunc(ninfo->outarg, sb, &ninfo->outsize);
@@ -1696,7 +1697,33 @@ __elfN(putnote)(struct note_info *ninfo,
 		return;
 	sbuf_start_section(sb, &old_len);
 	ninfo->outfunc(ninfo->outarg, sb, &ninfo->outsize);
-	sbuf_end_section(sb, old_len, ELF_NOTE_ROUNDSIZE, 0);
+	sect_len = sbuf_end_section(sb, old_len, ELF_NOTE_ROUNDSIZE, 0);
+	if (sect_len < 0)
+		return;
+
+	new_len = (size_t)sect_len;
+	descsz = roundup(note.n_descsz, ELF_NOTE_ROUNDSIZE);
+	if (new_len < descsz) {
+		/*
+		 * It is expected that individual note emitters will correctly
+		 * predict their expected output size and fill up to that size
+		 * themselves, padding in a format-specific way if needed.
+		 * However, in case they don't, just do it here with zeros.
+		 */
+		for (i = 0; i < descsz - new_len; i++)
+			sbuf_putc(sb, 0);
+	} else if (new_len > descsz) {
+		/*
+		 * We can't always truncate sb -- we may have drained some
+		 * of it already.
+		 */
+		KASSERT(new_len == descsz, ("%s: Note type %u changed as we "
+		    "read it (%zu > %zu).  Since it is longer than "
+		    "expected, this coredump's notes are corrupt.  THIS "
+		    "IS A BUG in the note_procstat routine for type %u.\n",
+		    __func__, (unsigned)note.n_type, new_len, descsz,
+		    (unsigned)note.n_type));
+	}
 }
 
 /*
@@ -1875,29 +1902,56 @@ __elfN(note_procstat_proc)(void *arg, st
 CTASSERT(sizeof(struct kinfo_file) == KINFO_FILE_SIZE);
 #endif
 
+static int pack_fileinfo = 1;
+SYSCTL_INT(_kern, OID_AUTO, coredump_pack_fileinfo, CTLFLAG_RWTUN,
+    &pack_fileinfo, 0,
+    "Enable file path packing in 'procstat -f' coredump notes");
+
 static void
 note_procstat_files(void *arg, struct sbuf *sb, size_t *sizep)
 {
 	struct proc *p;
-	size_t size;
-	int structsize;
+	size_t size, sect_sz, i;
+	ssize_t start_len, sect_len;
+	int structsize, filedesc_flags;
+
+	if (pack_fileinfo)
+		filedesc_flags = KERN_FILEDESC_PACK_KINFO;
+	else
+		filedesc_flags = 0;
 
 	p = (struct proc *)arg;
+	structsize = sizeof(struct kinfo_file);
 	if (sb == NULL) {
 		size = 0;
 		sb = sbuf_new(NULL, NULL, 128, SBUF_FIXEDLEN);
 		sbuf_set_drain(sb, sbuf_drain_count, &size);
 		sbuf_bcat(sb, &structsize, sizeof(structsize));
 		PROC_LOCK(p);
-		kern_proc_filedesc_out(p, sb, -1);
+		kern_proc_filedesc_out(p, sb, -1, filedesc_flags);
 		sbuf_finish(sb);
 		sbuf_delete(sb);
 		*sizep = size;
 	} else {
-		structsize = sizeof(struct kinfo_file);
+		sbuf_start_section(sb, &start_len);
+
 		sbuf_bcat(sb, &structsize, sizeof(structsize));
 		PROC_LOCK(p);
-		kern_proc_filedesc_out(p, sb, -1);
+		kern_proc_filedesc_out(p, sb, *sizep - sizeof(structsize),
+		    filedesc_flags);
+
+		sect_len = sbuf_end_section(sb, start_len, 0, 0);
+		if (sect_len < 0)
+			return;
+		sect_sz = sect_len;
+
+		KASSERT(sect_sz <= *sizep,
+		    ("kern_proc_filedesc_out did not respect maxlen; "
+		     "requested %zu, got %zu", *sizep - sizeof(structsize),
+		     sect_sz - sizeof(structsize)));
+
+		for (i = 0; i < *sizep - sect_sz && sb->s_error == 0; i++)
+			sbuf_putc(sb, 0);
 	}
 }
 

Modified: head/sys/kern/kern_descrip.c
==============================================================================
--- head/sys/kern/kern_descrip.c	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/sys/kern/kern_descrip.c	Thu Sep  3 20:32:10 2015	(r287442)
@@ -3283,7 +3283,7 @@ pack_kinfo(struct kinfo_file *kif)
 
 static void
 export_file_to_kinfo(struct file *fp, int fd, cap_rights_t *rightsp,
-    struct kinfo_file *kif, struct filedesc *fdp)
+    struct kinfo_file *kif, struct filedesc *fdp, int flags)
 {
 	int error;
 
@@ -3307,12 +3307,15 @@ export_file_to_kinfo(struct file *fp, in
 	error = fo_fill_kinfo(fp, kif, fdp);
 	if (error == 0)
 		kif->kf_status |= KF_ATTR_VALID;
-	pack_kinfo(kif);
+	if ((flags & KERN_FILEDESC_PACK_KINFO) != 0)
+		pack_kinfo(kif);
+	else
+		kif->kf_structsize = roundup2(sizeof(*kif), sizeof(uint64_t));
 }
 
 static void
 export_vnode_to_kinfo(struct vnode *vp, int fd, int fflags,
-    struct kinfo_file *kif)
+    struct kinfo_file *kif, int flags)
 {
 	int error;
 
@@ -3327,7 +3330,10 @@ export_vnode_to_kinfo(struct vnode *vp, 
 	kif->kf_fd = fd;
 	kif->kf_ref_count = -1;
 	kif->kf_offset = -1;
-	pack_kinfo(kif);
+	if ((flags & KERN_FILEDESC_PACK_KINFO) != 0)
+		pack_kinfo(kif);
+	else
+		kif->kf_structsize = roundup2(sizeof(*kif), sizeof(uint64_t));
 	vrele(vp);
 }
 
@@ -3336,6 +3342,7 @@ struct export_fd_buf {
 	struct sbuf 		*sb;
 	ssize_t			remainder;
 	struct kinfo_file	kif;
+	int			flags;
 };
 
 static int
@@ -3363,7 +3370,8 @@ export_file_to_sb(struct file *fp, int f
 
 	if (efbuf->remainder == 0)
 		return (0);
-	export_file_to_kinfo(fp, fd, rightsp, &efbuf->kif, efbuf->fdp);
+	export_file_to_kinfo(fp, fd, rightsp, &efbuf->kif, efbuf->fdp,
+	    efbuf->flags);
 	FILEDESC_SUNLOCK(efbuf->fdp);
 	error = export_kinfo_to_sb(efbuf);
 	FILEDESC_SLOCK(efbuf->fdp);
@@ -3380,7 +3388,7 @@ export_vnode_to_sb(struct vnode *vp, int
 		return (0);
 	if (efbuf->fdp != NULL)
 		FILEDESC_SUNLOCK(efbuf->fdp);
-	export_vnode_to_kinfo(vp, fd, fflags, &efbuf->kif);
+	export_vnode_to_kinfo(vp, fd, fflags, &efbuf->kif, efbuf->flags);
 	error = export_kinfo_to_sb(efbuf);
 	if (efbuf->fdp != NULL)
 		FILEDESC_SLOCK(efbuf->fdp);
@@ -3393,7 +3401,8 @@ export_vnode_to_sb(struct vnode *vp, int
  * Takes a locked proc as argument, and returns with the proc unlocked.
  */
 int
-kern_proc_filedesc_out(struct proc *p,  struct sbuf *sb, ssize_t maxlen)
+kern_proc_filedesc_out(struct proc *p,  struct sbuf *sb, ssize_t maxlen,
+    int flags)
 {
 	struct file *fp;
 	struct filedesc *fdp;
@@ -3425,6 +3434,7 @@ kern_proc_filedesc_out(struct proc *p,  
 	efbuf->fdp = NULL;
 	efbuf->sb = sb;
 	efbuf->remainder = maxlen;
+	efbuf->flags = flags;
 	if (tracevp != NULL)
 		export_vnode_to_sb(tracevp, KF_FD_TYPE_TRACE, FREAD | FWRITE,
 		    efbuf);
@@ -3501,7 +3511,8 @@ sysctl_kern_proc_filedesc(SYSCTL_HANDLER
 		return (error);
 	}
 	maxlen = req->oldptr != NULL ? req->oldlen : -1;
-	error = kern_proc_filedesc_out(p, &sb, maxlen);
+	error = kern_proc_filedesc_out(p, &sb, maxlen,
+	    KERN_FILEDESC_PACK_KINFO);
 	error2 = sbuf_finish(&sb);
 	sbuf_delete(&sb);
 	return (error != 0 ? error : error2);
@@ -3541,7 +3552,7 @@ export_vnode_for_osysctl(struct vnode *v
 
 	vref(vp);
 	FILEDESC_SUNLOCK(fdp);
-	export_vnode_to_kinfo(vp, type, 0, kif);
+	export_vnode_to_kinfo(vp, type, 0, kif, KERN_FILEDESC_PACK_KINFO);
 	kinfo_to_okinfo(kif, okif);
 	error = SYSCTL_OUT(req, okif, sizeof(*okif));
 	FILEDESC_SLOCK(fdp);
@@ -3584,7 +3595,8 @@ sysctl_kern_proc_ofiledesc(SYSCTL_HANDLE
 	for (i = 0; fdp->fd_refcnt > 0 && i <= fdp->fd_lastfile; i++) {
 		if ((fp = fdp->fd_ofiles[i].fde_file) == NULL)
 			continue;
-		export_file_to_kinfo(fp, i, NULL, kif, fdp);
+		export_file_to_kinfo(fp, i, NULL, kif, fdp,
+		    KERN_FILEDESC_PACK_KINFO);
 		FILEDESC_SUNLOCK(fdp);
 		kinfo_to_okinfo(kif, okif);
 		error = SYSCTL_OUT(req, okif, sizeof(*okif));

Modified: head/sys/kern/vfs_vnops.c
==============================================================================
--- head/sys/kern/vfs_vnops.c	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/sys/kern/vfs_vnops.c	Thu Sep  3 20:32:10 2015	(r287442)
@@ -46,6 +46,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/param.h>
 #include <sys/systm.h>
 #include <sys/disk.h>
+#include <sys/fail.h>
 #include <sys/fcntl.h>
 #include <sys/file.h>
 #include <sys/kdb.h>
@@ -2379,6 +2380,24 @@ vn_fill_kinfo(struct file *fp, struct ki
 	return (error);
 }
 
+static inline void
+vn_fill_junk(struct kinfo_file *kif)
+{
+	size_t len, olen;
+
+	/*
+	 * Simulate vn_fullpath returning changing values for a given
+	 * vp during e.g. coredump.
+	 */
+	len = (arc4random() % (sizeof(kif->kf_path) - 2)) + 1;
+	olen = strlen(kif->kf_path);
+	if (len < olen)
+		strcpy(&kif->kf_path[len - 1], "$");
+	else
+		for (; olen < len; olen++)
+			strcpy(&kif->kf_path[olen], "A");
+}
+
 int
 vn_fill_kinfo_vnode(struct vnode *vp, struct kinfo_file *kif)
 {
@@ -2396,6 +2415,10 @@ vn_fill_kinfo_vnode(struct vnode *vp, st
 	if (freepath != NULL)
 		free(freepath, M_TEMP);
 
+	KFAIL_POINT_CODE(DEBUG_FP, fill_kinfo_vnode__random_path,
+		vn_fill_junk(kif);
+	);
+
 	/*
 	 * Retrieve vnode attributes.
 	 */

Modified: head/sys/sys/user.h
==============================================================================
--- head/sys/sys/user.h	Thu Sep  3 19:42:56 2015	(r287441)
+++ head/sys/sys/user.h	Thu Sep  3 20:32:10 2015	(r287442)
@@ -539,6 +539,8 @@ struct kinfo_sigtramp {
 #define KERN_PROC_NOTHREADS	0x1
 #define KERN_PROC_MASK32	0x2
 
+/* Flags for kern_proc_filedesc_out. */
+#define	KERN_FILEDESC_PACK_KINFO	0x00000001U
 struct sbuf;
 
 /*
@@ -550,7 +552,8 @@ struct sbuf;
  * to be locked on enter.  On return the process is unlocked.
  */
 
-int	kern_proc_filedesc_out(struct proc *p, struct sbuf *sb, ssize_t maxlen);
+int	kern_proc_filedesc_out(struct proc *p, struct sbuf *sb, ssize_t maxlen,
+	int flags);
 int	kern_proc_cwd_out(struct proc *p, struct sbuf *sb, ssize_t maxlen);
 int	kern_proc_out(struct proc *p, struct sbuf *sb, int flags);
 int	kern_proc_vmmap_out(struct proc *p, struct sbuf *sb);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201509032032.t83KWAtl043698>