diff mbox series

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

Message ID 20231006051801.423973-1-sumit.garg@linaro.org
State New
Headers
Series [v2] KEYS: trusted: Remove redundant static calls usage |

Commit Message

Sumit Garg Oct. 6, 2023, 5:18 a.m. UTC 
  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(-)

Comments

Hyeonggon Yoo Oct. 6, 2023, 5:56 a.m. UTC | #1 
On Fri, Oct 6, 2023 at 2:18 PM Sumit Garg <sumit.garg@linaro.org> 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>

I verified that this patch fixes the original problem.
Thanks!

Feel free to add:
Tested-By: Hyeonggon Yoo <42.hyeyoo@gmail.com>


Hyeonggon

> ---
>
> 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);
> --
> 2.34.1
>
Jarkko Sakkinen Oct. 10, 2023, 12:33 p.m. UTC | #2 
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?

BR, Jarkko
Sumit Garg Oct. 10, 2023, 1:14 p.m. UTC | #3 
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/

-Sumit

> BR, Jarkko
>
Jarkko Sakkinen Oct. 10, 2023, 1:49 p.m. UTC | #4 
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
Ahmad Fatoum Oct. 10, 2023, 2:19 p.m. UTC | #5 
Hello Jarkko,

On 10.10.23 15:49, Jarkko Sakkinen wrote:
> 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 you define a default exit function as __weak and expect trusted key sources
to override it, you can only have one trust source at most in the compiled
kernel and no boot-time selection would be possible.

Cheers,
Ahmad

> 
> 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
> 
>
Jarkko Sakkinen Oct. 10, 2023, 2:31 p.m. UTC | #6 
On Tue, 2023-10-10 at 16:19 +0200, Ahmad Fatoum wrote:
> Hello Jarkko,
> 
> On 10.10.23 15:49, Jarkko Sakkinen wrote:
> > 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 you define a default exit function as __weak and expect trusted key sources
> to override it, you can only have one trust source at most in the compiled
> kernel and no boot-time selection would be possible.

Right, got it, thank you.

So, I still would consider trusted_key_default_exit() and assign that in the
declaration to trusted_exit.

BR, Jarkko
Linus Torvalds Oct. 10, 2023, 6:28 p.m. UTC | #7 
On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@linaro.org> 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.

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.

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. But as mentioned, this is a separate issue
from the whole "you currently can't do static calls from __exit
functions" issue.

                  Linus
Jarkko Sakkinen Oct. 10, 2023, 7:05 p.m. UTC | #8 
On Tue, 2023-10-10 at 11:28 -0700, Linus Torvalds wrote:
> On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@linaro.org> 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.
> 
> 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.

Sumit, can you remind me why this continues *on any failure*?

E.g. something like this would make more sense to me:

		ret = trusted_key_sources[i].ops->init();
		if (!ret) {
			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);
			migratable = trusted_key_sources[i].ops->migratable;
			break;
		}

		if (ret != -ENODEV)
			break;
`
BR, Jarkko
Jarkko Sakkinen Oct. 10, 2023, 7:07 p.m. UTC | #9 
On Tue, 2023-10-10 at 22:05 +0300, Jarkko Sakkinen wrote:
> On Tue, 2023-10-10 at 11:28 -0700, Linus Torvalds wrote:
> > On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@linaro.org> 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.
> > 
> > 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.
> 
> Sumit, can you remind me why this continues *on any failure*?
> 
> E.g. something like this would make more sense to me:
> 
>                 ret = trusted_key_sources[i].ops->init();
>                 if (!ret) {
>                         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);

Please ignore the line above :-)

BR, Jarkko
Sumit Garg Oct. 11, 2023, 5:52 a.m. UTC | #10 
On Tue, 10 Oct 2023 at 23:59, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@linaro.org> 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
Sumit Garg Oct. 11, 2023, 5:54 a.m. UTC | #11 
On Wed, 11 Oct 2023 at 00:35, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Tue, 2023-10-10 at 11:28 -0700, Linus Torvalds wrote:
> > On Thu, 5 Oct 2023 at 22:18, Sumit Garg <sumit.garg@linaro.org> 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.
> >
> > 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.
>
> Sumit, can you remind me why this continues *on any failure*?

We should give other trust sources a chance to register for trusted
keys if the primary one fails.

-Sumit

>
> E.g. something like this would make more sense to me:
>
>                 ret = trusted_key_sources[i].ops->init();
>                 if (!ret) {
>                         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);
>                         migratable = trusted_key_sources[i].ops->migratable;
>                         break;
>                 }
>
>                 if (ret != -ENODEV)
>                         break;
> `
> BR, Jarkko
diff mbox series

Patch

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);