Received-SPF: pass (google.com: domain of linux-kernel+bounces-123846-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 28 Mar 2024 23:54:37 +0000
From: Justin Stitt <justinstitt@google.com>
To: Arnd Bergmann <arnd@kernel.org>
Cc: linux-kernel@vger.kernel.org, 
	Andrew Morton <akpm@linux-foundation.org>, Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH 05/11] test_hexdump: avoid string truncation warning
Message-ID: <u7hc4tkkwr5eua2p4bdetox2lqtawzb6dwimuipejhoiitbzhh@yu3e4sljhgxg>
References: <20240328140512.4148825-1-arnd@kernel.org>
 <20240328140512.4148825-6-arnd@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240328140512.4148825-6-arnd@kernel.org>

Hi,

On Thu, Mar 28, 2024 at 03:04:49PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> gcc can warn when a string is too long to fit into the strncpy()
> destination buffer, as it is here depending on the function
> arguments:
> 
>     inlined from 'test_hexdump_prepare_test.constprop' at /home/arnd/arm-soc/lib/test_hexdump.c:116:3:
> include/linux/fortify-string.h:108:33: error: '__builtin_strncpy' output truncated copying between 0 and 32 bytes from a string of length 32 [-Werror=stringop-truncation]
>   108 | #define __underlying_strncpy    __builtin_strncpy
>       |                                 ^
> include/linux/fortify-string.h:187:16: note: in expansion of macro '__underlying_strncpy'
>   187 |         return __underlying_strncpy(p, q, size);
>       |                ^~~~~~~~~~~~~~~~~~~~
> 
> As far as I can tell, this is harmless here because the truncation is
> intentional, but using strscpy_pad() will avoid the warning since gcc
> does not (yet) know about it.
>

We need to be careful. strscpy() or strscpy_pad() are not drop-in
replacements for strncpy().

if @l is less than the length of @data_a we might have a problem because
strscpy_pad() will eagerly assign a NUL-byte to dest[l-1].

It looks like @l could be less than the length of @data_a judging from:
        size_t len = get_random_u32_inclusive(1, d);


Let me model the potential behavior before and after, understanding that
data_a is defined as:

static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";

Before (using strncpy):
p = ['j', 'u', 'n', 'k'] // example destination buffer
assume @l = 3
then we are trying to copy ".2." from @data_a, resulting in
p = ['.', '2', '.', 'k']

After (using strscpy_pad()):
p = ['j', 'u', 'n', 'k'] // example destination buffer
assume @l = 3
then we are trying to copy ".2." from @data_a, resulting in
p = ['.', '2', '\0', 'k']

because strscpy got to the end of its allowed size and didn't find a
NUL-term from its source string, so it eagerly assigns a NUL-byte to the
end, essentially truncating our string.


Here's the responsible code from strscpy's implementation:
	if (res)
		dest[res-1] = '\0';

https://elixir.bootlin.com/linux/latest/source/lib/string.c#L107

It is possible I haven't fully considered the context of this change but
I think using strscpy_pad() will cause these tests to fail, if they
aren't failing I think we're getting lucky.


Also, maybe this godbolt example can help demonstrate:
https://godbolt.org/z/nWGKraTvT

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  lib/test_hexdump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
> index b916801f23a8..c9820122af56 100644
> --- a/lib/test_hexdump.c
> +++ b/lib/test_hexdump.c
> @@ -113,7 +113,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
>  			*p++ = ' ';
>  		} while (p < test + rs * 2 + rs / gs + 1);
>  
> -		strncpy(p, data_a, l);
> +		strscpy_pad(p, data_a, l);
>  		p += l;
>  	}
>  
> -- 
> 2.39.2
> 

Thanks
Justin