linux-kernel - Re: [PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring

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] [day] [month] [year] [list]

Message-ID: <YjeXgEbUE+X7bshR@iki.fi>
Date:   Sun, 20 Mar 2022 23:07:12 +0200
From:   Jarkko Sakkinen <jarkko@...nel.org>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     David Howells <dhowells@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Snowberg <eric.snowberg@...cle.com>,
        Paul Moore <paul@...l-moore.com>, keyrings@...r.kernel.org,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] certs: Remove panic() calls from
 system_trusted_keyring_init()

On Thu, Mar 17, 2022 at 09:31:10AM +0100, Mickaël Salaün wrote:
> 
> On 17/03/2022 09:30, Mickaël Salaün wrote:
> > 
> > On 17/03/2022 08:36, Jarkko Sakkinen wrote:
> > > On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
> > > > From: Mickaël Salaün <mic@...ux.microsoft.com>
> > > > 
> > > > Replace panic() calls from device_initcall(system_trusted_keyring_init)
> > > > with proper error handling using -ENODEV.
> > > > 
> > > > Suggested-by: Jarkko Sakkinen <jarkko@...nel.org> [1]
> > > > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> > > > Signed-off-by: Mickaël Salaün <mic@...ux.microsoft.com>
> > > > Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
> > > > ---
> > > >   certs/system_keyring.c | 26 ++++++++++++++++++++------
> > > >   1 file changed, 20 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > > index 05b66ce9d1c9..428046a7aa7f 100644
> > > > --- a/certs/system_keyring.c
> > > > +++ b/certs/system_keyring.c
> > > > @@ -148,8 +148,10 @@ static __init int system_trusted_keyring_init(void)
> > > >                     KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
> > > >                     KEY_ALLOC_NOT_IN_QUOTA,
> > > >                     NULL, NULL);
> > > > -    if (IS_ERR(builtin_trusted_keys))
> > > > -        panic("Can't allocate builtin trusted keyring\n");
> > > > +    if (IS_ERR(builtin_trusted_keys)) {
> > > > +        pr_err("Can't allocate builtin trusted keyring\n");
> > > > +        return -ENODEV;
> > > > +    }
> > > >   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > > >       secondary_trusted_keys =
> > > > @@ -161,14 +163,26 @@ static __init int
> > > > system_trusted_keyring_init(void)
> > > >                     KEY_ALLOC_NOT_IN_QUOTA,
> > > >                     get_builtin_and_secondary_restriction(),
> > > >                     NULL);
> > > > -    if (IS_ERR(secondary_trusted_keys))
> > > > -        panic("Can't allocate secondary trusted keyring\n");
> > > > +    if (IS_ERR(secondary_trusted_keys)) {
> > > > +        pr_err("Can't allocate secondary trusted keyring\n");
> > > > +        goto err_secondary;
> > > > +    }
> > > > -    if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
> > > > -        panic("Can't link trusted keyrings\n");
> > > > +    if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
> > > > +        pr_err("Can't link trusted keyrings\n");
> > > > +        goto err_link;
> > > > +    }
> > > >   #endif
> > > >       return 0;
> > > > +
> > > > +err_link:
> > > > +    key_put(secondary_trusted_keys);
> > > > +
> > > > +err_secondary:
> > > > +    key_put(builtin_trusted_keys);
> > > > +
> > > > +    return -ENODEV;
> > > >   }
> > > >   /*
> > > > -- 
> > > > 2.35.1
> > > > 
> > > 
> > > Changes make sense to me but you should implement all this to the
> > > original
> > > patch set.
> 
> Do you mean to squash these two patches together?

You could squash function documentation to the corresponding
patch addressing the use of panic() so that we know why things
are done against normal status quo.

BR, Jarkko