Re: [PATCH v2 3/3] string.h: Use ARRAY_SIZE() for memtostr*()/strtomem*()

[Suren Baghdasaryan]

On Thu, Feb 6, 2025 at 1:40 PM Kees Cook <kees@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-kbuild-all/202501310832.kiAeOt2z-lkp@xxxxxxxxx/
> Suggested-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> Link: https://github.com/llvm/llvm-project/commit/d8e0a6d5e9dd2311641f9a8a5d2bf90829951ddc [1]
> Signed-off-by: Kees Cook <kees@xxxxxxxxxx>

Tested-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

Thanks Kees!
CC'ing Andrew Morton since mm-unstable contains my workaround patch
[1] for the issue fixed by this patchset. I'm not sure which tree will
be taking these fixes but once they are in we should drop [1]. Or
maybe we just drop it now from mm-unstable? Seems like only testbot
was the only one who complained...

[1] 0e91345e75b2 ("alloc_tag: work around clang-14 build issue with
__builtin_object_size()")

> ---
> Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Cc: nathan@xxxxxxxxxx
> Cc: Andy Shevchenko <andy@xxxxxxxxxx>
> Cc: linux-hardening@xxxxxxxxxxxxxxx
> ---
>  include/linux/string.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 493ac4862c77..fc5ae145bd78 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -411,7 +411,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * must be discoverable by the compiler.
>   */
>  #define strtomem_pad(dest, src, pad)   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) ||                \
> @@ -434,7 +435,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * must be discoverable by the compiler.
>   */
>  #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) ||                \
> @@ -453,7 +455,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * Note that sizes of @dest and @src must be known at compile-time.
>   */
>  #define memtostr(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);          \
>         const size_t _src_chars = strnlen(src, _src_len);               \
>         const size_t _copy_len = min(_dest_len - 1, _src_chars);        \
> @@ -478,7 +481,8 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
>   * Note that sizes of @dest and @src must be known at compile-time.
>   */
>  #define memtostr_pad(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);          \
>         const size_t _src_chars = strnlen(src, _src_len);               \
>         const size_t _copy_len = min(_dest_len - 1, _src_chars);        \
> --
> 2.34.1
>