Hi Kees, Several people brought to my attention the news that the kernel has "removed" strncpy(3) --in reality, it has just renamed it into strtomem_pad()--. That led me to have a look at what you're using instead, and as expected, strncpy(3) still exists, with a different name. I'd like to say that I like the name; it's certainly better than strncpy(3). It tells exactly what it does and what it is. I'll review some commits that affected this macro, and also the resulting macro. commit 6270f4deba3fbd77d1717fb8634f1fc612ff69e2 Author: Kees Cook Date: 2025-02-05 13:45:26 -0800 string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*() The destination argument of memtostr*() and strtomem*() must be a fixed-size char array at compile time, so there is no need to use __builtin_object_size() (which is useful for when an argument is either a pointer or unknown). Instead use ARRAY_SIZE(), which has the benefit of working around a bug in Clang (fixed[1] in 15+) that got __builtin_object_size() wrong sometimes. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/ Suggested-by: Kent Overstreet Link: https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf9082> Tested-by: Suren Baghdasaryan Signed-off-by: Kees Cook diff --git a/include/linux/string.h b/include/linux/string.h --- a/include/linux/string.h +++ b/include/linux/string.h @@ -435,9 +436,10 @@ */ #define strtomem(dest, src) do { \ - const size_t _dest_len = __builtin_object_size(dest, 1); \ + const size_t _dest_len = __must_be_byte_array(dest) + \ + ARRAY_SIZE(dest); \ const size_t _src_len = __builtin_object_size(src, 1); \ \ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ _dest_len == (size_t)-1); \ memcpy(dest, src, strnlen(src, min(_src_len, _dest_len))); \ } while (0) The commit above could have removed the (size_t)-1 check in the BUILD_BUG_ON() call, I believe. ARRAY_SIZE() already implies that the size is not (size_t)-1. To be complete, it would also imply that the size is constant, until the kernel starts using VMTs, so the entire BUILD_BUG_ON() call could be removed, but just in case we use VMTs in the future, the part about __builtin_constant_p(_dest_len) could be kept. BTW, I agree ARRAY_SIZE() is a better choice here. All the uses of strtomem() use indeed a compile-time-fixed size. commit 0e108725f6cc5b3be9e607f89c9fbcbb236367b7 Author: Kees Cook Date: 2023-10-18 10:53:58 -0700 string: Adjust strtomem() logic to allow for smaller sources Arnd noticed we have a case where a shorter source string is being copied into a destination byte array, but this results in a strnlen() call that exceeds the size of the source. This is seen with -Wstringop-overread: In file included from ../include/linux/uuid.h:11, from ../include/linux/mod_devicetable.h:14, from ../include/linux/cpufeature.h:12, from ../arch/x86/coco/tdx/tdx.c:7: ../arch/x86/coco/tdx/tdx.c: In function 'tdx_panic.constprop': ../include/linux/string.h:284:9: error: 'strnlen' specified bound 64 exceeds source size 60 [-Werror=stringop-overread] 284 | memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../arch/x86/coco/tdx/tdx.c:124:9: note: in expansion of macro 'strtomem_pad' 124 | strtomem_pad(message.str, msg, '\0'); | ^~~~~~~~~~~~ Use the smaller of the two buffer sizes when calling strnlen(). When src length is unknown (SIZE_MAX), it is adjusted to use dest length, which is what the original code did. Reported-by: Arnd Bergmann Fixes: dfbafa70bde2 ("string: Introduce strtomem() and strtomem_pad()") Tested-by: Arnd Bergmann Cc: Andy Shevchenko Cc: linux-hardening@vger.kernel.org Signed-off-by: Kees Cook diff --git a/include/linux/string.h b/include/linux/string.h index dbfc66400050..9e3cb6923b0e 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -277,10 +277,12 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count, */ #define strtomem_pad(dest, src, pad) do { \ const size_t _dest_len = __builtin_object_size(dest, 1); \ + const size_t _src_len = __builtin_object_size(src, 1); \ \ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ _dest_len == (size_t)-1); \ - memcpy_and_pad(dest, _dest_len, src, strnlen(src, _dest_len), pad); \ + memcpy_and_pad(dest, _dest_len, src, \ + strnlen(src, min(_src_len, _dest_len)), pad); \ } while (0) /** @@ -298,10 +300,11 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count, */ #define strtomem(dest, src) do { \ const size_t _dest_len = __builtin_object_size(dest, 1); \ + const size_t _src_len = __builtin_object_size(src, 1); \ \ BUILD_BUG_ON(!__builtin_constant_p(_dest_len) || \ _dest_len == (size_t)-1); \ - memcpy(dest, src, min(_dest_len, strnlen(src, _dest_len))); \ + memcpy(dest, src, strnlen(src, min(_src_len, _dest_len))); \ } while (0) /** This is clearly a compiler bug. strnlen(3) will never read more than the string length. In case the compiler knows the size of the src buffer, it must also know that it's a properly-terminated string, and thus not diagnose anything about the buffer size. It is only in the case where the source is not a string when the size would matter. And thus, the only effects of this commit are: - Silencing a false-positive compiler diagnostic. - Silencing plausible bugs where the source string is not really a string. I'd strongly encourage talking to GCC to fix this bug. Per the commit message, I don't see that having been considered. Apart from these commits, I'd also like to mention that the ability to specify the padding seems superfluous. All uses call it with '\0', so it would be better to leave it as a mandatory NUL unless and until one use case for a different padding is found. The only current call with a non-NUL padding is in the tests of the macro itself. Also, I like the tests about the nonstring attribute. That makes this strncpy(3) variant quite robust. Have a lovely night! Alex --