Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys

From: Mimi Zohar <zohar@linux.ibm.com>
To: Eric Snowberg <eric.snowberg@oracle.com>,
	keyrings@vger.kernel.org, linux-integrity@vger.kernel.org,
	dhowells@redhat.com, dwmw2@infradead.org,
	herbert@gondor.apana.org.au, davem@davemloft.net,
	jarkko@kernel.org, jmorris@namei.org, serge@hallyn.com
Cc: keescook@chromium.org, gregkh@linuxfoundation.org,
	torvalds@linux-foundation.org, scott.branden@broadcom.com,
	weiyongjun1@huawei.com, nayna@linux.ibm.com, ebiggers@google.com,
	ardb@kernel.org, nramas@linux.microsoft.com, lszubowi@redhat.com,
	linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	James.Bottomley@HansenPartnership.com, pjones@redhat.com,
	glin@suse.com, konrad.wilk@oracle.com
Subject: Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
Date: Thu, 05 Aug 2021 09:58:16 -0400	[thread overview]
Message-ID: <6c751dadf4ce7385d0391ea26f1c7e4e910219e0.camel@linux.ibm.com> (raw)
In-Reply-To: <20210726171319.3133879-11-eric.snowberg@oracle.com>

On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:

> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index dcaf74102ab2..b27ae30eaadc 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -45,6 +45,15 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
>  				     const union key_payload *payload,
>  				     struct key *restriction_key)
>  {
> +	/* If the secondary trusted keyring is not enabled, we may link
> +	 * through to the mok keyring and the search may follow that link.
> +	 */

Refer to section "8) Commenting" of Documentation/process/coding-
style.rst for the format of multi line comments.

> +	if (mok_trusted_keys && type == &key_type_keyring &&
> +	    dest_keyring == builtin_trusted_keys &&
> +	    payload == &mok_trusted_keys->payload)
> +		/* Allow the mok keyring to be added to the builtin */
> +		return 0;
> +

Unless you're changing the meaning of the restriction, then a new
restriction needs to be defined.  In this case, please don't change the
meaning of restrict_link_by_builtin_trusted().  Instead define a new
restriction named restrict_link_by_builtin_and_ca_trusted().

>  	return restrict_link_by_signature(dest_keyring, type, payload,
>  					  builtin_trusted_keys);
>  }
> @@ -91,6 +100,15 @@ int restrict_link_by_builtin_and_secondary_trusted(
>  		/* Allow the builtin keyring to be added to the secondary */
>  		return 0;
>  
> +	/* If we have a secondary trusted keyring, it may contain a link
> +	 * through to the mok keyring and the search may follow that link.
> +	 */
> +	if (mok_trusted_keys && type == &key_type_keyring &&
> +	    dest_keyring == secondary_trusted_keys &&
> +	    payload == &mok_trusted_keys->payload)
> +		/* Allow the mok keyring to be added to the secondary */
> +		return 0;
> +

Similarly here, please define a new restriction maybe named
restrict_link_by_builtin_secondary_and_ca_trusted().   To avoid code
duplication, the new restriction could be a wrapper around the existing
function.

>  	return restrict_link_by_signature(dest_keyring, type, payload,
>  					  secondary_trusted_keys);
>  }
> @@ -321,5 +339,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  void __init set_mok_trusted_keys(struct key *keyring)
>  {
>  	mok_trusted_keys = keyring;
> +
> +	if (key_link(system_trusted_keys, mok_trusted_keys) < 0)
> +		panic("Can't link (mok) trusted keyrings\n");
>  }

From the thread discussion on 00/12:

Only the builtin keys should ever be on the builtin keyring.  The
builtin keyring would need to be linked to the mok keyring.  But in the
secondary keyring case, the mok keyring would be linked to the
secondary keyring, similar to how the builtin keyring is linked to the
secondary keyring.

        if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
                panic("Can't link trusted keyrings\n");

thanks,

Mimi

>  #endif