linux-kernel - Re: [PATCH] KEYS: trusted: Rollback init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <186a4b62517ead88df8c3c0e9e9585e88f9a6fd8.camel@kernel.org>
Date:   Wed, 11 Oct 2023 13:12:02 +0300
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Sumit Garg <sumit.garg@...aro.org>
Cc:     keyrings@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        stable@...r.kernel.org, James Bottomley <jejb@...ux.ibm.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        David Howells <dhowells@...hat.com>,
        Paul Moore <paul@...l-moore.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        "open list:KEYS-TRUSTED" <linux-integrity@...r.kernel.org>,
        "open list:SECURITY SUBSYSTEM" 
        <linux-security-module@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KEYS: trusted: Rollback init_trusted() consistently

On Wed, 2023-10-11 at 11:27 +0530, Sumit Garg wrote:
> On Wed, 11 Oct 2023 at 04:46, Jarkko Sakkinen <jarkko@...nel.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@...ux-foundation.org>
> > Closes: https://lore.kernel.org/linux-integrity/CAHk-=whOPoLaWM8S8GgoOPT7a2+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > Cc: stable@...r.kernel.org # v5.13+
> > Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.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."

1. This condition is lacking an inline comment.
2. 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.

Can you e.g. explain a legit use case when something else is
returned than -ENODEV but it is cool and we can continue in 
some real world use case?

BR, Jarkko