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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, 2023-10-10 at 18:44 +0530, Sumit Garg wrote:
> On Tue, 10 Oct 2023 at 18:03, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Fri, 2023-10-06 at 10:48 +0530, Sumit Garg 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.
> > > 
> > > Note here that although it will fix the current crash report, ultimately
> > > the static call infrastructure should be fixed to either support its
> > > future usage from module __init and __exit functions or not.
> > > 
> > > Reported-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> > > Link: https://lore.kernel.org/lkml/ZRhKq6e5nF%2F4ZIV1@fedora/#t
> > > Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > ---
> > > 
> > > Changes in v2:
> > > - Polish commit message as per comments from Mimi
> > > 
> > >  security/keys/trusted-keys/trusted_core.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> > > index c6fc50d67214..85fb5c22529a 100644
> > > --- a/security/keys/trusted-keys/trusted_core.c
> > > +++ b/security/keys/trusted-keys/trusted_core.c
> > > @@ -44,13 +44,12 @@ static const struct trusted_key_source trusted_key_sources[] = {
> > >  #endif
> > >  };
> > > 
> > > -DEFINE_STATIC_CALL_NULL(trusted_key_init, *trusted_key_sources[0].ops->init);
> > >  DEFINE_STATIC_CALL_NULL(trusted_key_seal, *trusted_key_sources[0].ops->seal);
> > >  DEFINE_STATIC_CALL_NULL(trusted_key_unseal,
> > >                         *trusted_key_sources[0].ops->unseal);
> > >  DEFINE_STATIC_CALL_NULL(trusted_key_get_random,
> > >                         *trusted_key_sources[0].ops->get_random);
> > > -DEFINE_STATIC_CALL_NULL(trusted_key_exit, *trusted_key_sources[0].ops->exit);
> > > +static void (*trusted_key_exit)(void);
> > >  static unsigned char migratable;
> > > 
> > >  enum {
> > > @@ -359,19 +358,16 @@ static int __init init_trusted(void)
> > >                 if (!get_random)
> > >                         get_random = kernel_get_random;
> > > 
> > > -               static_call_update(trusted_key_init,
> > > -                                  trusted_key_sources[i].ops->init);
> > >                 static_call_update(trusted_key_seal,
> > >                                    trusted_key_sources[i].ops->seal);
> > >                 static_call_update(trusted_key_unseal,
> > >                                    trusted_key_sources[i].ops->unseal);
> > >                 static_call_update(trusted_key_get_random,
> > >                                    get_random);
> > > -               static_call_update(trusted_key_exit,
> > > -                                  trusted_key_sources[i].ops->exit);
> > > +               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;
> > >         }
> > > @@ -388,7 +384,8 @@ static int __init init_trusted(void)
> > > 
> > >  static void __exit cleanup_trusted(void)
> > >  {
> > > -       static_call_cond(trusted_key_exit)();
> > > +       if (trusted_key_exit)
> > > +               (*trusted_key_exit)();
> > >  }
> > > 
> > >  late_initcall(init_trusted);
> > 
> > Would it be less confusing to require trusted_key_exit from each?
> > 
> 
> It is already required for each trust source to provide exit callback
> but this NULL check was added via this fix [1] in case there isn't any
> trust source present.
> 
> [1] https://lkml.kernel.org/stable/20220126184155.220814-1-dave.kleikamp@oracle.com/

I'd considering creating a placeholder trusted_key_default_exit() with
perhaps pr_debug() statement acknowledging it getting called.

Hmm.. if we had that I wonder if we could get away with __weak... Then
you would not need to assign anything. This is not through-out analyzed.
Tbh I'm not sure how module loader handles this type of scenario but
at least the placeholder function would make sense in any case.

If abusing weak symbols was in-fact possible probably then the whole
idea of using static_call could be thrown to garbage bin but there's
now a lot of context here related on how module loader works linux
that I'm ignoring...

BR, Jarkko