Linux Kernel module notification bug

Linux Kernel module notification bug

[Adam Zabrocki]

Hello,

I believe that the following commit does introduce a gentle "functionality 
bug":

"module: delay kobject uevent until after module init call":
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3

The official Linux Kernel API for the kernel module activities notification has
been divided based on the readiness 'stage' for such module. We have the
following stages:

        MODULE_STATE_LIVE,      /* Normal state. */
        MODULE_STATE_COMING,    /* Full formed, running module_init. */
        MODULE_STATE_GOING,     /* Going away. */
        MODULE_STATE_UNFORMED,  /* Still setting it up. */

LIVE means that the kernel module is correctly running and all initialization
work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.

In the described commit, creation of the KOBJECT has been moved after invoking
a notficiation of the newly formed kernel module state (LIVE). That's somehow
inconsistent from my understanding of the kernel modules notifiers logic.
Creation of the new objects (like KOBJ) should be done before notification of
the stage LIVE is invoked.

This commit breaks some of the projects that rely on the LIVE notification to
monitor when the newly loaded module is ready.

I believe that the latest time when 'kobject_uevent' function can be called is
just before invoking 'blocking_notifier_call_chain', e.g:

diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..7d56b1b07237 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3681,14 +3681,14 @@ static noinline int do_init_module(struct module *mod)
                dump_stack();
        }

+       /* Delay uevent until module has finished its init routine */
+       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
+
        /* Now it's a first class citizen! */
        mod->state = MODULE_STATE_LIVE;
        blocking_notifier_call_chain(&module_notify_list,
                                     MODULE_STATE_LIVE, mod);

-       /* Delay uevent until module has finished its init routine */
-       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
-
        /*
         * We need to finish all async code before the module init sequence
         * is done.  This has potential to deadlock.  For example, a newly

Thanks,
Adam

--
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl

Re: Linux Kernel module notification bug

[Jessica Yu]

+++ Adam Zabrocki [10/01/21 18:54 +0100]:
>Hello,
>
>I believe that the following commit does introduce a gentle "functionality
>bug":
>
>"module: delay kobject uevent until after module init call":
>https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
>
>The official Linux Kernel API for the kernel module activities notification has
>been divided based on the readiness 'stage' for such module. We have the
>following stages:
>
>        MODULE_STATE_LIVE,      /* Normal state. */
>        MODULE_STATE_COMING,    /* Full formed, running module_init. */
>        MODULE_STATE_GOING,     /* Going away. */
>        MODULE_STATE_UNFORMED,  /* Still setting it up. */
>
>LIVE means that the kernel module is correctly running and all initialization
>work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
>
>In the described commit, creation of the KOBJECT has been moved after invoking
>a notficiation of the newly formed kernel module state (LIVE). That's somehow
>inconsistent from my understanding of the kernel modules notifiers logic.
>Creation of the new objects (like KOBJ) should be done before notification of
>the stage LIVE is invoked.

I'm confused. We're not creating any kobjects here. That is all done
in mod_sysfs_setup(), which is called while the module is still
COMING.  What that commit does is delay telling userspace about the
module (specifically, systemd/udev) until the module is basically
ready. Systemd was basically receiving the uevent too early, before
the module has initialized, hence we decided to delay the uevent [1].

>This commit breaks some of the projects that rely on the LIVE notification to
>monitor when the newly loaded module is ready.

Hm, could you please explain specifically what is the issue you're seeing?
What projects is it breaking?

Thanks,

Jessica

[1] https://github.com/systemd/systemd/issues/17586

Re: Linux Kernel module notification bug

[Adam Zabrocki]

Hello,

