* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-06 5:18 [PATCH v2] KEYS: trusted: Remove redundant static calls usage Sumit Garg
@ 2023-10-06 5:56 ` Hyeonggon Yoo
2023-10-10 12:33 ` Jarkko Sakkinen
2023-10-10 18:28 ` Linus Torvalds
2 siblings, 0 replies; 12+ messages in thread
From: Hyeonggon Yoo @ 2023-10-06 5:56 UTC (permalink / raw)
To: Sumit Garg
Cc: torvalds, jarkko, peterz, zohar, linux-kernel, linux-integrity,
jejb, David.Kaplan, bp, mingo, x86, regressions
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-06 5:18 [PATCH v2] KEYS: trusted: Remove redundant static calls usage Sumit Garg
2023-10-06 5:56 ` Hyeonggon Yoo
@ 2023-10-10 12:33 ` Jarkko Sakkinen
2023-10-10 13:14 ` Sumit Garg
2023-10-10 18:28 ` Linus Torvalds
2 siblings, 1 reply; 12+ messages in thread
From: Jarkko Sakkinen @ 2023-10-10 12:33 UTC (permalink / raw)
To: Sumit Garg, torvalds, peterz, zohar
Cc: linux-kernel, linux-integrity, jejb, David.Kaplan, bp, mingo, x86,
regressions, Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 12:33 ` Jarkko Sakkinen
@ 2023-10-10 13:14 ` Sumit Garg
2023-10-10 13:49 ` Jarkko Sakkinen
0 siblings, 1 reply; 12+ messages in thread
From: Sumit Garg @ 2023-10-10 13:14 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: torvalds, peterz, zohar, linux-kernel, linux-integrity, jejb,
David.Kaplan, bp, mingo, x86, regressions, Hyeonggon Yoo
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 13:14 ` Sumit Garg
@ 2023-10-10 13:49 ` Jarkko Sakkinen
2023-10-10 14:19 ` Ahmad Fatoum
0 siblings, 1 reply; 12+ messages in thread
From: Jarkko Sakkinen @ 2023-10-10 13:49 UTC (permalink / raw)
To: Sumit Garg
Cc: torvalds, peterz, zohar, linux-kernel, linux-integrity, jejb,
David.Kaplan, bp, mingo, x86, regressions, Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 13:49 ` Jarkko Sakkinen
@ 2023-10-10 14:19 ` Ahmad Fatoum
2023-10-10 14:31 ` Jarkko Sakkinen
0 siblings, 1 reply; 12+ messages in thread
From: Ahmad Fatoum @ 2023-10-10 14:19 UTC (permalink / raw)
To: Jarkko Sakkinen, Sumit Garg
Cc: torvalds, peterz, zohar, linux-kernel, linux-integrity, jejb,
David.Kaplan, bp, mingo, x86, regressions, Hyeonggon Yoo
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
>
>
--
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 |
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 14:19 ` Ahmad Fatoum
@ 2023-10-10 14:31 ` Jarkko Sakkinen
0 siblings, 0 replies; 12+ messages in thread
From: Jarkko Sakkinen @ 2023-10-10 14:31 UTC (permalink / raw)
To: Ahmad Fatoum, Sumit Garg
Cc: torvalds, peterz, zohar, linux-kernel, linux-integrity, jejb,
David.Kaplan, bp, mingo, x86, regressions, Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-06 5:18 [PATCH v2] KEYS: trusted: Remove redundant static calls usage Sumit Garg
2023-10-06 5:56 ` Hyeonggon Yoo
2023-10-10 12:33 ` Jarkko Sakkinen
@ 2023-10-10 18:28 ` Linus Torvalds
2023-10-10 19:05 ` Jarkko Sakkinen
2023-10-11 5:52 ` Sumit Garg
2 siblings, 2 replies; 12+ messages in thread
From: Linus Torvalds @ 2023-10-10 18:28 UTC (permalink / raw)
To: Sumit Garg, David Howells
Cc: jarkko, peterz, zohar, linux-kernel, linux-integrity, jejb,
David.Kaplan, bp, mingo, x86, regressions, Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 18:28 ` Linus Torvalds
@ 2023-10-10 19:05 ` Jarkko Sakkinen
2023-10-10 19:07 ` Jarkko Sakkinen
2023-10-11 5:54 ` Sumit Garg
2023-10-11 5:52 ` Sumit Garg
1 sibling, 2 replies; 12+ messages in thread
From: Jarkko Sakkinen @ 2023-10-10 19:05 UTC (permalink / raw)
To: Linus Torvalds, Sumit Garg, David Howells
Cc: peterz, zohar, linux-kernel, linux-integrity, jejb, David.Kaplan,
bp, mingo, x86, regressions, Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 19:05 ` Jarkko Sakkinen
@ 2023-10-10 19:07 ` Jarkko Sakkinen
2023-10-11 5:54 ` Sumit Garg
1 sibling, 0 replies; 12+ messages in thread
From: Jarkko Sakkinen @ 2023-10-10 19:07 UTC (permalink / raw)
To: Linus Torvalds, Sumit Garg, David Howells
Cc: peterz, zohar, linux-kernel, linux-integrity, jejb, David.Kaplan,
bp, mingo, x86, regressions, Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 19:05 ` Jarkko Sakkinen
2023-10-10 19:07 ` Jarkko Sakkinen
@ 2023-10-11 5:54 ` Sumit Garg
1 sibling, 0 replies; 12+ messages in thread
From: Sumit Garg @ 2023-10-11 5:54 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Linus Torvalds, David Howells, peterz, zohar, linux-kernel,
linux-integrity, jejb, David.Kaplan, bp, mingo, x86, regressions,
Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] KEYS: trusted: Remove redundant static calls usage
2023-10-10 18:28 ` Linus Torvalds
2023-10-10 19:05 ` Jarkko Sakkinen
@ 2023-10-11 5:52 ` Sumit Garg
1 sibling, 0 replies; 12+ messages in thread
From: Sumit Garg @ 2023-10-11 5:52 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, jarkko, peterz, zohar, linux-kernel,
linux-integrity, jejb, David.Kaplan, bp, mingo, x86, regressions,
Hyeonggon Yoo
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
^ permalink raw reply [flat|nested] 12+ messages in thread