Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <YidDznCPSmFmfNwE@iki.fi>
References: <YidDznCPSmFmfNwE@iki.fi> <20210712170313.884724-1-mic@digikod.net> <20210712170313.884724-6-mic@digikod.net>
To:     Jarkko Sakkinen <jarkko@kernel.org>
Cc:     dhowells@redhat.com,
        =?us-ascii?Q?=3D=3Fiso-8859-1=3FQ=3FMicka=3DEBl?=
         =?us-ascii?Q?=5FSala=3DFCn=3F=3D?= <mic@digikod.net>,
        David Woodhouse <dwmw2@infradead.org>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Snowberg <eric.snowberg@oracle.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        James Morris <jmorris@namei.org>,
        =?us-ascii?Q?=3D=3Fiso-8859-1=3FQ=3FMicka=3DEBl?=
         =?us-ascii?Q?=5FSala=3DFCn=3F=3D?= <mic@linux.microsoft.com>,
        Mimi Zohar <zohar@linux.ibm.com>,
        "Serge E . Hallyn" <serge@hallyn.com>,
        Tyler Hicks <tyhicks@linux.microsoft.com>,
        keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
        linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org
Subject: Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date:   Wed, 30 Mar 2022 14:44:00 +0100
Message-ID: <2937432.1648647840@warthog.procyon.org.uk>
Precedence: bulk

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> >  /*
> >   * Initialise the blacklist
> >   */
> >  static int __init blacklist_init(void)
> >  {
> >  	const char *const *bl;
> > +	struct key_restriction *restriction;
> >=20=20
> >  	if (register_key_type(&key_type_blacklist) < 0)
> >  		panic("Can't allocate system blacklist key type\n");
> >=20=20
> > +	restriction =3D kzalloc(sizeof(*restriction), GFP_KERNEL);
> > +	if (!restriction)
> > +		panic("Can't allocate blacklist keyring restriction\n");
>=20
>=20
> This prevents me from taking this to my pull request. In moderns standard=
s,
> 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=C3=ABl 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.
>=20
> This should rather be e.g.
>=20
>         restriction =3D 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 t=
hat
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