linux-kernel - Re: [PATCH v4] kunit: run test suites only after module initialization completes

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: <CAMuHMdXTA9WK8vJYUYma33KZK7199MX+dghzHXTE4JKygTsbPg@mail.gmail.com>
Date: Fri, 26 Jan 2024 15:44:28 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Marco Pagani <marpagan@...hat.com>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>, David Gow <davidgow@...gle.com>, 
	Jinjie Ruan <ruanjinjie@...wei.com>, Rae Moar <rmoar@...gle.com>, 
	Shuah Khan <skhan@...uxfoundation.org>, Richard Fitzgerald <rf@...nsource.cirrus.com>, 
	Javier Martinez Canillas <javierm@...hat.com>, linux-kselftest@...r.kernel.org, 
	kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] kunit: run test suites only after module
 initialization completes

Hi Marco,

On Wed, Jan 10, 2024 at 5:01 PM Marco Pagani <marpagan@...hat.com> wrote:
> Commit 2810c1e99867 ("kunit: Fix wild-memory-access bug in
> kunit_free_suite_set()") fixed a wild-memory-access bug that could have
> happened during the loading phase of test suites built and executed as
> loadable modules. However, it also introduced a problematic side effect
> that causes test suites modules to crash when they attempt to register
> fake devices.
>
> When a module is loaded, it traverses the MODULE_STATE_UNFORMED and
> MODULE_STATE_COMING states before reaching the normal operating state
> MODULE_STATE_LIVE. Finally, when the module is removed, it moves to
> MODULE_STATE_GOING before being released. However, if the loading
> function load_module() fails between complete_formation() and
> do_init_module(), the module goes directly from MODULE_STATE_COMING to
> MODULE_STATE_GOING without passing through MODULE_STATE_LIVE.
>
> This behavior was causing kunit_module_exit() to be called without
> having first executed kunit_module_init(). Since kunit_module_exit() is
> responsible for freeing the memory allocated by kunit_module_init()
> through kunit_filter_suites(), this behavior was resulting in a
> wild-memory-access bug.
>
> Commit 2810c1e99867 ("kunit: Fix wild-memory-access bug in
> kunit_free_suite_set()") fixed this issue by running the tests when the
> module is still in MODULE_STATE_COMING. However, modules in that state
> are not fully initialized, lacking sysfs kobjects. Therefore, if a test
> module attempts to register a fake device, it will inevitably crash.
>
> This patch proposes a different approach to fix the original
> wild-memory-access bug while restoring the normal module execution flow
> by making kunit_module_exit() able to detect if kunit_module_init() has
> previously initialized the tests suite set. In this way, test modules
> can once again register fake devices without crashing.
>
> This behavior is achieved by checking whether mod->kunit_suites is a
> virtual or direct mapping address. If it is a virtual address, then
> kunit_module_init() has allocated the suite_set in kunit_filter_suites()
> using kmalloc_array(). On the contrary, if mod->kunit_suites is still
> pointing to the original address that was set when looking up the
> kunit_test_suites section of the module, then the loading phase has
> failed and there's no memory to be freed.
>
> v4:
> - rebased on 6.8
> - noted that kunit_filter_suites() must return a virtual address
> v3:
> - add a comment to clarify why the start address is checked
> v2:
> - add include <linux/mm.h>
>
> Fixes: 2810c1e99867 ("kunit: Fix wild-memory-access bug in kunit_free_suite_set()")
> Reviewed-by: David Gow <davidgow@...gle.com>
> Tested-by: Rae Moar <rmoar@...gle.com>
> Tested-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>
> Signed-off-by: Marco Pagani <marpagan@...hat.com>

Thank you, this fixes the crashes I was seeing when running
platform_device_test, and which I bisected to the commit in the
Fixes tag.

Tested-by: Geert Uytterhoeven <geert+renesas@...der.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68korg

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds