diff mbox series

[v3,2/9] ima: Always return a file measurement in ima_file_hash()

Message ID 20220302111404.193900-3-roberto.sassu@huawei.com (mailing list archive)
State Accepted
Commit 280fe8367b0dc45b6ac5e04fad03e16e99540c0c
Delegated to: BPF
Headers show
Series bpf-lsm: Extend interoperability with IMA | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf success VM_Test
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Roberto Sassu March 2, 2022, 11:13 a.m. UTC
__ima_inode_hash() checks if a digest has been already calculated by
looking for the integrity_iint_cache structure associated to the passed
inode.

Users of ima_file_hash() (e.g. eBPF) might be interested in obtaining the
information without having to setup an IMA policy so that the digest is
always available at the time they call this function.

In addition, they likely expect the digest to be fresh, e.g. recalculated
by IMA after a file write. Although getting the digest from the
bprm_committed_creds hook (as in the eBPF test) ensures that the digest is
fresh, as the IMA hook is executed before that hook, this is not always the
case (e.g. for the mmap_file hook).

Call ima_collect_measurement() in __ima_inode_hash(), if the file
descriptor is available (passed by ima_file_hash()) and the digest is not
available/not fresh, and store the file measurement in a temporary
integrity_iint_cache structure.

This change does not cause memory usage increase, due to using the
temporary integrity_iint_cache structure, and due to freeing the
ima_digest_data structure inside integrity_iint_cache before exiting from
__ima_inode_hash().

For compatibility reasons, the behavior of ima_inode_hash() remains
unchanged.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_main.c | 46 ++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 13 deletions(-)

Comments

Mimi Zohar March 6, 2022, 7:31 p.m. UTC | #1
On Wed, 2022-03-02 at 12:13 +0100, Roberto Sassu wrote:
> __ima_inode_hash() checks if a digest has been already calculated by
> looking for the integrity_iint_cache structure associated to the passed
> inode.
> 
> Users of ima_file_hash() (e.g. eBPF) might be interested in obtaining the
> information without having to setup an IMA policy so that the digest is
> always available at the time they call this function.
> 
> In addition, they likely expect the digest to be fresh, e.g. recalculated
> by IMA after a file write. Although getting the digest from the
> bprm_committed_creds hook (as in the eBPF test) ensures that the digest is
> fresh, as the IMA hook is executed before that hook, this is not always the
> case (e.g. for the mmap_file hook).
> 
> Call ima_collect_measurement() in __ima_inode_hash(), if the file
> descriptor is available (passed by ima_file_hash()) and the digest is not
> available/not fresh, and store the file measurement in a temporary
> integrity_iint_cache structure.
> 
> This change does not cause memory usage increase, due to using the
> temporary integrity_iint_cache structure, and due to freeing the
> ima_digest_data structure inside integrity_iint_cache before exiting from
> __ima_inode_hash().
> 
> For compatibility reasons, the behavior of ima_inode_hash() remains
> unchanged.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

The patch itself is fine, but with great hesitancy due to the existing
eBPF integrity gaps and how these functions are planned to be used,

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
diff mbox series

Patch

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 946ba8a12eab..ed1a82f1def3 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -520,20 +520,38 @@  int ima_file_check(struct file *file, int mask)
 }
 EXPORT_SYMBOL_GPL(ima_file_check);
 
-static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
+static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
+			    size_t buf_size)
 {
-	struct integrity_iint_cache *iint;
-	int hash_algo;
+	struct integrity_iint_cache *iint = NULL, tmp_iint;
+	int rc, hash_algo;
 
-	if (!ima_policy_flag)
-		return -EOPNOTSUPP;
+	if (ima_policy_flag) {
+		iint = integrity_iint_find(inode);
+		if (iint)
+			mutex_lock(&iint->mutex);
+	}
+
+	if ((!iint || !(iint->flags & IMA_COLLECTED)) && file) {
+		if (iint)
+			mutex_unlock(&iint->mutex);
+
+		memset(&tmp_iint, 0, sizeof(tmp_iint));
+		tmp_iint.inode = inode;
+		mutex_init(&tmp_iint.mutex);
+
+		rc = ima_collect_measurement(&tmp_iint, file, NULL, 0,
+					     ima_hash_algo, NULL);
+		if (rc < 0)
+			return -EOPNOTSUPP;
+
+		iint = &tmp_iint;
+		mutex_lock(&iint->mutex);
+	}
 
-	iint = integrity_iint_find(inode);
 	if (!iint)
 		return -EOPNOTSUPP;
 
-	mutex_lock(&iint->mutex);
-
 	/*
 	 * ima_file_hash can be called when ima_collect_measurement has still
 	 * not been called, we might not always have a hash.
@@ -552,12 +570,14 @@  static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
 	hash_algo = iint->ima_hash->algo;
 	mutex_unlock(&iint->mutex);
 
+	if (iint == &tmp_iint)
+		kfree(iint->ima_hash);
+
 	return hash_algo;
 }
 
 /**
- * ima_file_hash - return the stored measurement if a file has been hashed and
- * is in the iint cache.
+ * ima_file_hash - return a measurement of the file
  * @file: pointer to the file
  * @buf: buffer in which to store the hash
  * @buf_size: length of the buffer
@@ -570,7 +590,7 @@  static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
  * The file hash returned is based on the entire file, including the appended
  * signature.
  *
- * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
+ * If the measurement cannot be performed, return -EOPNOTSUPP.
  * If the parameters are incorrect, return -EINVAL.
  */
 int ima_file_hash(struct file *file, char *buf, size_t buf_size)
@@ -578,7 +598,7 @@  int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 	if (!file)
 		return -EINVAL;
 
-	return __ima_inode_hash(file_inode(file), buf, buf_size);
+	return __ima_inode_hash(file_inode(file), file, buf, buf_size);
 }
 EXPORT_SYMBOL_GPL(ima_file_hash);
 
@@ -605,7 +625,7 @@  int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
 	if (!inode)
 		return -EINVAL;
 
-	return __ima_inode_hash(inode, buf, buf_size);
+	return __ima_inode_hash(inode, NULL, buf, buf_size);
 }
 EXPORT_SYMBOL_GPL(ima_inode_hash);