strtomem_pad() review

strtomem_pad() review

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 7030 bytes --]

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 <kees@kernel.org>
	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 <lkp@intel.com>
	    Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@intel.com/
	    Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
	    Link: https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf9082>
	    Tested-by: Suren Baghdasaryan <surenb@google.com>
	    Signed-off-by: Kees Cook <kees@kernel.org>

	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 <kees@kernel.org>
	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 <arnd@arndb.de>
	    Fixes: dfbafa70bde2 ("string: Introduce strtomem() and strtomem_pad()")
	    Tested-by: Arnd Bergmann <arnd@arndb.de>
	    Cc: Andy Shevchenko <andy@kernel.org>
	    Cc: linux-hardening@vger.kernel.org
	    Signed-off-by: Kees Cook <keescook@chromium.org>

	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

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]