Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[David Howells <dhowells@redhat.com>]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> >  /*
> >   * Initialise the blacklist
> >   */
> >  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");
> >  
> > +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > +	if (!restriction)
> > +		panic("Can't allocate blacklist keyring restriction\n");
> 
> 
> This prevents me from taking this to my pull request. In moderns standards,
> no new BUG_ON(), panic() etc. should never added to the kernel.

I would argue that in this case, though, it is reasonable.  This should only
be called during kernel initialisation and, as Mickaël points out, if you
can't allocate that small amount of memory, the kernel isn't going to boot
much further.

> I missed this in my review.
> 
> This should rather be e.g.
> 
>         restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> 	if (!restriction) {
> 		pr_err("Can't allocate blacklist keyring restriction\n");
>                 return 0;
>         }

You can't just return 0.  That indicates success - but if by some miracle, the
kernel actually gets to a point where userspace can happen, it could mean that
we're missing the security restrictions of the blacklist.

Now, we could defer the panic to add_key_to_revocation_list(), but if you
can't set in place the required security restrictions, I think it's arguable
that the kernel either needs to panic or it needs to blacklist everything.

David