2023-10-05 14:29:26

by Sumit Garg

Subject: [PATCH] KEYS: trusted: Remove redundant static calls usage

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() doesn't adds any value neither from performance
point and nor there is any security benefit. Hence switch to use indirect
function calls instead.

Note here that although it will fix the current crash reported. But
ultimately we need fix up static calls infrastructure to either support
its future usage from module __init and __exit functions or not.

Reported-by: Hyeonggon Yoo <[email protected]>
Closes: https://lore.kernel.org/lkml/ZRhKq6e5nF%2F4ZIV1@fedora/#t
Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
Signed-off-by: Sumit Garg <[email protected]>
---
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

2023-10-05 17:07:22

by Mimi Zohar

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

Hi Summit,

On Thu, 2023-10-05 at 19:03 +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.

Split the above paragraph into two sentences.

> However, the usage of static call invocations for trusted_key_init()
> and trusted_key_exit() doesn't adds any value neither from performance
> point and nor there is any security benefit.

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 reported. But
> ultimately we need fix up static calls infrastructure to either support
> its future usage from module __init and __exit functions or not.

The first line is a sentence fragment. Please replace the period with
a comma.

report, ultimately the static call call infrastructure should ...

>
> Reported-by: Hyeonggon Yoo <[email protected]>
> Closes: https://lore.kernel.org/lkml/ZRhKq6e5nF%2F4ZIV1@fedora/#t

Replace "Closes" with "Link".

> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> Signed-off-by: Sumit Garg <[email protected]>

--
thanks,

Mimi