On Mon, Jan 11, 2021 at 03:20:48PM +0100, Jessica Yu wrote:
> +++ Adam Zabrocki [10/01/21 18:54 +0100]:
> > Hello,
> > 
> > I believe that the following commit does introduce a gentle "functionality
> > bug":
> > 
> > "module: delay kobject uevent until after module init call":
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
> > 
> > The official Linux Kernel API for the kernel module activities notification has
> > been divided based on the readiness 'stage' for such module. We have the
> > following stages:
> > 
> >        MODULE_STATE_LIVE,      /* Normal state. */
> >        MODULE_STATE_COMING,    /* Full formed, running module_init. */
> >        MODULE_STATE_GOING,     /* Going away. */
> >        MODULE_STATE_UNFORMED,  /* Still setting it up. */
> > 
> > LIVE means that the kernel module is correctly running and all initialization
> > work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
> > 
> > In the described commit, creation of the KOBJECT has been moved after invoking
> > a notficiation of the newly formed kernel module state (LIVE). That's somehow
> > inconsistent from my understanding of the kernel modules notifiers logic.
> > Creation of the new objects (like KOBJ) should be done before notification of
> > the stage LIVE is invoked.
> 
> I'm confused. We're not creating any kobjects here. That is all done
> in mod_sysfs_setup(), which is called while the module is still
> COMING.  What that commit does is delay telling userspace about the
> module (specifically, systemd/udev) until the module is basically
> ready. Systemd was basically receiving the uevent too early, before
> the module has initialized, hence we decided to delay the uevent [1].
> 

Sorry for the confusion on my side. I was referring to the internal state of 
the KOBJ itself which is being actively modified when uevent is sent. During 
the module creation KOBJ_ADD modifies 'kobj->state_add_uevent_sent'. Until 
recent commit, kernel didn't modify KOBJ after sending LIVE notification.

> > This commit breaks some of the projects that rely on the LIVE notification to
> > monitor when the newly loaded module is ready.
> 
> Hm, could you please explain specifically what is the issue you're seeing?
> What projects is it breaking?
> 

I'm specifically referencing these projects which are tracking kernel modules
for integrity purpose (e.g. anti-rootkit tools) like Linux Kernel Runtime 
Guard.
It is possible to modify these tools to adopt and take into account 
modification of KOBJ after LIVE state notification. However, from my 
understanding of the kernel modules notifiers logic, KOBJ should be fully 
formed at this stage.

Thanks,
Adam

> Thanks,
> 
> Jessica
> 
> [1] https://github.com/systemd/systemd/issues/17586

-- 
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl

Re: Linux Kernel module notification bug

[Jessica Yu]

+++ Adam Zabrocki [12/01/21 01:15 +0100]:
>Hello,
>
>On Mon, Jan 11, 2021 at 03:20:48PM +0100, Jessica Yu wrote:
>> +++ Adam Zabrocki [10/01/21 18:54 +0100]:
>> > Hello,
>> >
>> > I believe that the following commit does introduce a gentle "functionality
>> > bug":
>> >
>> > "module: delay kobject uevent until after module init call":
>> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
>> >
>> > The official Linux Kernel API for the kernel module activities notification has
>> > been divided based on the readiness 'stage' for such module. We have the
>> > following stages:
>> >
>> >        MODULE_STATE_LIVE,      /* Normal state. */
>> >        MODULE_STATE_COMING,    /* Full formed, running module_init. */
>> >        MODULE_STATE_GOING,     /* Going away. */
>> >        MODULE_STATE_UNFORMED,  /* Still setting it up. */
>> >
>> > LIVE means that the kernel module is correctly running and all initialization
>> > work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
>> >
>> > In the described commit, creation of the KOBJECT has been moved after invoking
>> > a notficiation of the newly formed kernel module state (LIVE). That's somehow
>> > inconsistent from my understanding of the kernel modules notifiers logic.
>> > Creation of the new objects (like KOBJ) should be done before notification of
>> > the stage LIVE is invoked.
>>
>> I'm confused. We're not creating any kobjects here. That is all done
>> in mod_sysfs_setup(), which is called while the module is still
>> COMING.  What that commit does is delay telling userspace about the
>> module (specifically, systemd/udev) until the module is basically
>> ready. Systemd was basically receiving the uevent too early, before
>> the module has initialized, hence we decided to delay the uevent [1].
>>
>
>Sorry for the confusion on my side. I was referring to the internal state of
>the KOBJ itself which is being actively modified when uevent is sent. During
>the module creation KOBJ_ADD modifies 'kobj->state_add_uevent_sent'. Until
>recent commit, kernel didn't modify KOBJ after sending LIVE notification.
>
>> > This commit breaks some of the projects that rely on the LIVE notification to
>> > monitor when the newly loaded module is ready.
>>
>> Hm, could you please explain specifically what is the issue you're seeing?
>> What projects is it breaking?
>>
>
>I'm specifically referencing these projects which are tracking kernel modules
>for integrity purpose (e.g. anti-rootkit tools) like Linux Kernel Runtime
>Guard.
>It is possible to modify these tools to adopt and take into account
>modification of KOBJ after LIVE state notification. However, from my
>understanding of the kernel modules notifiers logic, KOBJ should be fully
>formed at this stage.

