Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage

[Sumit Garg]

On Tue, 10 Oct 2023 at 23:59, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
> >
> > Static calls invocations aren't well supported from module __init and
> > __exit functions. Especially the static call from cleanup_trusted() led
> > to a crash on x86 kernel with CONFIG_DEBUG_VIRTUAL=y.
> >
> > However, the usage of static call invocations for trusted_key_init()
> > and trusted_key_exit() don't add any value from either a performance or
> > security perspective. Hence switch to use indirect function calls instead.
>
> I applied this patch to my tree, since it is a fix for the issue, and
> doesn't change any logic otherwise.

Thanks.

>
> However, I do note that the code logic is completely broken. It was
> broken before too, and apparently causes no problems, but it's still
> wrong.
>
> That's a separate issue, and would want a separate patch, but since I
> noticed it when applying this one, I'm replying here:
>
> > +               trusted_key_exit = trusted_key_sources[i].ops->exit;
> >                 migratable = trusted_key_sources[i].ops->migratable;
> >
> > -               ret = static_call(trusted_key_init)();
> > +               ret = trusted_key_sources[i].ops->init();
> >                 if (!ret)
> >                         break;
>
> Note how this sets "trusted_key_exit" even when the ->init() function fails.
>
> Then we potentially do the module exit:
>
> >  static void __exit cleanup_trusted(void)
> >  {
> > -       static_call_cond(trusted_key_exit)();
> > +       if (trusted_key_exit)
> > +               (*trusted_key_exit)();
> >  }
>
> With an exit function that doesn't match a successful init() call.
>
> Now, *normally* this isn't a problem, because if the init() call
> fails, we'll go on to the next one, and if they *all* fail, we'll fail
> the module load, and we obviously won't call the cleanup_trusted()
> function at all.
>
> EXCEPT.
>
> We have this:
>
>         /*
>          * encrypted_keys.ko depends on successful load of this module even if
>          * trusted key implementation is not found.
>          */
>         if (ret == -ENODEV)
>                 return 0;
>
> so that init() may actually have failed, and we still succeed in
> loading the module, and now we will call that exit function to clean
> up something that was never successfully done.

Here we consider -ENODEV as a success case since we don't want to
block encrypted keys module loading since it can use user key as
master key instead.

>
> This hopefully doesn't matter in practice, and the cleanup function
> will just not do anything, but it is illogical and inconsistent. So I
> think it should be fixed.

Agree as the exit function won't do anything without the device being
present but we should make it consistent.

-Sumit

> But as mentioned, this is a separate issue
> from the whole "you currently can't do static calls from __exit
> functions" issue.
>
>                   Linus