KEYS: trusted: Rollback init_trusted() consistently

  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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
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(-)
  

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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> 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.

-Sumit

>                         break;
>         }
>
> --
> 2.39.2
>
  

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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > 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."

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
  

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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > 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."
> 
> 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.

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.

BR, Jarkko
  

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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > 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."
> >
> > 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.
>
> 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.

The original intention to iterate through the trust source list was to
allow a single Linux kernel binary to support platforms with varying
trust sources (one or choose one from multiple) available. IMO, any
restriction on the basis of error codes here since we can't predict
them correctly may forfeit this intended use-case.

-Sumit

>
> BR, Jarkko
  

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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > 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."
> > > 
> > > 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.
> > 
> > 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
  

On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > 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."
> > > >
> > > > 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.
> > >
> > > 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.
>

Fair enough, I would suggest we document it appropriately such that it
is clear to the users or somebody looking at the code.

-Sumit

> BR, Jarkko
  

On Wed, 2023-10-11 at 18:25 +0530, Sumit Garg wrote:
> On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > > 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."
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > 
> 
> Fair enough, I would suggest we document it appropriately such that it
> is clear to the users or somebody looking at the code.

I went throught the backends on how they implement init:

1. Returns -ENODEV when it does not exist.
2. Calls driver_register(). Something is wrong enough if that
   fails to rollback the whole procedure.
3. TPM: -ENODEV

Therefore, I would keep in the existing patch since there is no weird
uapi visible legacy behavior to support in the first place. And for
that reason there is no good reason to have all those four POSIX rc's
in the list.

BR, Jarkko
  

On Wed, 2023-10-11 at 16:06 +0300, Jarkko Sakkinen wrote:
> On Wed, 2023-10-11 at 18:25 +0530, Sumit Garg wrote:
> > On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > > > 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."
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > 
> > 
> > Fair enough, I would suggest we document it appropriately such that it
> > is clear to the users or somebody looking at the code.
> 
> I went throught the backends on how they implement init:
> 
> 
CAAM:
> 1. Returns -ENODEV when it does not exist.
TEE:
> 2. Calls driver_register(). Something is wrong enough if that
>    fails to rollback the whole procedure.
> 3. TPM: -ENODEV

BR, Jarkko
  

On Wed, 11 Oct 2023 at 18:36, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, 2023-10-11 at 18:25 +0530, Sumit Garg wrote:
> > On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > >
> > > 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > > > 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."
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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.
> > >
> >
> > Fair enough, I would suggest we document it appropriately such that it
> > is clear to the users or somebody looking at the code.
>
> I went throught the backends on how they implement init:
>
> 1. Returns -ENODEV when it does not exist.
> 2. Calls driver_register(). Something is wrong enough if that
>    fails to rollback the whole procedure.
> 3. TPM: -ENODEV
>
> Therefore, I would keep in the existing patch since there is no weird
> uapi visible legacy behavior to support in the first place. And for
> that reason there is no good reason to have all those four POSIX rc's
> in the list.

Okay I can live with this patch as long as it doesn't break the
intended use-case.

-Sumit

>
> BR, Jarkko
>
>
  

On Wed, 2023-10-11 at 19:12 +0530, Sumit Garg wrote:
> On Wed, 11 Oct 2023 at 18:36, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Wed, 2023-10-11 at 18:25 +0530, Sumit Garg wrote:
> > > On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > 
> > > > 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > > > > 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."
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > 
> > > 
> > > Fair enough, I would suggest we document it appropriately such that it
> > > is clear to the users or somebody looking at the code.
> > 
> > I went throught the backends on how they implement init:
> > 
> > 1. Returns -ENODEV when it does not exist.
> > 2. Calls driver_register(). Something is wrong enough if that
> >    fails to rollback the whole procedure.
> > 3. TPM: -ENODEV
> > 
> > Therefore, I would keep in the existing patch since there is no weird
> > uapi visible legacy behavior to support in the first place. And for
> > that reason there is no good reason to have all those four POSIX rc's
> > in the list.
> 
> Okay I can live with this patch as long as it doesn't break the
> intended use-case.

Well this sort of policy has been already existing for some time:

	/*
	 * encrypted_keys.ko depends on successful load of this module even if
	 * trusted key implementation is not found.
	 */
	if (ret == -ENODEV)
		return 0;

