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

[Ahmad Fatoum]

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@xxxxxxxxxx> 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@xxxxxxxxx>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>>
>>>> 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@xxxxxxxxxx/
> 
> 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
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |