Received-SPF: pass (google.com: domain of linux-kernel+bounces-170952-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <e6e2b31983610bf566d8046edbac0e00c8453233.camel@huaweicloud.com>
Subject: Re: [RFC][PATCH] ima: Use sequence number to wait for policy updates
From: Roberto Sassu <roberto.sassu@huaweicloud.com>
To: zohar@linux.ibm.com, dmitry.kasatkin@gmail.com,
 eric.snowberg@oracle.com,  paul@paul-moore.com, jmorris@namei.org,
 serge@hallyn.com,  john.johansen@canonical.com,
 stephen.smalley.work@gmail.com,  casey@schaufler-ca.com, eparis@redhat.com
Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, 
	linux-kernel@vger.kernel.org, guozihua@huawei.com, omosnace@redhat.com, 
	audit@vger.kernel.org, apparmor@lists.ubuntu.com, selinux@vger.kernel.org, 
	Roberto Sassu <roberto.sassu@huawei.com>
Date: Tue, 07 May 2024 11:32:03 +0200
In-Reply-To: <20240507092831.3590793-1-roberto.sassu@huaweicloud.com>
References: <20240507092831.3590793-1-roberto.sassu@huaweicloud.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
Precedence: bulk
MIME-Version: 1.0

On Tue, 2024-05-07 at 11:28 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
>=20
> Maintain a global sequence number, and set it to individual policy rules,
> when they are created.

Just did an attempt, to see if this path is viable.

This patch would be an alternative to:

[PATCH v3] ima: Avoid blocking in RCU read-side critical section

Roberto

> When a rule is stale, wait for the global sequence number to increase,
> which happens when the LSM policy has been fully updated.
>=20
> Finally, restart the ima_match_policy() loop, which should not encounter
> the stale rule anymore.
>=20
> On policy update, already increase the rule sequence number, so that a wa=
it
> can be implemented for the next policy update.
>=20
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_policy.c | 65 +++++++++++++++--------------
>  1 file changed, 33 insertions(+), 32 deletions(-)
>=20
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima=
/ima_policy.c
> index c0556907c2e6..4f8275e216fe 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -51,9 +51,12 @@
>  #define INVALID_PCR(a) (((a) < 0) || \
>  	(a) >=3D (sizeof_field(struct ima_iint_cache, measured_pcrs) * 8))
> =20
> +DECLARE_WAIT_QUEUE_HEAD(wait_queue_t);
> +
>  int ima_policy_flag;
>  static int temp_ima_appraise;
>  static int build_ima_appraise __ro_after_init;
> +static int global_seqno;
> =20
>  atomic_t ima_setxattr_allowed_hash_algorithms;
> =20
> @@ -122,6 +125,7 @@ struct ima_rule_entry {
>  	struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyr=
ings */
>  	struct ima_rule_opt_list *label; /* Measure data grouped under this lab=
el */
>  	struct ima_template_desc *template;
> +	int seqno;
>  };
> =20
>  /*
> @@ -442,6 +446,8 @@ static int ima_lsm_update_rule(struct ima_rule_entry =
*entry)
>  	if (!nentry)
>  		return -ENOMEM;
> =20
> +	nentry->seqno++;
> +
>  	list_replace_rcu(&entry->list, &nentry->list);
>  	synchronize_rcu();
>  	/*
> @@ -497,6 +503,8 @@ int ima_lsm_policy_change(struct notifier_block *nb, =
unsigned long event,
>  		return NOTIFY_DONE;
> =20
>  	ima_lsm_update_rules();
> +	global_seqno++;
> +	wake_up(&wait_queue_t);
>  	return NOTIFY_OK;
>  }
> =20
> @@ -560,18 +568,16 @@ static bool ima_match_rule_data(struct ima_rule_ent=
ry *rule,
>   * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC=
)
>   * @func_data: func specific data, may be NULL
>   *
> - * Returns true on rule match, false on failure.
> + * Returns 1 on rule match, 0 on mismatch, -ESTALE on stale policy.
>   */
> -static bool ima_match_rules(struct ima_rule_entry *rule,
> -			    struct mnt_idmap *idmap,
> -			    struct inode *inode, const struct cred *cred,
> -			    u32 secid, enum ima_hooks func, int mask,
> -			    const char *func_data)
> +static int ima_match_rules(struct ima_rule_entry *rule,
> +			   struct mnt_idmap *idmap,
> +			   struct inode *inode, const struct cred *cred,
> +			   u32 secid, enum ima_hooks func, int mask,
> +			   const char *func_data)
>  {
>  	int i;
> -	bool result =3D false;
>  	struct ima_rule_entry *lsm_rule =3D rule;
> -	bool rule_reinitialized =3D false;
> =20
>  	if ((rule->flags & IMA_FUNC) &&
>  	    (rule->func !=3D func && func !=3D POST_SETATTR))
> @@ -642,7 +648,6 @@ static bool ima_match_rules(struct ima_rule_entry *ru=
le,
>  				return false;
>  		}
> =20
> -retry:
>  		switch (i) {
>  		case LSM_OBJ_USER:
>  		case LSM_OBJ_ROLE:
> @@ -663,27 +668,13 @@ static bool ima_match_rules(struct ima_rule_entry *=
rule,
>  			break;
>  		}
> =20
> -		if (rc =3D=3D -ESTALE && !rule_reinitialized) {
> -			lsm_rule =3D ima_lsm_copy_rule(rule);
> -			if (lsm_rule) {
> -				rule_reinitialized =3D true;
> -				goto retry;
> -			}
> -		}
> -		if (!rc) {
> -			result =3D false;
> -			goto out;
> -		}
> +		if (!rc)
> +			return false;
> +		else if (rc =3D=3D -ESTALE)
> +			return rc;
>  	}
> -	result =3D true;
> =20
> -out:
> -	if (rule_reinitialized) {
> -		for (i =3D 0; i < MAX_LSM_RULES; i++)
> -			ima_filter_rule_free(lsm_rule->lsm[i].rule);
> -		kfree(lsm_rule);
> -	}
> -	return result;
> +	return true;
>  }
> =20
>  /*
> @@ -741,12 +732,12 @@ int ima_match_policy(struct mnt_idmap *idmap, struc=
t inode *inode,
>  		     const char *func_data, unsigned int *allowed_algos)
>  {
>  	struct ima_rule_entry *entry;
> -	int action =3D 0, actmask =3D flags | (flags << 1);
> +	int action =3D 0, rc, actmask =3D flags | (flags << 1);
>  	struct list_head *ima_rules_tmp;
> =20
>  	if (template_desc && !*template_desc)
>  		*template_desc =3D ima_template_desc_current();
> -
> +retry:
>  	rcu_read_lock();
>  	ima_rules_tmp =3D rcu_dereference(ima_rules);
>  	list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
> @@ -754,9 +745,18 @@ int ima_match_policy(struct mnt_idmap *idmap, struct=
 inode *inode,
>  		if (!(entry->action & actmask))
>  			continue;
> =20
> -		if (!ima_match_rules(entry, idmap, inode, cred, secid,
> -				     func, mask, func_data))
> +		rc =3D ima_match_rules(entry, idmap, inode, cred, secid,
> +				     func, mask, func_data);
> +		if (!rc)
>  			continue;
> +		else if (rc =3D=3D -ESTALE) {
> +			rcu_read_unlock();
> +
> +			wait_event_interruptible(wait_queue_t,
> +				(global_seqno =3D=3D entry->seqno + 1));
> +
> +			goto retry;
> +		}
> =20
>  		action |=3D entry->flags & IMA_NONACTION_FLAGS;
> =20
> @@ -1153,6 +1153,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry =
*entry,
>  			result =3D 0;
>  	}
> =20
> +	entry->seqno =3D global_seqno;
>  	return result;
>  }
> =20