From faaf26cb2588c231c48f8160f0023a58101731d0 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Thu, 27 Oct 2022 14:16:04 -0400 Subject: [PATCH] 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. Signed-off-by: Richard Yao --- cmd/zed/zed_event.c | 6 ++++- include/os/freebsd/spl/sys/kmem.h | 3 +++ include/os/linux/spl/sys/kmem.h | 2 ++ include/sys/spa.h | 2 +- include/sys/zfs_context.h | 3 +++ lib/libspl/os/linux/zone.c | 2 +- lib/libzfs/os/freebsd/libzfs_compat.c | 2 +- lib/libzpool/kernel.c | 29 ++++++++++++++++++++ module/os/freebsd/spl/spl_string.c | 30 +++++++++++++++++++++ module/os/linux/zfs/zfs_sysfs.c | 6 ++--- module/zfs/dataset_kstats.c | 7 ++++- module/zfs/spa_misc.c | 2 +- module/zfs/vdev_raidz_math.c | 23 ++++++++-------- module/zfs/zfs_chksum.c | 38 +++++++++++++-------------- tests/zfs-tests/cmd/mmapwrite.c | 2 +- 15 files changed, 117 insertions(+), 40 deletions(-) diff --git a/cmd/zed/zed_event.c b/cmd/zed/zed_event.c index 079c6a043e5d..ebd6851a2a86 100644 --- 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 ae2941b80912..27d290863c0b 100644 --- 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 86e872fa2492..111924303e16 100644 --- 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 8dcd8895ba22..b260dd32820e 100644 --- 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 d29d7118ff00..0d31195447d1 100644 --- 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 e37b34975df6..622d04cbc14a 100644 --- 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 f4900943c4af..5e280cbae935 100644 --- 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 1f58acb0404f..1d46470947d3 100644 --- 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 523e10ff6936..eb74720c984a 100644 --- 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 4a2091f3c396..e2431fe8a803 100644 --- 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 b63f42a21e44..57b8faf213eb 100644 --- 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 9ec37502986f..ca55d55405d3 100644 --- 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 f74a76a8d5ba..2980f8acfbd7 100644 --- 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 74b4cb8d2e63..4a9a36d87e66 100644 --- 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); diff --git a/tests/zfs-tests/cmd/mmapwrite.c b/tests/zfs-tests/cmd/mmapwrite.c index 04ccd3c871fa..a18609898485 100644 --- a/tests/zfs-tests/cmd/mmapwrite.c +++ b/tests/zfs-tests/cmd/mmapwrite.c @@ -99,7 +99,7 @@ map_writer(void *filename) err(1, "open file failed"); } ret = ftruncate(fd, page_size); - if (fd == -1) { + if (ret == -1) { err(1, "truncate file failed"); } } else {