Ah I see, thanks for the clarification. I was under the impression
that kobj->state_add_uevent_sent is an internal field interesting
only to lib/kobject.c, and did not know tools like you mention are
implicitly tracking that.

In any case, I think your suggested change doesn't affect the
systemd/udev issue we were trying to fix, as long as the uevent is
sent after module init(). Would you mind sending a patch? (with a
Fixes: tag and including the explanations you've provided in this
thread in the changelog)

Thanks!

Jessica

Re: Linux Kernel module notification bug

[Greg Kroah-Hartman]

On Tue, Jan 12, 2021 at 11:46:55AM +0100, Jessica Yu wrote:
> +++ Adam Zabrocki [12/01/21 01:15 +0100]:
> > Hello,
> > 
> > On Mon, Jan 11, 2021 at 03:20:48PM +0100, Jessica Yu wrote:
> > > +++ Adam Zabrocki [10/01/21 18:54 +0100]:
> > > > Hello,
> > > >
> > > > I believe that the following commit does introduce a gentle "functionality
> > > > bug":
> > > >
> > > > "module: delay kobject uevent until after module init call":
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/kernel/module.c?id=38dc717e97153e46375ee21797aa54777e5498f3
> > > >
> > > > The official Linux Kernel API for the kernel module activities notification has
> > > > been divided based on the readiness 'stage' for such module. We have the
> > > > following stages:
> > > >
> > > >        MODULE_STATE_LIVE,      /* Normal state. */
> > > >        MODULE_STATE_COMING,    /* Full formed, running module_init. */
> > > >        MODULE_STATE_GOING,     /* Going away. */
> > > >        MODULE_STATE_UNFORMED,  /* Still setting it up. */
> > > >
> > > > LIVE means that the kernel module is correctly running and all initialization
> > > > work has been already done. Otherwise, we have event 'COMING' or 'UNFORMED'.
> > > >
> > > > In the described commit, creation of the KOBJECT has been moved after invoking
> > > > a notficiation of the newly formed kernel module state (LIVE). That's somehow
> > > > inconsistent from my understanding of the kernel modules notifiers logic.
> > > > Creation of the new objects (like KOBJ) should be done before notification of
> > > > the stage LIVE is invoked.
> > > 
> > > I'm confused. We're not creating any kobjects here. That is all done
> > > in mod_sysfs_setup(), which is called while the module is still
> > > COMING.  What that commit does is delay telling userspace about the
> > > module (specifically, systemd/udev) until the module is basically
> > > ready. Systemd was basically receiving the uevent too early, before
> > > the module has initialized, hence we decided to delay the uevent [1].
> > > 
> > 
> > Sorry for the confusion on my side. I was referring to the internal state of
> > the KOBJ itself which is being actively modified when uevent is sent. During
> > the module creation KOBJ_ADD modifies 'kobj->state_add_uevent_sent'. Until
> > recent commit, kernel didn't modify KOBJ after sending LIVE notification.
> > 
> > > > This commit breaks some of the projects that rely on the LIVE notification to
> > > > monitor when the newly loaded module is ready.
> > > 
> > > Hm, could you please explain specifically what is the issue you're seeing?
> > > What projects is it breaking?
> > > 
> > 
> > I'm specifically referencing these projects which are tracking kernel modules
> > for integrity purpose (e.g. anti-rootkit tools) like Linux Kernel Runtime
> > Guard.
> > It is possible to modify these tools to adopt and take into account
> > modification of KOBJ after LIVE state notification. However, from my
> > understanding of the kernel modules notifiers logic, KOBJ should be fully
> > formed at this stage.
> 
> Ah I see, thanks for the clarification. I was under the impression
> that kobj->state_add_uevent_sent is an internal field interesting
> only to lib/kobject.c, and did not know tools like you mention are
> implicitly tracking that.

