Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Oct 2022 12:06:36 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     linux-hardening@vger.kernel.org,
        Nick Desaulniers <ndesaulniers@google.com>,
        llvm@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fortify: Short-circuit known-safe calls to strscpy()
Message-ID: <Y075PIwTnnYF3Ak7@dev-arch.thelio-3990X>
References: <20221018083051.never.939-kees@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221018083051.never.939-kees@kernel.org>
Precedence: bulk

On Tue, Oct 18, 2022 at 01:32:51AM -0700, Kees Cook wrote:
> Replacing compile-time safe calls of strcpy()-related functions with
> strscpy() was always calling the full strscpy() logic when a builtin
> would be better. For example:
> 
> 	char buf[16];
> 	strcpy(buf, "yes");
> 
> would reduce to __builtin_memcpy(buf, "yes", 4), but not if it was:
> 
> 	strscpy(buf, yes, sizeof(buf));
> 
> Fix this by checking if all sizes are known at compile-time.
> 
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

I ran the following commands and the tests all passed:

$ tools/testing/kunit/kunit.py run --arch arm64 --cross_compile aarch64-linux-gnu- strscpy

$ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 strscpy

$ tools/testing/kunit/kunit.py run --arch x86_64 strscpy

$ tools/testing/kunit/kunit.py run --arch x86_64 --make_options LLVM=1 strscpy

$ tools/testing/kunit/kunit.py run --arch arm64 --cross_compile aarch64-linux-gnu- --kconfig_add CONFIG_FORTIFY_SOURCE=y strscpy

$ tools/testing/kunit/kunit.py run --arch arm64 --kconfig_add CONFIG_FORTIFY_SOURCE=y --make_options LLVM=1 strscpy

$ tools/testing/kunit/kunit.py run --arch x86_64 --kconfig_add CONFIG_FORTIFY_SOURCE=y strscpy

$ tools/testing/kunit/kunit.py run --arch x86_64 --kconfig_add CONFIG_FORTIFY_SOURCE=y --make_options LLVM=1 strscpy

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  include/linux/fortify-string.h | 10 ++++++++++
>  lib/strscpy_kunit.c            | 13 +++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index aa1a50009632..c473adb55cf5 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -321,6 +321,16 @@ __FORTIFY_INLINE ssize_t strscpy(char * const POS p, const char * const POS q, s
>  	if (__compiletime_lessthan(p_size, size))
>  		__write_overflow();
>  
> +	/* Short-circuit for compile-time known-safe lengths. */
> +	if (__compiletime_lessthan(p_size, SIZE_MAX)) {
> +		len = __compiletime_strlen(q);
> +
> +		if (len < SIZE_MAX && __compiletime_lessthan(len, size)) {
> +			__underlying_memcpy(p, q, len + 1);
> +			return len;
> +		}
> +	}
> +
>  	/*
>  	 * This call protects from read overflow, because len will default to q
>  	 * length if it smaller than size.
> diff --git a/lib/strscpy_kunit.c b/lib/strscpy_kunit.c
> index 98523f828d3a..a6b6344354ed 100644
> --- a/lib/strscpy_kunit.c
> +++ b/lib/strscpy_kunit.c
> @@ -81,6 +81,8 @@ static void tc(struct kunit *test, char *src, int count, int expected,
>  
>  static void strscpy_test(struct kunit *test)
>  {
> +	char dest[8];
> +
>  	/*
>  	 * tc() uses a destination buffer of size 6 and needs at
>  	 * least 2 characters spare (one for null and one to check for
> @@ -111,6 +113,17 @@ static void strscpy_test(struct kunit *test)
>  	tc(test, "ab",   4, 2,	    2, 1, 1);
>  	tc(test, "a",    4, 1,	    1, 1, 2);
>  	tc(test, "",     4, 0,	    0, 1, 3);
> +
> +	/* Compile-time-known source strings. */
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "", ARRAY_SIZE(dest)), 0);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "", 3), 0);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "", 1), 0);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "", 0), -E2BIG);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "Fixed", ARRAY_SIZE(dest)), 5);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "Fixed", 3), -E2BIG);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "Fixed", 1), -E2BIG);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "Fixed", 0), -E2BIG);
> +	KUNIT_EXPECT_EQ(test, strscpy(dest, "This is too long", ARRAY_SIZE(dest)), -E2BIG);
>  }
>  
>  static struct kunit_case strscpy_test_cases[] = {
> -- 
> 2.34.1
> 
>