If we would need a list of error codes, then this is also incorrect
implementation because the error codes that you listed should be
also success cases.

BR, Jarkko
  

On Wed, 2023-10-11 at 16:55 +0300, Jarkko Sakkinen wrote:
> On Wed, 2023-10-11 at 19:12 +0530, Sumit Garg wrote:
> > On Wed, 11 Oct 2023 at 18:36, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > On Wed, 2023-10-11 at 18:25 +0530, Sumit Garg wrote:
> > > > On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > > 
> > > > > 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > > > > > 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."
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > > 
> > > > Fair enough, I would suggest we document it appropriately such that it
> > > > is clear to the users or somebody looking at the code.
> > > 
> > > I went throught the backends on how they implement init:
> > > 
> > > 1. Returns -ENODEV when it does not exist.
> > > 2. Calls driver_register(). Something is wrong enough if that
> > >    fails to rollback the whole procedure.
> > > 3. TPM: -ENODEV
> > > 
> > > Therefore, I would keep in the existing patch since there is no weird
> > > uapi visible legacy behavior to support in the first place. And for
> > > that reason there is no good reason to have all those four POSIX rc's
> > > in the list.
> > 
> > Okay I can live with this patch as long as it doesn't break the
> > intended use-case.
> 
> Well this sort of policy has been already existing for some time:
> 
>         /*
>          * encrypted_keys.ko depends on successful load of this module even if
>          * trusted key implementation is not found.
>          */
>         if (ret == -ENODEV)
>                 return 0;
> 
> If we would need a list of error codes, then this is also incorrect
> implementation because the error codes that you listed should be
> also success cases.

The dead obvious constraint here is that whatever error codes are
processed they need to be exact same anyway right?

If things fall apart you should really not continue. This is IMHO
categorizes as a critical bug, not just debatable aspect on how
subsystems are engineered. I.e.I do not consider this as any sort
of API discussion per se.

BR, Jarkko
  

On Wed, 11 Oct 2023 at 19:35, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, 2023-10-11 at 16:55 +0300, Jarkko Sakkinen wrote:
> > On Wed, 2023-10-11 at 19:12 +0530, Sumit Garg wrote:
> > > On Wed, 11 Oct 2023 at 18:36, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > >
> > > > On Wed, 2023-10-11 at 18:25 +0530, Sumit Garg wrote:
> > > > > On Wed, 11 Oct 2023 at 18:07, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > > >
> > > > > > 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+nMH5h3TLKtn=R_3w4R1_Uvg@mail.gmail.com/
> > > > > > > > > > > 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."
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > 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.
> > > > > >
> > > > >
> > > > > Fair enough, I would suggest we document it appropriately such that it
> > > > > is clear to the users or somebody looking at the code.
> > > >
> > > > I went throught the backends on how they implement init:
> > > >
> > > > 1. Returns -ENODEV when it does not exist.
> > > > 2. Calls driver_register(). Something is wrong enough if that
> > > >    fails to rollback the whole procedure.
> > > > 3. TPM: -ENODEV
> > > >
> > > > Therefore, I would keep in the existing patch since there is no weird
> > > > uapi visible legacy behavior to support in the first place. And for
> > > > that reason there is no good reason to have all those four POSIX rc's
> > > > in the list.
> > >
> > > Okay I can live with this patch as long as it doesn't break the
> > > intended use-case.
> >
> > Well this sort of policy has been already existing for some time:
> >
> >         /*
> >          * encrypted_keys.ko depends on successful load of this module even if
> >          * trusted key implementation is not found.
> >          */
> >         if (ret == -ENODEV)
> >                 return 0;
> >
> > If we would need a list of error codes, then this is also incorrect
> > implementation because the error codes that you listed should be
> > also success cases.
>

As I mentioned before we can go ahead with this policy for trust
sources and see how it pans out.

> The dead obvious constraint here is that whatever error codes are
> processed they need to be exact same anyway right?
>
> If things fall apart you should really not continue. This is IMHO
> categorizes as a critical bug,

Here we are discussing trust sources as multiple independent devices.
If a particular device probe/init fails then it shouldn't be a blocker
to probe/init another device.

> not just debatable aspect on how
> subsystems are engineered. I.e.I do not consider this as any sort
> of API discussion per se.

Agree, I see it as a policy decision for the trusted keys subsystem.

-Sumit

>
> BR, Jarkko