On Fri, Mar 11, 2022 at 12:47 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > > Replace panic() calls from device_initcall(blacklist_init) with proper > error handling using -ENODEV. > > Suggested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx> [1] > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@xxxxxx [1] > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > Link: https://lore.kernel.org/r/20220311174741.250424-2-mic@xxxxxxxxxxx > --- > 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
