Introduce kmem_scnprintf()

`snprintf()` is meant to protect against buffer overflows, but operating
on the buffer using its return value, possibly by calling it again, can
cause a buffer overflow, because it will return how many characters it
would have written if it had enough space even when it did not. In a
number of places, we repeatedly call snprintf() by successively
incrementing a buffer offset and decrementing a buffer length, by its
return value. This is a potentially unsafe usage of `snprintf()`
whenever the buffer length is reached. CodeQL complained about this.

To fix this, we introduce `kmem_scnprintf()`, which will return 0 when
the buffer is zero or the number of written characters, minus 1 to
exclude the NULL character, when the buffer was too small. In all other
cases, it behaves like snprintf(). The name is inspired by the Linux and
XNU kernels' `scnprintf()`. The implementation was written before I
thought to look at `scnprintf()` and had a good name for it, but it
turned out to have identical semantics to the Linux kernel version.
That lead to the name, `kmem_scnprintf()`.

CodeQL only catches this issue in loops, so repeated use of snprintf()
outside of a loop was not caught. As a result, a thorough audit of the
codebase was done to examine all instances of `snprintf()` usage for
potential problems and a few were caught. Fixes for them are included in
this patch.

Unfortunately, ZED is one of the places where `snprintf()` is
potentially used incorrectly. Since using `kmem_scnprintf()` in it would
require changing how it is linked, we modify its usage to make it safe,
no matter what buffer length is used. In addition, there was a bug in
the use of the return value where the NULL format character was not
being written by pwrite(). That has been fixed.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
Closes openzfs#14098

Author: ryao

cmd/zed/zed_event.c

@@ -139,8 +139,12 @@ _bump_event_queue_length(void)
 	if (qlen == orig_qlen)
 		goto done;
 	wr = snprintf(qlen_buf, sizeof (qlen_buf), "%ld", qlen);
+	if (wr >= sizeof (qlen_buf)) {
+		wr = sizeof (qlen_buf) - 1;
+		zed_log_msg(LOG_WARNING, "Truncation in %s()", __func__);
+	}
 
-	if (pwrite(zzlm, qlen_buf, wr, 0) < 0)
+	if (pwrite(zzlm, qlen_buf, wr + 1, 0) < 0)
 		goto done;
 
 	zed_log_msg(LOG_WARNING, "Bumping queue length to %ld", qlen);

include/os/freebsd/spl/sys/kmem.h

@@ -57,6 +57,9 @@ extern char	*kmem_asprintf(const char *, ...)
 extern char *kmem_vasprintf(const char *fmt, va_list ap)
     __attribute__((format(printf, 1, 0)));
 
