[PATCH v1 0/2] Remove panic() from keyring init calls

[PATCH v1 0/2] Remove panic() from keyring init calls

[Mickaël Salaün]

As suggested by Jarkko [1], let's remove the panic() calls from the
keyring initializations.  This series applies on top of commit
c9e54f38976a ("integrity: Only use machine keyring when
uefi_check_trust_mok_keys is true"), which also includes 50c486fe3108
("certs: Allow root user to append signed hashes to the blacklist
keyring").

[1] https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi
[2] https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=c9e54f38976a1c0ec69c0a6208b3fd55fceb01d1

Regards,

Mickaël Salaün (2):
  certs: Remove panic() calls from blacklist_init()
  certs: Remove panic() calls from system_trusted_keyring_init()

 certs/blacklist.c      | 27 +++++++++++++++++++++------
 certs/system_keyring.c | 26 ++++++++++++++++++++------
 2 files changed, 41 insertions(+), 12 deletions(-)


base-commit: c9e54f38976a1c0ec69c0a6208b3fd55fceb01d1
-- 
2.35.1

[PATCH v1 1/2] certs: Remove panic() calls from blacklist_init()

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Replace panic() calls from device_initcall(blacklist_init) with proper
error handling using -ENODEV.

Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20220311174741.250424-2-mic@digikod.net
---
 certs/blacklist.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 486ce0dd8e9c..ea7a77f156da 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -313,12 +313,16 @@ static int __init blacklist_init(void)
 	const char *const *bl;
 	struct key_restriction *restriction;
 
-	if (register_key_type(&key_type_blacklist) < 0)
-		panic("Can't allocate system blacklist key type\n");
+	if (register_key_type(&key_type_blacklist) < 0) {
+		pr_err("Can't allocate system blacklist key type\n");
+		return -ENODEV;
+	}
 
 	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
-	if (!restriction)
-		panic("Can't allocate blacklist keyring restriction\n");
+	if (!restriction) {
+		pr_err("Can't allocate blacklist keyring restriction\n");
+		goto err_restriction;
+	}
 	restriction->check = restrict_link_for_blacklist;
 
 	blacklist_keyring =
@@ -333,13 +337,24 @@ static int __init blacklist_init(void)
 			      , KEY_ALLOC_NOT_IN_QUOTA |
 			      KEY_ALLOC_SET_KEEP,
 			      restriction, NULL);
-	if (IS_ERR(blacklist_keyring))
-		panic("Can't allocate system blacklist keyring\n");
+	if (IS_ERR(blacklist_keyring)) {
+		pr_err("Can't allocate system blacklist keyring\n");
+		goto err_keyring;
+	}
 
 	for (bl = blacklist_hashes; *bl; bl++)
 		if (mark_raw_hash_blacklisted(*bl) < 0)
 			pr_err("- blacklisting failed\n");
 	return 0;
+
+
+err_keyring:
+	kfree(restriction);
+
+err_restriction:
+	unregister_key_type(&key_type_blacklist);
+
+	return -ENODEV;
 }
 
 /*
-- 
2.35.1

Re: [PATCH v1 1/2] certs: Remove panic() calls from blacklist_init()

[Paul Moore]

On Fri, Mar 11, 2022 at 12:47 PM Mickaël Salaün <mic@digikod.net> wrote:
> From: Mickaël Salaün <mic@linux.microsoft.com>
>
> Replace panic() calls from device_initcall(blacklist_init) with proper
> error handling using -ENODEV.
>
> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20220311174741.250424-2-mic@digikod.net
> ---
>  certs/blacklist.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)

I'm not sure we can safely rely on a non-zero error code saving us in
the care of failure, can we?

The blacklist_init() function is registered as an initcall via
device_initcall() which I believe is either executed via
do_init_module() in the case of a dynamic module load, or via
do_initcalls() if built into the kernel.  In either case the result is
that the module/functionality doesn't load and the kernel continues on
executing.  While this could be acceptable for some non-critical
modules, if this particular module fails to load it defeats the
certificate/key based deny list for signed modules, yes?

I completely understand the strong desire to purge the kernel of
panic()s, BUG()s, and the like, but if a critical piece of security
functionality that users expect to be present fails to initialize,
panic()ing is likely the right thing to do.

> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 486ce0dd8e9c..ea7a77f156da 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -313,12 +313,16 @@ static int __init blacklist_init(void)
>         const char *const *bl;
>         struct key_restriction *restriction;
>
> -       if (register_key_type(&key_type_blacklist) < 0)
> -               panic("Can't allocate system blacklist key type\n");
> +       if (register_key_type(&key_type_blacklist) < 0) {
> +               pr_err("Can't allocate system blacklist key type\n");
> +               return -ENODEV;
> +       }
>
>         restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> -       if (!restriction)
> -               panic("Can't allocate blacklist keyring restriction\n");
> +       if (!restriction) {
> +               pr_err("Can't allocate blacklist keyring restriction\n");
> +               goto err_restriction;
> +       }
>         restriction->check = restrict_link_for_blacklist;
>
>         blacklist_keyring =
> @@ -333,13 +337,24 @@ static int __init blacklist_init(void)
>                               , KEY_ALLOC_NOT_IN_QUOTA |
>                               KEY_ALLOC_SET_KEEP,
>                               restriction, NULL);
> -       if (IS_ERR(blacklist_keyring))
> -               panic("Can't allocate system blacklist keyring\n");
> +       if (IS_ERR(blacklist_keyring)) {
> +               pr_err("Can't allocate system blacklist keyring\n");
> +               goto err_keyring;
> +       }
>
>         for (bl = blacklist_hashes; *bl; bl++)
>                 if (mark_raw_hash_blacklisted(*bl) < 0)
>                         pr_err("- blacklisting failed\n");
>         return 0;
> +
> +
> +err_keyring:
> +       kfree(restriction);
> +
> +err_restriction:
> +       unregister_key_type(&key_type_blacklist);
> +
> +       return -ENODEV;
>  }

-- 
paul-moore.com

Re: [PATCH v1 1/2] certs: Remove panic() calls from blacklist_init()

[Jarkko Sakkinen]

On Fri, Mar 11, 2022 at 05:00:32PM -0500, Paul Moore wrote:
> On Fri, Mar 11, 2022 at 12:47 PM Mickaël Salaün <mic@digikod.net> wrote:
> > From: Mickaël Salaün <mic@linux.microsoft.com>
> >
> > Replace panic() calls from device_initcall(blacklist_init) with proper
> > error handling using -ENODEV.
> >
> > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > Link: https://lore.kernel.org/r/20220311174741.250424-2-mic@digikod.net
> > ---
> >  certs/blacklist.c | 27 +++++++++++++++++++++------
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> I'm not sure we can safely rely on a non-zero error code saving us in
> the care of failure, can we?
> 
> The blacklist_init() function is registered as an initcall via
> device_initcall() which I believe is either executed via
> do_init_module() in the case of a dynamic module load, or via
> do_initcalls() if built into the kernel.  In either case the result is
> that the module/functionality doesn't load and the kernel continues on
> executing.  While this could be acceptable for some non-critical
> modules, if this particular module fails to load it defeats the
> certificate/key based deny list for signed modules, yes?
> 
> I completely understand the strong desire to purge the kernel of
> panic()s, BUG()s, and the like, but if a critical piece of security
> functionality that users expect to be present fails to initialize,
> panic()ing is likely the right thing to do.

OK, I get this. 

What this function should have is this information documented in
the header. Otherwise, this is just confusing.

BR, Jarkko

[PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring_init()

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Replace panic() calls from device_initcall(system_trusted_keyring_init)
with proper error handling using -ENODEV.

Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
Signed-off-by: Mickaël Salaün <mic@linux.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

Re: [PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring_init()

[Jarkko Sakkinen]

On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
> From: Mickaël Salaün <mic@linux.microsoft.com>
> 
> Replace panic() calls from device_initcall(system_trusted_keyring_init)
> with proper error handling using -ENODEV.
> 
> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> Signed-off-by: Mickaël Salaün <mic@linux.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.

BR, Jarkko

Re: [PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring_init()

[Mickaël Salaün]

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@linux.microsoft.com>
>>
>> Replace panic() calls from device_initcall(system_trusted_keyring_init)
>> with proper error handling using -ENODEV.
>>
>> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
>> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
>> Signed-off-by: Mickaël Salaün <mic@linux.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.

You agreed to add this patch on top of the others a few days ago: 
https://lore.kernel.org/r/f8b1ea77afe8d6698b4a2122254ff8be310412b1.camel@kernel.org

What do you think about Paul's concerns?

Re: [PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring_init()

[Mickaël Salaün]

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@linux.microsoft.com>
>>>
>>> Replace panic() calls from device_initcall(system_trusted_keyring_init)
>>> with proper error handling using -ENODEV.
>>>
>>> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
>>> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
>>> Signed-off-by: Mickaël Salaün <mic@linux.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 agreed to add this patch on top of the others a few days ago: 
> https://lore.kernel.org/r/f8b1ea77afe8d6698b4a2122254ff8be310412b1.camel@kernel.org 
> 
> 
> What do you think about Paul's concerns?

Re: [PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring_init()

[Jarkko Sakkinen]

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@linux.microsoft.com>
> > > > 
> > > > Replace panic() calls from device_initcall(system_trusted_keyring_init)
> > > > with proper error handling using -ENODEV.
> > > > 
> > > > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> > > > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> > > > Signed-off-by: Mickaël Salaün <mic@linux.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

Re: [PATCH v1 2/2] certs: Remove panic() calls from system_trusted_keyring_init()

[Jarkko Sakkinen]

On Thu, Mar 17, 2022 at 09:30:02AM +0100, 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@linux.microsoft.com>
> > > 
> > > Replace panic() calls from device_initcall(system_trusted_keyring_init)
> > > with proper error handling using -ENODEV.
> > > 
> > > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> > > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> > > Signed-off-by: Mickaël Salaün <mic@linux.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.
> 
> You agreed to add this patch on top of the others a few days ago: https://lore.kernel.org/r/f8b1ea77afe8d6698b4a2122254ff8be310412b1.camel@kernel.org
> 
> What do you think about Paul's concerns?

Yes, but I missed this part.

I think the right call would be to include Paul's concerns documented.

BR, Jarkko