Messages in this thread | | | From | Paul Moore <> | Date | Fri, 11 Mar 2022 17:00:32 -0500 | Subject | Re: [PATCH v1 1/2] certs: Remove panic() calls from blacklist_init() |
| |
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
| |