Re: [PATCH v8 16/19] ima: Enable re-auditing of modified files

From: Stefan Berger <stefanb@linux.ibm.com>
To: Stefan Berger <stefanb@linux.vnet.ibm.com>,
	linux-integrity@vger.kernel.org
Cc: zohar@linux.ibm.com, 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
Subject: Re: [PATCH v8 16/19] ima: Enable re-auditing of modified files
Date: Wed, 5 Jan 2022 10:21:38 -0500	[thread overview]
Message-ID: <be083959-b1ef-9de7-0ea6-71ab36596523@linux.ibm.com> (raw)
In-Reply-To: <20220104170416.1923685-17-stefanb@linux.vnet.ibm.com>


On 1/4/22 12:04, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Walk the list of ns_status associated with an iint if the file has
> changed and reset the IMA_AUDITED flag, which is part of the
> IMA_DONE_MASK. This causes a new audit message to be emitted when the
> file is again accessed on either the host or in an IMA namespace.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   security/integrity/ima/ima_main.c | 33 ++++++++++++++++++++++++++++++-
>   1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 99dc984b49c9..bc3ab08f39c6 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -153,6 +153,35 @@ static void ima_rdwr_violation_check(struct ima_namespace *ns,
>   				  "invalid_pcr", "open_writers");
>   }
>   
> +#ifdef CONFIG_IMA_NS
> +
> +static void mask_iint_ns_status_flags(struct integrity_iint_cache *iint,
> +				      unsigned long mask)
> +{
> +	struct ns_status *status;
> +	unsigned long flags;
> +
> +	read_lock(&iint->ns_list_lock);
> +	list_for_each_entry(status, &iint->ns_list, ns_next) {
> +		flags = iint_flags(iint, status) & mask;
> +		set_iint_flags(iint, status, flags);
> +	}
> +	read_unlock(&iint->ns_list_lock);
> +}
> +
> +#else
> +
> +static void mask_iint_ns_status_flags(struct integrity_iint_cache *iint,
> +				      unsigned long mask)
> +{
> +	unsigned long flags;
> +
> +	flags = iint_flags(iint, NULL) & mask;
> +	set_iint_flags(iint, NULL, flags);
> +}
> +
> +#endif
> +

The above two cases are due to this here:

diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..201a9d46d6e1 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -138,6 +138,10 @@ struct integrity_iint_cache {
  	enum integrity_status ima_creds_status:4;
  	enum integrity_status evm_status:4;
  	struct ima_digest_data *ima_hash;
+#ifdef CONFIG_IMA_NS
+	rwlock_t ns_list_lock;
+	struct list_head ns_list;
+#endif
  };

Ideally namespaced and non-namespaced code cases would share the code and to be able to share it the above #ifdef CONFIG_IMA_NS  in integrity.hshouldn't be there but we would have the lock and list in IMA namespacing and non-namespacing case. The above two code cases shown above are just the beginning and as more variables are moved from the iint into the ns_status these types of 'two cases of code' would show up in more places. So I think we should prevent this already now.

To be able to share the code we need an ns_status on a list in the non-namespacing case as well. In the non-namespacing case it would always be a single ns_status on the list. What is worth a decision is how to get the ns_status on the list. One idea would be to conditionally embed it into the integrity_iint_cache like this:

/* integrity data associated with an inode */
struct integrity_iint_cache {
         struct rb_node rb_node; /* rooted in integrity_iint_tree */
         struct mutex mutex;     /* protects: version, flags, digest */
         struct inode *inode;    /* back pointer to inode in question */
         u64 version;            /* track inode changes */
         unsigned long flags;
         unsigned long atomic_flags;
         enum integrity_status ima_file_status:4;
         enum integrity_status ima_mmap_status:4;
         enum integrity_status ima_bprm_status:4;
         enum integrity_status ima_read_status:4;
         enum integrity_status ima_creds_status:4;
         enum integrity_status evm_status:4;
         struct ima_digest_data *ima_hash;
         rwlock_t ns_list_lock;
         struct list_head ns_list;
#ifndef CONFIG_IMA_NS
	struct ns_status status;
#endif
};

This would prevent a 2nd cache just for allocation of ns_status in the 
non-namespacing case and getting the  embedded ns_status onto the list 
would also be like this:

     INIT_LIST_HEAD(&iint->ns_list);

#ifndef CONFIG_IMA_NS

     INIT_LIST_HEAD(&iint->status.ns_next);

     list_add_tail(&iint->status.ns_next, &iint->ns_list);

#endif

The other option is to allocated the ns_status via a minimal 
implementation of ima_ns_status.c for the non-namespaced case using 
kmalloc's from a cache for ns_status structures.


Also, the new 'rwlock_t ns_list_lock' in the iint would really only be 
necessary for locking in the namespacing case. However, to be able to 
share the code we would need to keep this lock around for the 
non-namespacing case as well so that we can call read_lock() in both 
cases. An option would be to introduce a macro for the locking that is a 
no-op in the non-namespacing case and does the actual locking in the 
namespacing case. I am not sure what would be better. I would prefer to 
explicitly see the read_lock()...



>   static void ima_check_last_writer(struct integrity_iint_cache *iint,
>   				  struct inode *inode, struct file *file)
>   {
> @@ -169,8 +198,10 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>   		if (!IS_I_VERSION(inode) ||
>   		    !inode_eq_iversion(inode, iint->version) ||
>   		    (iint->flags & IMA_NEW_FILE)) {
> -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> +			mask_iint_ns_status_flags(iint,
> +					~(IMA_DONE_MASK | IMA_NEW_FILE));
>   			iint->measured_pcrs = 0;
> +
>   			if (update)
>   				ima_update_xattr(iint, file);
>   		}
