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 |
