| Message ID | 20231006051801.423973-1-sumit.garg@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KEYS: trusted: Remove redundant static calls usage | expand |
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 >
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
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 >
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
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 > >
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
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
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
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
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
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 --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);
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(-)