| Message ID | 20220311174741.250424-2-mic@digikod.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove panic() from keyring init calls | expand |
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; > }
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
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; } /*