On Wed, 2023-10-11 at 17:47 +0530, Sumit Garg wrote:
On Wed, 11 Oct 2023 at 16:04, Jarkko Sakkinen jarkko@kernel.org wrote:
On Wed, 2023-10-11 at 13:12 +0300, Jarkko Sakkinen wrote:
On Wed, 2023-10-11 at 11:27 +0530, Sumit Garg wrote:
On Wed, 11 Oct 2023 at 04:46, Jarkko Sakkinen jarkko@kernel.org wrote:
Do bind neither static calls nor trusted_key_exit() before a successful init, in order to maintain a consistent state. In addition, depart the init_trusted() in the case of a real error (i.e. getting back something else than -ENODEV).
Reported-by: Linus Torvalds torvalds@linux-foundation.org Closes: https://lore.kernel.org/linux-integrity/CAHk-=whOPoLaWM8S8GgoOPT7a2+nMH5h3TL... Cc: stable@vger.kernel.org # v5.13+ Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework") Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
security/keys/trusted-keys/trusted_core.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c index 85fb5c22529a..fee1ab2c734d 100644 --- a/security/keys/trusted-keys/trusted_core.c +++ b/security/keys/trusted-keys/trusted_core.c @@ -358,17 +358,17 @@ static int __init init_trusted(void) if (!get_random) get_random = kernel_get_random;
- 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); - trusted_key_exit = trusted_key_sources[i].ops->exit; - migratable = trusted_key_sources[i].ops->migratable;
ret = trusted_key_sources[i].ops->init(); - if (!ret) + 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);
+ trusted_key_exit = trusted_key_sources[i].ops->exit; + migratable = trusted_key_sources[i].ops->migratable; + }
+ if (!ret || ret != -ENODEV)
As mentioned in the other thread, we should allow other trust sources to be initialized if the primary one fails.
I sent the patch before I received that response but here's what you wrote:
"We should give other trust sources a chance to register for trusted keys if the primary one fails."
- This condition is lacking an inline comment.
- Neither this response or the one that you pointed out has any
explanation why for any system failure the process should continue.
You should really know the situations (e.g. list of posix error code) when the process can continue and "allow list" those. This way way too abstract. It cannot be let all possible system failures pass.
And it would nice if it printed out something for legit cases. Like "no device found" etc. And for rest it must really withdraw the whole process.
IMO, it would be quite tricky to come up with an allow list. Can we keep "EACCES", "EPERM", "ENOTSUPP" etc in that allow list? I think these are all debatable.
Yes, that does sounds reasonable.
About the debate. Well, it is better eagerly block and tree falls down somewhere we can consider extending the list through a fix.
This all wide open is worse than a few glitches somewhere, which are trivial to fix.
BR, Jarkko