+extern int kmem_scnprintf(char *restrict str, size_t size,
+    const char *restrict fmt, ...);
+
 typedef struct kmem_cache {
 	char		kc_name[32];
 #if !defined(KMEM_DEBUG)

include/os/linux/spl/sys/kmem.h

@@ -38,6 +38,8 @@ extern char *kmem_asprintf(const char *fmt, ...)
 extern char *kmem_strdup(const char *str);
 extern void kmem_strfree(char *str);
 
+#define	kmem_scnprintf	scnprintf
+
 /*
  * Memory allocation interfaces
  */

include/sys/spa.h

@@ -600,7 +600,7 @@ typedef struct blkptr {
 
 /*
  * This macro allows code sharing between zfs, libzpool, and mdb.
- * 'func' is either snprintf() or mdb_snprintf().
+ * 'func' is either kmem_scnprintf() or mdb_snprintf().
  * 'ws' (whitespace) can be ' ' for single-line format, '\n' for multi-line.
  */
 

include/sys/zfs_context.h

@@ -695,6 +695,9 @@ extern char *kmem_asprintf(const char *fmt, ...);
 #define	kmem_strfree(str) kmem_free((str), strlen(str) + 1)
 #define	kmem_strdup(s)  strdup(s)
 
+extern int kmem_scnprintf(char *restrict str, size_t size,
+    const char *restrict fmt, ...);
+
 /*
  * Hostname information
  */

lib/libspl/os/linux/zone.c

@@ -41,7 +41,7 @@ getzoneid(void)
 
 	int c = snprintf(path, sizeof (path), "/proc/self/ns/user");
 	/* This API doesn't have any error checking... */
-	if (c < 0)
+	if (c < 0 || c >= sizeof (path))
 		return (0);
 
 	ssize_t r = readlink(path, buf, sizeof (buf) - 1);

lib/libzfs/os/freebsd/libzfs_compat.c

@@ -202,7 +202,7 @@ libzfs_error_init(int error)
 	size_t msglen = sizeof (errbuf);
 
 	if (modfind("zfs") < 0) {
-		size_t len = snprintf(msg, msglen, dgettext(TEXT_DOMAIN,
+		size_t len = kmem_scnprintf(msg, msglen, dgettext(TEXT_DOMAIN,
 		    "Failed to load %s module: "), ZFS_KMOD);
 		msg += len;
 		msglen -= len;

lib/libzpool/kernel.c

@@ -956,6 +956,35 @@ kmem_asprintf(const char *fmt, ...)
 	return (buf);
 }
 
+/*
+ * kmem_scnprintf() will return the number of characters that it would have
+ * printed whenever it is limited by value of the size variable, rather than
+ * the number of characters that it did print. This can cause misbehavior on
+ * subsequent uses of the return value, so we define a safe version that will
+ * return the number of characters actually printed, minus the NULL format
+ * character.  Subsequent use of this by the safe string functions is safe
+ * whether it is snprintf(), strlcat() or strlcpy().
+ */
+int
+kmem_scnprintf(char *restrict str, size_t size, const char *restrict fmt, ...)
+{
+	int n;
+	va_list ap;
+
+	/* Make the 0 case a no-op so that we do not return -1 */
+	if (size == 0)
+		return (0);
+
+	va_start(ap, fmt);
+	n = vsnprintf(str, size, fmt, ap);
+	va_end(ap);
+
+	if (n >= size)
+		n = size - 1;
+
+	return (n);
+}
+
 zfs_file_t *
 zfs_onexit_fd_hold(int fd, minor_t *minorp)
 {

module/os/freebsd/spl/spl_string.c

@@ -105,3 +105,33 @@ kmem_strfree(char *str)
 	ASSERT3P(str, !=, NULL);
 	kmem_free(str, strlen(str) + 1);
 }
+
+/*
+ * kmem_scnprintf() will return the number of characters that it would have
+ * printed whenever it is limited by value of the size variable, rather than
+ * the number of characters that it did print. This can cause misbehavior on
+ * subsequent uses of the return value, so we define a safe version that will
+ * return the number of characters actually printed, minus the NULL format
+ * character.  Subsequent use of this by the safe string functions is safe
+ * whether it is snprintf(), strlcat() or strlcpy().
+ */
+
+int
+kmem_scnprintf(char *restrict str, size_t size, const char *restrict fmt, ...)
+{
+	int n;
+	va_list ap;
+
+	/* Make the 0 case a no-op so that we do not return -1 */
+	if (size == 0)
+		return (0);
+
+	va_start(ap, fmt);
+	n = vsnprintf(str, size, fmt, ap);
+	va_end(ap);
+
+	if (n >= size)
+		n = size - 1;
+
+	return (n);
+}

module/os/linux/zfs/zfs_sysfs.c

@@ -279,11 +279,11 @@ zprop_sysfs_show(const char *attr_name, const zprop_desc_t *property,
 
 		for (int i = 0; i < ARRAY_SIZE(type_map); i++) {
 			if (type_map[i].ztm_type & property->pd_types)  {
-				len += snprintf(buf + len, buflen - len, "%s ",
-				    type_map[i].ztm_name);
+				len += kmem_scnprintf(buf + len, buflen - len,
+				    "%s ", type_map[i].ztm_name);
 			}
 		}
-		len += snprintf(buf + len, buflen - len, "\n");
+		len += kmem_scnprintf(buf + len, buflen - len, "\n");
 		return (len);
 	}
 

module/zfs/dataset_kstats.c

@@ -128,8 +128,13 @@ dataset_kstats_create(dataset_kstats_t *dk, objset_t *objset)
 		    " snprintf() for kstat name returned %d",
 		    (unsigned long long)dmu_objset_id(objset), n);
 		return (SET_ERROR(EINVAL));
+	} else if (n >= KSTAT_STRLEN) {
+		zfs_dbgmsg("failed to create dataset kstat for objset %lld: "
+		    "kstat name length (%d) exceeds limit (%d)",
+		    (unsigned long long)dmu_objset_id(objset),
+		    n, KSTAT_STRLEN);
+		return (SET_ERROR(ENAMETOOLONG));
 	}
-	ASSERT3U(n, <, KSTAT_STRLEN);
 
 	kstat_t *kstat = kstat_create(kstat_module_name, 0, kstat_name,
 	    "dataset", KSTAT_TYPE_NAMED,

module/zfs/spa_misc.c

@@ -1536,7 +1536,7 @@ snprintf_blkptr(char *buf, size_t buflen, const blkptr_t *bp)
 		compress = zio_compress_table[BP_GET_COMPRESS(bp)].ci_name;
 	}
 
-	SNPRINTF_BLKPTR(snprintf, ' ', buf, buflen, bp, type, checksum,
+	SNPRINTF_BLKPTR(kmem_scnprintf, ' ', buf, buflen, bp, type, checksum,
 	    compress);
 }
 

module/zfs/vdev_raidz_math.c

@@ -285,17 +285,17 @@ raidz_math_kstat_headers(char *buf, size_t size)
 {
 	ASSERT3U(size, >=, RAIDZ_KSTAT_LINE_LEN);
 
-	ssize_t off = snprintf(buf, size, "%-17s", "implementation");
+	ssize_t off = kmem_scnprintf(buf, size, "%-17s", "implementation");
 
 	for (int i = 0; i < ARRAY_SIZE(raidz_gen_name); i++)
-		off += snprintf(buf + off, size - off, "%-16s",
+		off += kmem_scnprintf(buf + off, size - off, "%-16s",
 		    raidz_gen_name[i]);
 
 	for (int i = 0; i < ARRAY_SIZE(raidz_rec_name); i++)
-		off += snprintf(buf + off, size - off, "%-16s",
+		off += kmem_scnprintf(buf + off, size - off, "%-16s",
 		    raidz_rec_name[i]);
 
-	(void) snprintf(buf + off, size - off, "\n");
+	(void) kmem_scnprintf(buf + off, size - off, "\n");
 
 	return (0);
 }
@@ -311,34 +311,35 @@ raidz_math_kstat_data(char *buf, size_t size, void *data)
 	ASSERT3U(size, >=, RAIDZ_KSTAT_LINE_LEN);
 
 	if (cstat == fstat) {
-		off += snprintf(buf + off, size - off, "%-17s", "fastest");
+		off += kmem_scnprintf(buf + off, size - off, "%-17s",
+		    "fastest");
 
 		for (i = 0; i < ARRAY_SIZE(raidz_gen_name); i++) {
 			int id = fstat->gen[i];
-			off += snprintf(buf + off, size - off, "%-16s",
+			off += kmem_scnprintf(buf + off, size - off, "%-16s",
 			    raidz_supp_impl[id]->name);
 		}
 		for (i = 0; i < ARRAY_SIZE(raidz_rec_name); i++) {
 			int id = fstat->rec[i];
-			off += snprintf(buf + off, size - off, "%-16s",
+			off += kmem_scnprintf(buf + off, size - off, "%-16s",
 			    raidz_supp_impl[id]->name);
 		}
 	} else {
 		ptrdiff_t id = cstat - raidz_impl_kstats;
 
-		off += snprintf(buf + off, size - off, "%-17s",
+		off += kmem_scnprintf(buf + off, size - off, "%-17s",
 		    raidz_supp_impl[id]->name);
 
 		for (i = 0; i < ARRAY_SIZE(raidz_gen_name); i++)
-			off += snprintf(buf + off, size - off, "%-16llu",
+			off += kmem_scnprintf(buf + off, size - off, "%-16llu",
 			    (u_longlong_t)cstat->gen[i]);
 
 		for (i = 0; i < ARRAY_SIZE(raidz_rec_name); i++)
-			off += snprintf(buf + off, size - off, "%-16llu",
+			off += kmem_scnprintf(buf + off, size - off, "%-16llu",
 			    (u_longlong_t)cstat->rec[i]);
 	}
 
-	(void) snprintf(buf + off, size - off, "\n");
+	(void) kmem_scnprintf(buf + off, size - off, "\n");
 
 	return (0);
 }

module/zfs/zfs_chksum.c

@@ -81,15 +81,15 @@ chksum_kstat_headers(char *buf, size_t size)
 {
 	ssize_t off = 0;
 
-	off += snprintf(buf + off, size, "%-23s", "implementation");
-	off += snprintf(buf + off, size - off, "%8s", "1k");
-	off += snprintf(buf + off, size - off, "%8s", "4k");
-	off += snprintf(buf + off, size - off, "%8s", "16k");
-	off += snprintf(buf + off, size - off, "%8s", "64k");
-	off += snprintf(buf + off, size - off, "%8s", "256k");
-	off += snprintf(buf + off, size - off, "%8s", "1m");
-	off += snprintf(buf + off, size - off, "%8s", "4m");
-	(void) snprintf(buf + off, size - off, "%8s\n", "16m");
+	off += kmem_scnprintf(buf + off, size, "%-23s", "implementation");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "1k");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "4k");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "16k");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "64k");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "256k");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "1m");
+	off += kmem_scnprintf(buf + off, size - off, "%8s", "4m");
+	(void) kmem_scnprintf(buf + off, size - off, "%8s\n", "16m");
 
 	return (0);
 }
@@ -102,23 +102,23 @@ chksum_kstat_data(char *buf, size_t size, void *data)
 	char b[24];
 
 	cs = (chksum_stat_t *)data;
-	snprintf(b, 23, "%s-%s", cs->name, cs->impl);
-	off += snprintf(buf + off, size - off, "%-23s", b);
-	off += snprintf(buf + off, size - off, "%8llu",
+	kmem_scnprintf(b, 23, "%s-%s", cs->name, cs->impl);
+	off += kmem_scnprintf(buf + off, size - off, "%-23s", b);
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs1k);
-	off += snprintf(buf + off, size - off, "%8llu",
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs4k);
-	off += snprintf(buf + off, size - off, "%8llu",
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs16k);
-	off += snprintf(buf + off, size - off, "%8llu",
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs64k);
-	off += snprintf(buf + off, size - off, "%8llu",
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs256k);
-	off += snprintf(buf + off, size - off, "%8llu",
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs1m);
-	off += snprintf(buf + off, size - off, "%8llu",
+	off += kmem_scnprintf(buf + off, size - off, "%8llu",
 	    (u_longlong_t)cs->bs4m);
-	(void) snprintf(buf + off, size - off, "%8llu\n",
+	(void) kmem_scnprintf(buf + off, size - off, "%8llu\n",
 	    (u_longlong_t)cs->bs16m);
 
 	return (0);