diff options
author | Richard Yao <richard.yao@alumni.stonybrook.edu> | 2022-10-27 18:16:04 +0000 |
---|---|---|
committer | Brian Behlendorf <behlendorf1@llnl.gov> | 2022-10-29 20:05:11 +0000 |
commit | 97143b9d314d54409244f3995576d8cc8c1ebf0a (patch) | |
tree | efb7386fdf61ba289a8e99e8f958bc9b21c5f31e /include/os/freebsd/spl | |
parent | 2e08df84d8649439e5e9ed39ea13d4b755ee97c9 (diff) | |
download | src-97143b9d314d54409244f3995576d8cc8c1ebf0a.tar.gz src-97143b9d314d54409244f3995576d8cc8c1ebf0a.zip |
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
Diffstat (limited to 'include/os/freebsd/spl')
-rw-r--r-- | include/os/freebsd/spl/sys/kmem.h | 3 |
1 files changed, 3 insertions, 0 deletions
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) |