linux-hardening - Re: mocking init

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Dec 2022 22:52:48 -0800
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     David Gow <davidgow@...gle.com>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        kunit-dev@...glegroups.com, Petr Skocik <pskocik@...il.com>,
        linux-hardening@...r.kernel.org
Subject: Re: mocking init_task ?

On Thu, Dec 1, 2022 at 8:28 PM Kees Cook <keescook@...omium.org> wrote:
>
> Hi,
>
> I want to make a unit test for kill_something_info(), as there is a patch
> to fix a bug with it not working as expected under a specific process
> tree arrangement[1]. This seems like a great candidate for a unit test:
> given a specific state, return a specific result. Emboldened, I applied
> the "kunit: Support redirecting function calls" series[2], preparing to
> mock group_send_sig_info(), and ran head-long into for_each_process()
> ... which uses the address of the global init_task:
>
> #define for_each_process(p) \
>         for (p = &init_task ; (p = next_task(p)) != &init_task ; )
>
> :(
>
> I'm curious what you think might be the right approach to mock
> init_task, or for_each_process(), so I can apply unit tests to some of
> the "simple" process tree walkers...
>
> One idea I had was using the "kunit: Provide a static key to check if
> KUnit is actively running tests" series[3], and do something like this:
>
> #ifndef CONFIG_KUNIT
> #define init_task_ptr   &init_task
> #else
> #define init_task_ptr   ({                                      \
>                 struct task_struct *task = &init_task;          \
>                 if (static_branch_unlikely(&kunit_running)) {   \

nit: if(kunit_get_current_test()) is probably a bit safer.
For UML, it will never make a difference (IIUC), but we probably want
to ensure that current == the task_struct executing kunit tests.

>                         struct kunit *test;                     \
>                         test = current->kunit_test;             \
>                         if (test->mock_init_task)               \
>                                 task = test->mock_init_task;    \
>                 }                                               \
>                 task;                                           \
>         })
> #endif
>
> #define for_each_process(p) \
>         for (p = init_task_ptr ; (p = next_task(p)) != init_task_ptr ; )

I think this is pretty close to being the best you can do.

I have a terrible idea that afaict should work.
If it does work, I like it a lot better since it's less intrusive.

== my_test.c ==

#include <linux/sched/signal.h>
#undef for_each_process
#define for_each_process(p) \
        for (p = test_init_task ; (p = next_task(p)) != test_init_task ; )

struct task_struct _test_init_task = {
        .tasks          = LIST_HEAD_INIT(_test_init_task.tasks),
};

#define test_init_task ({ \
                        struct task_struct *task = &init_task;\
                        if (kunit_get_current_test()) {\
                                task = &_test_init_task;\
                        }\
                        task;\
})

#include <kunit/test.h>

static void my_test(struct kunit *test)
{
        struct task_struct *p;

        for_each_process(p)
                KUNIT_FAIL(test, "should not be called");

       /* can modify test_init_task as you see fit here */
}

And then you explicitly keep nothing else in this translation unit so
we limit the damage we've done.

Daniel

>
> And then tests can hang a mock init_task off the test? It seems really
> horrible, but there is a LOT of global state in the kernel, so I figured
> I had to start somewhere? :P
>
> Thoughts?
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/20221122161240.137570-1-pskocik@gmail.com/
> [2] https://lore.kernel.org/lkml/20220910212804.670622-1-davidgow@google.com/
> [3] https://lore.kernel.org/lkml/20221125084306.1063074-1-davidgow@google.com/
>
> --
> Kees Cook