[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABVgOSnXgA20O9bHTUP8uwcMH3Wggb2Agjpc3MK9qQa0+oH_Bg@mail.gmail.com>
Date: Thu, 11 Jul 2024 13:39:15 +0800
From: David Gow <davidgow@...gle.com>
To: Kees Cook <kees@...nel.org>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <rmoar@...gle.com>,
linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] kunit: executor: Simplify string allocation handling
On Wed, 10 Jul 2024 at 08:02, Kees Cook <kees@...nel.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@...nel.org>
> ---
> Cc: Brendan Higgins <brendan.higgins@...ux.dev>
> Cc: David Gow <davidgow@...gle.com>
> Cc: Rae Moar <rmoar@...gle.com>
> Cc: linux-kselftest@...r.kernel.org
> Cc: kunit-dev@...glegroups.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@...gle.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
>
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)
Powered by blists - more mailing lists