[PATCH] kunit: executor: Simplify string allocation handling

[PATCH] kunit: executor: Simplify string allocation handling

[Kees Cook]

The alloc/copy code pattern is better consolidated to single kstrdup (and
kstrndup) calls instead. This gets rid of deprecated[1] strncpy() uses as
well. Replace one other strncpy() use with the more idiomatic strscpy().

Link: https://github.com/KSPP/linux/issues/90 [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: David Gow <davidgow@google.com>
Cc: Rae Moar <rmoar@google.com>
Cc: linux-kselftest@vger.kernel.org
Cc: kunit-dev@googlegroups.com
---
 lib/kunit/executor.c      | 12 +++---------
 lib/kunit/executor_test.c |  2 +-
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 70b9a43cd257..34b7b6833df3 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -70,32 +70,26 @@ struct kunit_glob_filter {
 static int kunit_parse_glob_filter(struct kunit_glob_filter *parsed,
 				    const char *filter_glob)
 {
-	const int len = strlen(filter_glob);
 	const char *period = strchr(filter_glob, '.');
 
 	if (!period) {
-		parsed->suite_glob = kzalloc(len + 1, GFP_KERNEL);
+		parsed->suite_glob = kstrdup(filter_glob, GFP_KERNEL);
 		if (!parsed->suite_glob)
 			return -ENOMEM;
-
 		parsed->test_glob = NULL;
-		strcpy(parsed->suite_glob, filter_glob);
 		return 0;
 	}
 
-	parsed->suite_glob = kzalloc(period - filter_glob + 1, GFP_KERNEL);
+	parsed->suite_glob = kstrndup(filter_glob, period - filter_glob, GFP_KERNEL);
 	if (!parsed->suite_glob)
 		return -ENOMEM;
 
-	parsed->test_glob = kzalloc(len - (period - filter_glob) + 1, GFP_KERNEL);
+	parsed->test_glob = kstrdup(period + 1, GFP_KERNEL);
 	if (!parsed->test_glob) {
 		kfree(parsed->suite_glob);
 		return -ENOMEM;
 	}
 
-	strncpy(parsed->suite_glob, filter_glob, period - filter_glob);
-	strncpy(parsed->test_glob, period + 1, len - (period - filter_glob));
-
 	return 0;
 }
 
diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index 3f7f967e3688..7191be9c4f9b 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -286,7 +286,7 @@ static struct kunit_suite *alloc_fake_suite(struct kunit *test,
 
 	/* We normally never expect to allocate suites, hence the non-const cast. */
 	suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL);
-	strncpy((char *)suite->name, suite_name, sizeof(suite->name) - 1);
+	strscpy((char *)suite->name, suite_name);
 	suite->test_cases = test_cases;
 
 	return suite;
-- 
2.34.1

Re: [PATCH] kunit: executor: Simplify string allocation handling

[David Gow]

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

On Wed, 10 Jul 2024 at 08:02, Kees Cook <kees@kernel.org> wrote:
>
> The alloc/copy code pattern is better consolidated to single kstrdup (and
> kstrndup) calls instead. This gets rid of deprecated[1] strncpy() uses as
> well. Replace one other strncpy() use with the more idiomatic strscpy().
>
> Link: https://github.com/KSPP/linux/issues/90 [1]
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> Cc: Brendan Higgins <brendan.higgins@linux.dev>
> Cc: David Gow <davidgow@google.com>
> Cc: Rae Moar <rmoar@google.com>
> Cc: linux-kselftest@vger.kernel.org
> Cc: kunit-dev@googlegroups.com
> ---

Looks good apart from the strscpy() change, which is broken by the
(char *) cast. Using the 3-argument version worked here.

With the strscpy() fixed, this is:

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  lib/kunit/executor.c      | 12 +++---------
>  lib/kunit/executor_test.c |  2 +-
>  2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 70b9a43cd257..34b7b6833df3 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -70,32 +70,26 @@ struct kunit_glob_filter {
>  static int kunit_parse_glob_filter(struct kunit_glob_filter *parsed,
>                                     const char *filter_glob)
>  {
> -       const int len = strlen(filter_glob);
>         const char *period = strchr(filter_glob, '.');
>
>         if (!period) {
> -               parsed->suite_glob = kzalloc(len + 1, GFP_KERNEL);
> +               parsed->suite_glob = kstrdup(filter_glob, GFP_KERNEL);
>                 if (!parsed->suite_glob)
>                         return -ENOMEM;
> -
>                 parsed->test_glob = NULL;
> -               strcpy(parsed->suite_glob, filter_glob);
>                 return 0;
>         }
>
> -       parsed->suite_glob = kzalloc(period - filter_glob + 1, GFP_KERNEL);
> +       parsed->suite_glob = kstrndup(filter_glob, period - filter_glob, GFP_KERNEL);
>         if (!parsed->suite_glob)
>                 return -ENOMEM;
>
> -       parsed->test_glob = kzalloc(len - (period - filter_glob) + 1, GFP_KERNEL);
> +       parsed->test_glob = kstrdup(period + 1, GFP_KERNEL);
>         if (!parsed->test_glob) {
>                 kfree(parsed->suite_glob);
>                 return -ENOMEM;
>         }
>
> -       strncpy(parsed->suite_glob, filter_glob, period - filter_glob);
> -       strncpy(parsed->test_glob, period + 1, len - (period - filter_glob));
> -
>         return 0;
>  }
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index 3f7f967e3688..7191be9c4f9b 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -286,7 +286,7 @@ static struct kunit_suite *alloc_fake_suite(struct kunit *test,
>
>         /* We normally never expect to allocate suites, hence the non-const cast. */
>         suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL);
> -       strncpy((char *)suite->name, suite_name, sizeof(suite->name) - 1);
> +       strscpy((char *)suite->name, suite_name);

This is broken: we still need to pass the length of suite->name. The
(char *) cast, which is necessary to remove the 'cosnt' qualifier,
stops the strscpy() macro from treating suite->name as an array.

>         suite->test_cases = test_cases;
>
>         return suite;
> --
> 2.34.1
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

Re: [PATCH] kunit: executor: Simplify string allocation handling

[Kees Cook]

On Thu, Jul 11, 2024 at 01:39:15PM +0800, David Gow wrote:
> On Wed, 10 Jul 2024 at 08:02, Kees Cook <kees@kernel.org> wrote:
> > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> > index 3f7f967e3688..7191be9c4f9b 100644
> > --- a/lib/kunit/executor_test.c
> > +++ b/lib/kunit/executor_test.c
> > @@ -286,7 +286,7 @@ static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> >
> >         /* We normally never expect to allocate suites, hence the non-const cast. */
> >         suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL);
> > -       strncpy((char *)suite->name, suite_name, sizeof(suite->name) - 1);
> > +       strscpy((char *)suite->name, suite_name);
> 
> This is broken: we still need to pass the length of suite->name. The
> (char *) cast, which is necessary to remove the 'cosnt' qualifier,
> stops the strscpy() macro from treating suite->name as an array.

Ah! Thanks for catching that. I do build tests with "allmodconfig", and
I saw the #include for executor_test.c, but didn't notice it was for
_builtin_ only...

-- 
Kees Cook