On Thu, Oct 19, 2023 at 05:54:15PM +0000, Justin Stitt wrote: > Found with grep. > > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect super->signature to be NUL-terminated based on its usage with > memcmp against a NUL-term'd buffer: > btt_devs.c: > 253 | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) > btt.h: > 13 | #define BTT_SIG "BTT_ARENA_INFO\0" > > NUL-padding is not required as `super` is already zero-allocated: > btt.c: > 985 | super = kzalloc(sizeof(struct btt_sb), GFP_NOIO); > ... rendering any additional NUL-padding superfluous. > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Let's also use the more idiomatic strscpy usage of (dest, src, > sizeof(dest)) instead of (dest, src, XYZ_LEN) for buffers that the > compiler can determine the size of. This more tightly correlates the > destination buffer to the amount of bytes copied. > > Side note, this pattern of memcmp() on two NUL-terminated strings should > really be changed to just a strncmp(), if i'm not mistaken? I see > multiple instances of this pattern in this system: > > | if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0) > | return false; > > where BIT_SIG is defined (weirdly) as a double NUL-terminated string: > > | #define BTT_SIG "BTT_ARENA_INFO\0" > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinst...@google.com>
Reviewed-by: Kees Cook <keesc...@chromium.org> -- Kees Cook
