| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75Vf+zzVyVZ=fcXgdCMsp1MZdHmJ1aBTKFE4jHai2pWi27g@mail.gmail.com>
Date: Fri, 1 Mar 2024 13:20:41 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Andy Shevchenko <andy@...nel.org>, Kent Overstreet <kent.overstreet@...ux.dev>,
linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@...omium.org> wrote:
>
> Convert test-string_helpers.c to KUnit so it can be easily run with
> everything else.
...
> -#include <linux/array_size.h>
> #include <linux/init.h>
> +#include <kunit/test.h>
I know the order is broken here, but don't make it worse, please. And
stick with one schema where to put kunit/test.h always before
everything else and somewhere else (like always after linux/*).
> +#include <linux/array_size.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/module.h>
...
> +static void test_string_check_buf(struct kunit *test,
> + const char *name, unsigned int flags,
> + char *in, size_t p,
> + char *out_real, size_t q_real,
> + char *out_test, size_t q_test)
> {
> - if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> - return true;
> + int result;
> +
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
This needs a message.
> + result = memcmp(out_test, out_real, q_test);
> + KUNIT_EXPECT_EQ(test, 0, result);
Why do we need this assertion? We have a dump below to show what's wrong.
> + if (q_real == q_test && result == 0)
> + return;
I'm not sure this is an equivalent change. IIRC KUnit assertions do
not continue on failure. (Long time last time I run KUnit test)
>
> pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
>
> @@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *name, unsigned int flags,
> out_test, q_test, true);
> print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
> out_real, q_real, true);
> -
> - return false;
> }
...
> +static void
> +test_string_escape_overflow(struct kunit *test,
> + const char *in, int p, unsigned int flags, const char *esc,
> int q_test, const char *name)
> {
> int q_real;
>
> q_real = string_escape_mem(in, p, NULL, 0, flags, esc);
> - if (q_real != q_test)
> - pr_warn("Test '%s' failed: flags = %#x, osz = 0, expected %d, got %d\n",
> - name, flags, q_test, q_real);
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
You killed the message, not good.
> }
...
> +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2) \
> + do { \
> + BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf); \
> + BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf); \
No analogous assertions in KUnit?
> + __test_string_get_size(test, (size), (blk_size), (exp_result10), \
> + (exp_result2)); \
> } while (0)
...
> {
> - if (!memcmp(res, exp, strlen(exp) + 1))
> + int result = memcmp(res, exp, strlen(exp) + 1);
> + KUNIT_EXPECT_EQ(test, 0, result);
As per above, what's the added value of this assertion?
> + if (!result)
> return;
...
> @@ -590,65 +604,68 @@ static void __init test_string_upper_lower(void)
> for (i = 0; i < ARRAY_SIZE(strings_upper); i++) {
> const char *s = strings_upper[i].in;
> int len = strlen(strings_upper[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_upper(dst, s);
> - if (memcmp(dst, strings_upper[i].out, len)) {
> + result = memcmp(dst, strings_upper[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_upper' failed : expected %s, got %s!\n",
> strings_upper[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
>
> for (i = 0; i < ARRAY_SIZE(strings_lower); i++) {
> const char *s = strings_lower[i].in;
> int len = strlen(strings_lower[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_lower(dst, s);
> - if (memcmp(dst, strings_lower[i].out, len)) {
> + result = memcmp(dst, strings_lower[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_lower failed : : expected %s, got %s!\n",
> strings_lower[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
> }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists