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 #14098

3 files changed, 31 insertions, 2 deletions

diff --git a/lib/libspl/os/linux/zone.c b/lib/libspl/os/linux/zone.c
index e37b34975..622d04cbc 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 f4900943c..5e280cbae 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 1f58acb04..1d4647094 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)
 {