linux-kernel - Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202403011227.6C63925@keescook>
Date: Fri, 1 Mar 2024 12:29:26 -0800
From: Kees Cook <keescook@...omium.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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 01, 2024 at 01:20:41PM +0200, Andy Shevchenko wrote:
> 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/*).

Fixed.

> 
> > +#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.

I've improved this all around with ...STREQ and ...MEMEQ calls, and
added _MSG versions where needed to retain the prior reported test
context.

> ...
> 
> > +#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?

It's designed for runtime testing. This is fine as-is for compile-time
asserts.

Thanks for the reviews!

-Kees

-- 
Kees Cook