Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <5e4a862917785972281bbcb483404da01b71e801.camel@linux.ibm.com>
Subject: Re: [PATCH v10 26/27] ima: Limit number of policy rules in
 non-init_ima_ns
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Stefan Berger <stefanb@linux.ibm.com>,
        linux-integrity@vger.kernel.org
Cc:     serge@hallyn.com, christian.brauner@ubuntu.com,
        containers@lists.linux.dev, dmitry.kasatkin@gmail.com,
        ebiederm@xmission.com, krzysztof.struczynski@huawei.com,
        roberto.sassu@huawei.com, mpeters@redhat.com, lhinds@redhat.com,
        lsturman@redhat.com, puiterwi@redhat.com, jejb@linux.ibm.com,
        jamjoom@us.ibm.com, linux-kernel@vger.kernel.org,
        paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
Date:   Wed, 23 Feb 2022 10:38:39 -0500
In-Reply-To: <20220201203735.164593-27-stefanb@linux.ibm.com>
References: <20220201203735.164593-1-stefanb@linux.ibm.com>
         <20220201203735.164593-27-stefanb@linux.ibm.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Limit the number of policy rules one can set in non-init_ima_ns to a
> hardcoded 1024 rules. If the user attempts to exceed this limit by
> setting too many additional rules, emit an audit message with the cause
> 'too-many-rules' and simply ignore the newly added rules.

This paragraph describes 'what' you're doing, not 'why'.  Please prefix
this paragraph with a short, probably one sentence, reason for the
change.
> 
> Switch the accounting for the memory allocated for IMA policy rules to
> GFP_KERNEL_ACCOUNT so that cgroups kernel memory accounting takes effect.

Does this change affect the existing IMA policy rules for init_ima_ns?

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  security/integrity/ima/ima_fs.c     | 20 ++++++++++++++------
>  security/integrity/ima/ima_policy.c | 23 ++++++++++++++++++-----
>  2 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 58d80884880f..cd102bbd4677 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -410,24 +410,32 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>  {
>  	struct ima_namespace *ns = &init_ima_ns;
>  	const char *cause = ns->valid_policy ? "completed" : "failed";
> +	int err = 0;
>  
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
>  		return seq_release(inode, file);
>  
> -	if (ns->valid_policy && ima_check_policy(ns) < 0) {
> -		cause = "failed";
> -		ns->valid_policy = 0;
> +	if (ns->valid_policy) {
> +		err = ima_check_policy(ns);
> +		if (err < 0) {
> +			if (err == -ENOSPC)

Perhaps "EDQUOT" would be more appropriate.

> +				cause = "too-many-rules";
> +			else
> +				cause = "failed";
> +			ns->valid_policy = 0;
> +		}
>  	}
>  
>  	pr_info("policy update %s\n", cause);
> -	integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> -			    "policy_update", cause, !ns->valid_policy, 0);
> +	integrity_audit_message(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> +				"policy_update", cause, !ns->valid_policy, 0,
> +				-err);

The 'err' value is already properly set.  No need for the minus sign.

>  
>  	if (!ns->valid_policy) {
>  		ima_delete_rules(ns);
>  		ns->valid_policy = 1;
>  		clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
> -		return 0;
> +		return err;
>  	}
>  
>  	ima_update_policy(ns);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index acb4c36e539f..3f84d04f101d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -305,7 +305,8 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	opt_list = kzalloc(struct_size(opt_list, items, count), GFP_KERNEL);
> +	opt_list = kzalloc(struct_size(opt_list, items, count),
> +			   GFP_KERNEL_ACCOUNT);
>  	if (!opt_list) {
>  		kfree(src_copy);
>  		return ERR_PTR(-ENOMEM);
> @@ -379,7 +380,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_namespace *ns,
>  	 * Immutable elements are copied over as pointers and data; only
>  	 * lsm rules can change
>  	 */
> -	nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL);
> +	nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL_ACCOUNT);
>  	if (!nentry)
>  		return NULL;
>  
> @@ -826,7 +827,7 @@ static void add_rules(struct ima_namespace *ns,
>  
>  		if (policy_rule & IMA_CUSTOM_POLICY) {
>  			entry = kmemdup(&entries[i], sizeof(*entry),
> -					GFP_KERNEL);
> +					GFP_KERNEL_ACCOUNT);
>  			if (!entry)
>  				continue;
>  
> @@ -863,7 +864,7 @@ static int __init ima_init_arch_policy(struct ima_namespace *ns)
>  
>  	ns->arch_policy_entry = kcalloc(arch_entries + 1,
>  					sizeof(*ns->arch_policy_entry),
> -					GFP_KERNEL);
> +					GFP_KERNEL_ACCOUNT);
>  	if (!ns->arch_policy_entry)
>  		return 0;
>  
> @@ -975,8 +976,20 @@ void __init ima_init_policy(struct ima_namespace *ns)
>  /* Make sure we have a valid policy, at least containing some rules. */
>  int ima_check_policy(struct ima_namespace *ns)
>  {
> +	struct ima_rule_entry *entry;
> +	size_t len1 = 0;
> +	size_t len2 = 0;
> +
>  	if (list_empty(&ns->ima_temp_rules))
>  		return -EINVAL;
> +	if (ns != &init_ima_ns) {
> +		list_for_each_entry(entry, &ns->ima_temp_rules, list)
> +			len1++;
> +		list_for_each_entry(entry, &ns->ima_policy_rules, list)

Using list_for_each_entry() is fine here, because multiple policy
updates at the same time are blocked.  Please add a comment.

> +			len2++;
> +		if (len1 + len2 > 1024)

One variable should be enough.

> +			return -ENOSPC;
> +	}
>  	return 0;
>  }
>  
> @@ -1848,7 +1861,7 @@ ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule)
>  	if (*p == '#' || *p == '\0')
>  		return len;
>  
> -	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +	entry = kzalloc(sizeof(*entry), GFP_KERNEL_ACCOUNT);
>  	if (!entry) {
>  		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
>  				    NULL, op, "-ENOMEM", -ENOMEM, audit_info);

-- 
thanks,

Mimi