There is no in-kernel tools/users tracking stuff like this, so this is
not an issue.  The internals of kobjects can change at any time, there
is no "stable api" here at all.

thanks,

greg k-h

[PATCH] module: invoke kobject uevent before sending LIVE notification

[Adam Zabrocki]

The recent change "module: delay kobject uevent until after module init
call", while helping avoid a race between udev/systemd and the module
loader, made it unnecessarily more difficult to monitor kernel module
integrity by out-of-tree projects such as Linux Kernel Runtime Guard.

Specifically, that change delayed the kobject uevent unnecessarily too far,
to until after sending a MODULE_STATE_LIVE notification.  As the uevent
modifies internal state of the KOBJ itself, this violated the assumption
(non-guaranteed yet handy while we can maintain it) that the KOBJ remains
consistent and can be integrity-checked as soon as the module is LIVE.

To make all of these projects happy at once, move the kobject KOBJ_ADD
uevent to just before sending the MODULE_STATE_LIVE notification.

Fixes: 38dc717e9715 ("module: delay kobject uevent until after module init call")
Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl>
---
 kernel/module.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..7d56b1b07237 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3681,14 +3681,14 @@ static noinline int do_init_module(struct module *mod)
                dump_stack();
        }

+       /* Delay uevent until module has finished its init routine */
+       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
+
        /* Now it's a first class citizen! */
        mod->state = MODULE_STATE_LIVE;
        blocking_notifier_call_chain(&module_notify_list,
                                     MODULE_STATE_LIVE, mod);

-       /* Delay uevent until module has finished its init routine */
-       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
-
        /*
         * We need to finish all async code before the module init sequence
         * is done.  This has potential to deadlock.  For example, a newly

Re: [PATCH] module: invoke kobject uevent before sending LIVE notification

[Greg Kroah-Hartman]

On Wed, Jan 13, 2021 at 01:33:10AM +0100, Adam Zabrocki wrote:
> The recent change "module: delay kobject uevent until after module init
> call", while helping avoid a race between udev/systemd and the module
> loader, made it unnecessarily more difficult to monitor kernel module
> integrity by out-of-tree projects such as Linux Kernel Runtime Guard.

We don't support out-of-tree kernel code, sorry.

> Specifically, that change delayed the kobject uevent unnecessarily too far,
> to until after sending a MODULE_STATE_LIVE notification.  As the uevent
> modifies internal state of the KOBJ itself, this violated the assumption
> (non-guaranteed yet handy while we can maintain it) that the KOBJ remains
> consistent and can be integrity-checked as soon as the module is LIVE.
> 
> To make all of these projects happy at once, move the kobject KOBJ_ADD
> uevent to just before sending the MODULE_STATE_LIVE notification.
> 
> Fixes: 38dc717e9715 ("module: delay kobject uevent until after module init call")
> Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl>
> ---
>  kernel/module.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 4bf30e4b3eaa..7d56b1b07237 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3681,14 +3681,14 @@ static noinline int do_init_module(struct module *mod)
>                 dump_stack();
>         }
> 
> +       /* Delay uevent until module has finished its init routine */
> +       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
> +
>         /* Now it's a first class citizen! */
>         mod->state = MODULE_STATE_LIVE;
>         blocking_notifier_call_chain(&module_notify_list,
>                                      MODULE_STATE_LIVE, mod);
> 
> -       /* Delay uevent until module has finished its init routine */
> -       kobject_uevent(&mod->mkobj.kobj, KOBJ_ADD);
> -

No, the code is correct as-is, userspace should be told _after_ the
kernel itself has handled all of the needed housekeeping of the module
being added.

so consider this:

Nacked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>