linux-hardening - Re: [PATCH v2] lib: overflow: Convert 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 PHC
Open Source and information security mailing list archives

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

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

Date:   Sun, 27 Feb 2022 09:33:46 -0800
From:   Kees Cook <keescook@...omium.org>
To:     David Gow <davidgow@...gle.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Vitor Massaru Iha <vitor@...saru.org>,
        Daniel Latypov <dlatypov@...gle.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2] lib: overflow: Convert to Kunit

On Fri, Feb 25, 2022 at 03:57:17PM +0800, David Gow wrote:
> On Thu, Feb 24, 2022 at 1:48 PM Kees Cook <keescook@...omium.org> wrote:
> >
> > Convert overflow unit tests to KUnit, for better integration into the
> > kernel self test framework. Includes a rename of test_overflow.c to
> > overflow_kunit.c, and CONFIG_TEST_OVERFLOW to CONFIG_OVERFLOW_KUNIT_TEST.
> >
> > $ ./tools/testing/kunit/kunit.py config
> > ...
> (This 'config' step should be unnecessary... See below)

Ah yeah. I've removed that now.

> > $ ./tools/testing/kunit/kunit.py run overflow
> > ...
> > [14:33:51] Starting KUnit Kernel (1/1)...
> > [14:33:51] ============================================================
> > [14:33:51] ================== overflow (11 subtests) ==================
> > [14:33:51] [PASSED] u8_overflow_test
> > [14:33:51] [PASSED] s8_overflow_test
> > [14:33:51] [PASSED] u16_overflow_test
> > [14:33:51] [PASSED] s16_overflow_test
> > [14:33:51] [PASSED] u32_overflow_test
> > [14:33:51] [PASSED] s32_overflow_test
> > [14:33:51] [PASSED] u64_overflow_test
> > [14:33:51] [PASSED] s64_overflow_test
> > [14:33:51] [PASSED] overflow_shift_test
> > [14:33:51] [PASSED] overflow_allocation_test
> > [14:33:51] [PASSED] overflow_size_helpers_test
> > [14:33:51] ==================== [PASSED] overflow =====================
> > [14:33:51] ============================================================
> > [14:33:51] Testing complete. Passed: 11, Failed: 0, Crashed: 0, Skipped: 0, Errors: 0
> > [14:33:51] Elapsed time: 12.525s total, 0.001s configuring, 12.402s building, 0.101s running
> >
> > Cc: David Gow <davidgow@...gle.com>
> > Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
> > Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> > Co-developed-by: Vitor Massaru Iha <vitor@...saru.org>
> > Signed-off-by: Vitor Massaru Iha <vitor@...saru.org>
> > Link: https://lore.kernel.org/lkml/20200720224418.200495-1-vitor@massaru.org/
> > Co-developed-by: Daniel Latypov <dlatypov@...gle.com>
> > Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> > Link: https://lore.kernel.org/linux-kselftest/20210503211536.1384578-1-dlatypov@google.com/
> > Acked-by: Nick Desaulniers <ndesaulniers@...gle.com>
> > Link: https://lore.kernel.org/lkml/CAKwvOdm62iA1dNiC6Q11UJ-MnTqtc4kXkm-ubPaFMK824_k0nw@mail.gmail.com
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> > v1: https://lore.kernel.org/lkml/20220216224153.2242451-1-keescook@chromium.org/
> > v2: Fixed up the authorship more, pulled in other prior changes (Daniel)
> 
> This looks good to me. Some of the test cases _could_ probably be done
> as parameterised tests (given they're already basically table-driven),
> but I admit that could end up being excessively verbose.

In the end there was so much churn already I decided to leave it as-is.

> Apart from my not being able to find a tree this applies cleanly on,
> the tests work well here.

Ah, sorry about that! (More below...)

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

Thanks!

> [...]
> > @@ -600,127 +554,118 @@ struct __test_flex_array {
> >         unsigned long data[];
> ">  };
> >
> > -static int __init test_overflow_size_helpers(void)
> > +static void overflow_size_helpers_test(struct kunit *test)
> >  {
> >         /* Make sure struct_size() can be used in a constant expression. */
> >         u8 ce_array[struct_size((struct __test_flex_array *)0, data, 55)];
> >         struct __test_flex_array *obj;
> >         int count = 0;
> > -       int err = 0;
> >         int var;
> >         volatile int unconst = 0;
> 
> Where does this variable declaration (and its use below) come from?
> It's not in the version of the "overflow: Implement size_t saturating
> arithmetic helpers" patch in linux-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6312fc63aee9d2fef2f9c4d42e57e5b46828f0dc
> 
> It all works if I add it in manually, but it does make applying the
> change as-is something of a pain.

Ah, thanks for noticing that! It looks like I failed to send an update
for that patch. I'll resend this portion of my tree.

-Kees

-- 
Kees Cook