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 <behlendorf1@llnl.gov>
Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
Closes #14098

diff --git a/cmd/zed/zed_event.c b/cmd/zed/zed_event.c
index 079c6a043e5d25763d7560edce59390b09edfbd8..ebd6851a2a86cffbc748d97a89dcb351b6c4e14f 100644 (file)
--- a/cmd/zed/zed_event.c
+++ b/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);

diff --git a/include/os/freebsd/spl/sys/kmem.h b/include/os/freebsd/spl/sys/kmem.h
index ae2941b80912cd58e3217160702ca6fc49ee2544..27d290863c0bce8c236de0bd8efd5b511e6eb8ca 100644 (file)
--- a/include/os/freebsd/spl/sys/kmem.h
+++ b/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)

diff --git a/include/os/linux/spl/sys/kmem.h b/include/os/linux/spl/sys/kmem.h
index 86e872fa2492b170cc35e2fd8e2f6948bab9008b..111924303e169d042ac4278cc95711a5e33d5fc6 100644 (file)
--- a/include/os/linux/spl/sys/kmem.h
+++ b/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
  */

diff --git a/include/sys/spa.h b/include/sys/spa.h
index 8dcd8895ba22df44f9e867fc62f9250751d47ace..b260dd32820e5decf450f8d852d50271381cc950 100644 (file)
--- a/include/sys/spa.h
+++ b/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.
  */
 

diff --git a/include/sys/zfs_context.h b/include/sys/zfs_context.h
index d29d7118ff000135de57de9def4bfbd09368cf53..0d31195447d18d3b50a7f1e96eef7098f3f8f07e 100644 (file)
--- a/include/sys/zfs_context.h
+++ b/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
  */

diff --git a/lib/libspl/os/linux/zone.c b/lib/libspl/os/linux/zone.c
index e37b34975df6b2a97a979fd2dc7e0801ba994611..622d04cbc14a547ccdf91d5fbbea9b68352da7dd 100644 (file)
--- a/lib/libspl/os/linux/zone.c
+++ b/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);

diff --git a/lib/libzfs/os/freebsd/libzfs_compat.c b/lib/libzfs/os/freebsd/libzfs_compat.c
index f4900943c4af10e326a19c0ccdbb4e0c423e6154..5e280cbae9354fd29b93a9dc813790023d2900be 100644 (file)
--- a/lib/libzfs/os/freebsd/libzfs_compat.c
+++ b/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;

diff --git a/lib/libzpool/kernel.c b/lib/libzpool/kernel.c
index 1f58acb0404fd32f792878a15f46b57acd247e4d..1d46470947d3c88c2f020cfb34869b5a77c74c65 100644 (file)
--- a/lib/libzpool/kernel.c
+++ b/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)
 {

diff --git a/module/os/freebsd/spl/spl_string.c b/module/os/freebsd/spl/spl_string.c
index 523e10ff693602d0bcd7b09e9836dd6430dbed60..eb74720c984a6da0f405e8a6bf3df3041a3bb2ed 100644 (file)
--- a/module/os/freebsd/spl/spl_string.c
+++ b/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);
+}

diff --git a/module/os/linux/zfs/zfs_sysfs.c b/module/os/linux/zfs/zfs_sysfs.c
index 4a2091f3c39679b3560fd1fd43811d8615a5bcc2..e2431fe8a803fbb6db668b0f2d6cdcbbcd970545 100644 (file)
--- a/module/os/linux/zfs/zfs_sysfs.c
+++ b/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);
        }
 

diff --git a/module/zfs/dataset_kstats.c b/module/zfs/dataset_kstats.c
index b63f42a21e44e672ca1f11ecc2b91cee16d1e51a..57b8faf213eb612e00d5c49e18e2521a5ac5db38 100644 (file)
--- a/module/zfs/dataset_kstats.c
+++ b/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,

diff --git a/module/zfs/spa_misc.c b/module/zfs/spa_misc.c
index 9ec37502986f2e4d269cd0c4216c6a4bda6ebd80..ca55d55405d33d8d4f12a5dbaad8329ef4672a67 100644 (file)
--- a/module/zfs/spa_misc.c
+++ b/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);
 }
 

diff --git a/module/zfs/vdev_raidz_math.c b/module/zfs/vdev_raidz_math.c
index f74a76a8d5ba82924d63cba77563f2160db916a5..2980f8acfbd7f953173c0e865cf106fd2f3fd86a 100644 (file)
--- a/module/zfs/vdev_raidz_math.c
+++ b/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);
 }

diff --git a/module/zfs/zfs_chksum.c b/module/zfs/zfs_chksum.c
index 74b4cb8d2e63a9634acff1110f2ec8beaeed2888..4a9a36d87e663514ba0e184f7cb5328b5ac1084d 100644 (file)
--- a/module/zfs/zfs_chksum.c
+++ b/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);