Re: [PATCH v6 25/25] security: Enforce ordering of 'ima' and 'evm' LSMs

From: Casey Schaufler <casey@schaufler-ca.com>
To: Roberto Sassu <roberto.sassu@huaweicloud.com>,
	viro@zeniv.linux.org.uk, brauner@kernel.org,
	chuck.lever@oracle.com, jlayton@kernel.org, neilb@suse.de,
	kolga@netapp.com, Dai.Ngo@oracle.com, tom@talpey.com,
	paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com,
	zohar@linux.ibm.com, dmitry.kasatkin@gmail.com,
	dhowells@redhat.com, jarkko@kernel.org,
	stephen.smalley.work@gmail.com, eparis@parisplace.org,
	mic@digikod.net
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nfs@vger.kernel.org, linux-security-module@vger.kernel.org,
	linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
	selinux@vger.kernel.org, Roberto Sassu <roberto.sassu@huawei.com>,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v6 25/25] security: Enforce ordering of 'ima' and 'evm' LSMs
Date: Mon, 20 Nov 2023 16:50:56 -0800	[thread overview]
Message-ID: <24a9f95d-6a28-47d3-a0cf-48e1698e2445@schaufler-ca.com> (raw)
In-Reply-To: <20231120173318.1132868-26-roberto.sassu@huaweicloud.com>

On 11/20/2023 9:33 AM, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> The ordering of LSM_ORDER_LAST LSMs depends on how they are placed in the
> .lsm_info.init section of the kernel image.
>
> Without making any assumption on the LSM ordering based on how they are
> compiled, enforce that ordering at LSM infrastructure level.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/security.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/security/security.c b/security/security.c
> index 351a124b771c..b98db79ca500 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -263,6 +263,18 @@ static void __init initialize_lsm(struct lsm_info *lsm)
>  	}
>  }
>  
> +/* Find an LSM with a given name. */
> +static struct lsm_info __init *find_lsm(const char *name)
> +{
> +	struct lsm_info *lsm;
> +
> +	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++)
> +		if (!strcmp(lsm->name, name))
> +			return lsm;
> +
> +	return NULL;
> +}
> +
>  /*
>   * Current index to use while initializing the lsm id list.
>   */
> @@ -333,10 +345,23 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
>  
>  	/* LSM_ORDER_LAST is always last. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
> +		/* Do it later, to enforce the expected ordering. */
> +		if (!strcmp(lsm->name, "ima") || !strcmp(lsm->name, "evm"))
> +			continue;
> +

Hard coding the ordering of LSMs is incredibly ugly and unlikely to scale.
Not to mention perplexing the next time someone creates an LSM that "has to be last".

Why isn't LSM_ORDER_LAST sufficient? If it really isn't, how about adding
and using LSM_ORDER_LAST_I_REALLY_MEAN_IT* ?

Alternatively, a declaration of ordering requirements with regard to other
LSMs in lsm_info. You probably don't care where ima is relative to Yama,
but you need to be after SELinux and before evm. lsm_info could have 
must_precede and must_follow lists. Maybe a must_not_combine list, too,
although I'm hoping to make that unnecessary. 

And you should be using LSM_ID values instead of LSM names.

---
* Naming subject to Paul's sensibilities, of course.

>  		if (lsm->order == LSM_ORDER_LAST)
>  			append_ordered_lsm(lsm, "   last");
>  	}
>  
> +	/* Ensure that the 'ima' and 'evm' LSMs are last and in this order. */
> +	lsm = find_lsm("ima");
> +	if (lsm)
> +		append_ordered_lsm(lsm, "   last");
> +
> +	lsm = find_lsm("evm");
> +	if (lsm)
> +		append_ordered_lsm(lsm, "   last");
> +
>  	/* Disable all LSMs not in the ordered list. */
>  	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
>  		if (exists_ordered_lsm(lsm))