[PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Roberto Sassu]

Extend the interoperability with IMA, to give wider flexibility for the
implementation of integrity-focused LSMs based on eBPF.

Patch 1 fixes some style issues.

Patches 2-4 gives the ability to eBPF-based LSMs to take advantage of the
measurement capability of IMA without needing to setup a policy in IMA
(those LSMs might implement the policy capability themselves).

Patches 5-6 allows eBPF-based LSMs to evaluate files read by the kernel.

Changelog

v1:
- Modify ima_file_hash() only and allow the usage of the function with the
  modified behavior by eBPF-based LSMs through the new function
  bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash()
  and bpf_ima_file_hash() can be called inside the implementation of
  eBPF-based LSMs for this hook

Roberto Sassu (6):
  ima: Fix documentation-related warnings in ima_main.c
  ima: Always return a file measurement in ima_file_hash()
  bpf-lsm: Introduce new helper bpf_ima_file_hash()
  selftests/bpf: Add test for bpf_ima_file_hash()
  bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable
  selftests/bpf: Add test for bpf_lsm_kernel_read_file()

 include/uapi/linux/bpf.h                      | 11 +++++
 kernel/bpf/bpf_lsm.c                          | 21 +++++++++
 security/integrity/ima/ima_main.c             | 47 ++++++++++++-------
 tools/include/uapi/linux/bpf.h                | 11 +++++
 tools/testing/selftests/bpf/ima_setup.sh      |  2 +
 .../selftests/bpf/prog_tests/test_ima.c       | 30 ++++++++++--
 tools/testing/selftests/bpf/progs/ima.c       | 34 ++++++++++++--
 7 files changed, 132 insertions(+), 24 deletions(-)

-- 
2.32.0

[PATCH v2 1/6] ima: Fix documentation-related warnings in ima_main.c

[Roberto Sassu]

Fix some warnings in ima_main.c, displayed with W=n make argument.

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

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8c6e4514d494..946ba8a12eab 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -418,6 +418,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
 
 /**
  * ima_file_mprotect - based on policy, limit mprotect change
+ * @vma: vm_area_struct protection is set to
  * @prot: contains the protection that will be applied by the kernel.
  *
  * Files can be mmap'ed read/write and later changed to execute to circumvent
@@ -610,8 +611,8 @@ EXPORT_SYMBOL_GPL(ima_inode_hash);
 
 /**
  * ima_post_create_tmpfile - mark newly created tmpfile as new
- * @mnt_userns:	user namespace of the mount the inode was found from
- * @file : newly created tmpfile
+ * @mnt_userns: user namespace of the mount the inode was found from
+ * @inode: inode of the newly created tmpfile
  *
  * No measuring, appraising or auditing of newly created tmpfiles is needed.
  * Skip calling process_measurement(), but indicate which newly, created
@@ -643,7 +644,7 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 
 /**
  * ima_post_path_mknod - mark as a new inode
- * @mnt_userns:	user namespace of the mount the inode was found from
+ * @mnt_userns: user namespace of the mount the inode was found from
  * @dentry: newly created dentry
  *
  * Mark files created via the mknodat syscall as new, so that the
@@ -814,8 +815,8 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
  * ima_post_load_data - appraise decision based on policy
  * @buf: pointer to in memory file contents
  * @size: size of in memory file contents
- * @id: kernel load data caller identifier
- * @description: @id-specific description of contents
+ * @load_id: kernel load data caller identifier
+ * @description: @load_id-specific description of contents
  *
  * Measure/appraise/audit in memory buffer based on policy.  Policy rules
  * are written in terms of a policy identifier.
-- 
2.32.0

[PATCH v2 2/6] ima: Always return a file measurement in ima_file_hash()

[Roberto Sassu]

__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.

Call ima_collect_measurement() in __ima_inode_hash(), if the file
descriptor is available (passed by ima_file_hash()), 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 | 36 +++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 946ba8a12eab..3562a212a5ba 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -520,15 +520,27 @@ 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 && file) {
+		memset(&tmp_iint, 0, sizeof(tmp_iint));
+		tmp_iint.inode = inode;
+
+		rc = ima_collect_measurement(&tmp_iint, file, NULL, 0,
+					     ima_hash_algo, NULL);
+		if (rc < 0)
+			return -EOPNOTSUPP;
+
+		iint = &tmp_iint;
+	}
 
-	iint = integrity_iint_find(inode);
 	if (!iint)
 		return -EOPNOTSUPP;
 
@@ -552,12 +564,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 +584,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 +592,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 +619,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);
 
-- 
2.32.0

[PATCH v2 3/6] bpf-lsm: Introduce new helper bpf_ima_file_hash()

[Roberto Sassu]

ima_file_hash() has been modified to calculate the measurement of a file on
demand, if it has not been already performed by IMA. For compatibility
reasons, ima_inode_hash() remains unchanged.

Keep the same approach in eBPF and introduce the new helper
bpf_ima_file_hash() to take advantage of the modified behavior of
ima_file_hash().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/uapi/linux/bpf.h       | 11 +++++++++++
 kernel/bpf/bpf_lsm.c           | 20 ++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 11 +++++++++++
 3 files changed, 42 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b0383d371b9a..ba33d5718d6b 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4648,6 +4648,16 @@ union bpf_attr {
  *		**-EOPNOTSUP** if IMA is disabled or **-EINVAL** if
  *		invalid arguments are passed.
  *
+ * long bpf_ima_file_hash(struct file *file, void *dst, u32 size)
+ *	Description
+ *		Returns a calculated IMA hash of the *file*.
+ *		If the hash is larger than *size*, then only *size*
+ *		bytes will be copied to *dst*
+ *	Return
+ *		The **hash_algo** is returned on success,
+ *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
+ *		invalid arguments are passed.
+ *
  * struct socket *bpf_sock_from_file(struct file *file)
  *	Description
  *		If the given file represents a socket, returns the associated
@@ -5182,6 +5192,7 @@ union bpf_attr {
 	FN(bprm_opts_set),		\
 	FN(ktime_get_coarse_ns),	\
 	FN(ima_inode_hash),		\
+	FN(ima_file_hash),		\
 	FN(sock_from_file),		\
 	FN(check_mtu),			\
 	FN(for_each_map_elem),		\
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 9e4ecc990647..e8d27af5bbcc 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -99,6 +99,24 @@ static const struct bpf_func_proto bpf_ima_inode_hash_proto = {
 	.allowed	= bpf_ima_inode_hash_allowed,
 };
 
+BPF_CALL_3(bpf_ima_file_hash, struct file *, file, void *, dst, u32, size)
+{
+	return ima_file_hash(file, dst, size);
+}
+
+BTF_ID_LIST_SINGLE(bpf_ima_file_hash_btf_ids, struct, file)
+
+static const struct bpf_func_proto bpf_ima_file_hash_proto = {
+	.func		= bpf_ima_file_hash,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &bpf_ima_file_hash_btf_ids[0],
+	.arg2_type	= ARG_PTR_TO_UNINIT_MEM,
+	.arg3_type	= ARG_CONST_SIZE,
+	.allowed	= bpf_ima_inode_hash_allowed,
+};
+
 static const struct bpf_func_proto *
 bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -121,6 +139,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_bprm_opts_set_proto;
 	case BPF_FUNC_ima_inode_hash:
 		return prog->aux->sleepable ? &bpf_ima_inode_hash_proto : NULL;
+	case BPF_FUNC_ima_file_hash:
+		return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL;
 	default:
 		return tracing_prog_func_proto(func_id, prog);
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b0383d371b9a..ba33d5718d6b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4648,6 +4648,16 @@ union bpf_attr {
  *		**-EOPNOTSUP** if IMA is disabled or **-EINVAL** if
  *		invalid arguments are passed.
  *
+ * long bpf_ima_file_hash(struct file *file, void *dst, u32 size)
+ *	Description
+ *		Returns a calculated IMA hash of the *file*.
+ *		If the hash is larger than *size*, then only *size*
+ *		bytes will be copied to *dst*
+ *	Return
+ *		The **hash_algo** is returned on success,
+ *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
+ *		invalid arguments are passed.
+ *
  * struct socket *bpf_sock_from_file(struct file *file)
  *	Description
  *		If the given file represents a socket, returns the associated
@@ -5182,6 +5192,7 @@ union bpf_attr {
 	FN(bprm_opts_set),		\
 	FN(ktime_get_coarse_ns),	\
 	FN(ima_inode_hash),		\
+	FN(ima_file_hash),		\
 	FN(sock_from_file),		\
 	FN(check_mtu),			\
 	FN(for_each_map_elem),		\
-- 
2.32.0

[PATCH v2 4/6] selftests/bpf: Add test for bpf_ima_file_hash()

[Roberto Sassu]

Modify the existing IMA test to call bpf_ima_file_hash() and update the
expected result accordingly.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .../selftests/bpf/prog_tests/test_ima.c       | 29 ++++++++++++++++---
 tools/testing/selftests/bpf/progs/ima.c       | 10 +++++--
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
index 97d8a6f84f4a..62bf0e830453 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
@@ -13,9 +13,10 @@
 
 #include "ima.skel.h"
 
-static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
+static int run_measured_process(const char *measured_dir, u32 *monitored_pid,
+				bool *use_ima_file_hash)
 {
-	int child_pid, child_status;
+	int err, child_pid, child_status;
 
 	child_pid = fork();
 	if (child_pid == 0) {
@@ -24,6 +25,21 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
 		       NULL);
 		exit(errno);
 
+	} else if (child_pid > 0) {
+		waitpid(child_pid, &child_status, 0);
+		err = WEXITSTATUS(child_status);
+		if (err)
+			return err;
+	}
+
+	child_pid = fork();
+	if (child_pid == 0) {
+		*monitored_pid = getpid();
+		*use_ima_file_hash = true;
+		execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir,
+		       NULL);
+		exit(errno);
+
 	} else if (child_pid > 0) {
 		waitpid(child_pid, &child_status, 0);
 		return WEXITSTATUS(child_status);
@@ -72,12 +88,17 @@ void test_test_ima(void)
 	if (CHECK(err, "failed to run command", "%s, errno = %d\n", cmd, errno))
 		goto close_clean;
 
-	err = run_measured_process(measured_dir, &skel->bss->monitored_pid);
+	err = run_measured_process(measured_dir, &skel->bss->monitored_pid,
+				   &skel->bss->use_ima_file_hash);
 	if (CHECK(err, "run_measured_process", "err = %d\n", err))
 		goto close_clean;
 
 	err = ring_buffer__consume(ringbuf);
-	ASSERT_EQ(err, 1, "num_samples_or_err");
+	/*
+	 * 1 sample with use_ima_file_hash = false
+	 * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
+	 */
+	ASSERT_EQ(err, 3, "num_samples_or_err");
 	ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
 
 close_clean:
diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
index 96060ff4ffc6..9bb63f96cfc0 100644
--- a/tools/testing/selftests/bpf/progs/ima.c
+++ b/tools/testing/selftests/bpf/progs/ima.c
@@ -18,6 +18,8 @@ struct {
 
 char _license[] SEC("license") = "GPL";
 
+bool use_ima_file_hash;
+
 SEC("lsm.s/bprm_committed_creds")
 void BPF_PROG(ima, struct linux_binprm *bprm)
 {
@@ -28,8 +30,12 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
 
 	pid = bpf_get_current_pid_tgid() >> 32;
 	if (pid == monitored_pid) {
-		ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
-					 sizeof(ima_hash));
+		if (!use_ima_file_hash)
+			ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
+						 sizeof(ima_hash));
+		else
+			ret = bpf_ima_file_hash(bprm->file, &ima_hash,
+						sizeof(ima_hash));
 		if (ret < 0 || ima_hash == 0)
 			return;
 
-- 
2.32.0

[PATCH v2 5/6] bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable

[Roberto Sassu]

Make bpf_lsm_kernel_read_file() as sleepable, so that bpf_ima_inode_hash()
or bpf_ima_file_hash() can be called inside the implementation of this
hook.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 kernel/bpf/bpf_lsm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index e8d27af5bbcc..064eccba641d 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -187,6 +187,7 @@ BTF_ID(func, bpf_lsm_inode_setxattr)
 BTF_ID(func, bpf_lsm_inode_symlink)
 BTF_ID(func, bpf_lsm_inode_unlink)
 BTF_ID(func, bpf_lsm_kernel_module_request)
+BTF_ID(func, bpf_lsm_kernel_read_file)
 BTF_ID(func, bpf_lsm_kernfs_init_security)
 
 #ifdef CONFIG_KEYS
-- 
2.32.0

[PATCH v2 6/6] selftests/bpf: Add test for bpf_lsm_kernel_read_file()

[Roberto Sassu]

Test the ability of bpf_lsm_kernel_read_file() to call the sleepable
functions bpf_ima_inode_hash() or bpf_ima_file_hash() to obtain a
measurement of a loaded IMA policy.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 tools/testing/selftests/bpf/ima_setup.sh      |  2 ++
 .../selftests/bpf/prog_tests/test_ima.c       |  3 +-
 tools/testing/selftests/bpf/progs/ima.c       | 28 ++++++++++++++++---
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
index 8e62581113a3..82530f19f85a 100755
--- a/tools/testing/selftests/bpf/ima_setup.sh
+++ b/tools/testing/selftests/bpf/ima_setup.sh
@@ -51,6 +51,7 @@ setup()
 
 	ensure_mount_securityfs
 	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
+	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${mount_dir}/policy_test
 }
 
 cleanup() {
@@ -74,6 +75,7 @@ run()
 	local mount_dir="${tmp_dir}/mnt"
 	local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
 
+	echo ${mount_dir}/policy_test > ${IMA_POLICY_FILE}
 	exec "${copied_bin_path}"
 }
 
diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
index 62bf0e830453..c4a62d7b70df 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
@@ -97,8 +97,9 @@ void test_test_ima(void)
 	/*
 	 * 1 sample with use_ima_file_hash = false
 	 * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
+	 * 1 sample with use_ima_file_hash = true (IMA policy)
 	 */
-	ASSERT_EQ(err, 3, "num_samples_or_err");
+	ASSERT_EQ(err, 4, "num_samples_or_err");
 	ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
 
 close_clean:
diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
index 9bb63f96cfc0..9b4c03f30a1c 100644
--- a/tools/testing/selftests/bpf/progs/ima.c
+++ b/tools/testing/selftests/bpf/progs/ima.c
@@ -20,8 +20,7 @@ char _license[] SEC("license") = "GPL";
 
 bool use_ima_file_hash;
 
-SEC("lsm.s/bprm_committed_creds")
-void BPF_PROG(ima, struct linux_binprm *bprm)
+static void ima_test_common(struct file *file)
 {
 	u64 ima_hash = 0;
 	u64 *sample;
@@ -31,10 +30,10 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
 	pid = bpf_get_current_pid_tgid() >> 32;
 	if (pid == monitored_pid) {
 		if (!use_ima_file_hash)
-			ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
+			ret = bpf_ima_inode_hash(file->f_inode, &ima_hash,
 						 sizeof(ima_hash));
 		else
-			ret = bpf_ima_file_hash(bprm->file, &ima_hash,
+			ret = bpf_ima_file_hash(file, &ima_hash,
 						sizeof(ima_hash));
 		if (ret < 0 || ima_hash == 0)
 			return;
@@ -49,3 +48,24 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
 
 	return;
 }
+
+SEC("lsm.s/bprm_committed_creds")
+void BPF_PROG(ima, struct linux_binprm *bprm)
+{
+	ima_test_common(bprm->file);
+}
+
+SEC("lsm.s/kernel_read_file")
+int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id,
+	     bool contents)
+{
+	if (!contents)
+		return 0;
+
+	if (id != READING_POLICY)
+		return 0;
+
+	ima_test_common(file);
+
+	return 0;
+}
-- 
2.32.0

Re: [PATCH v2 1/6] ima: Fix documentation-related warnings in ima_main.c

[Shuah Khan]

On 2/15/22 5:40 AM, Roberto Sassu wrote:
> Fix some warnings in ima_main.c, displayed with W=n make argument.
> 

Thank you for fixing these. Doc builds are full of them and few less
is welcome.

Adding the warns or summary of them to change log will be good.

> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   security/integrity/ima/ima_main.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 8c6e4514d494..946ba8a12eab 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -418,6 +418,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
>   
>   /**
>    * ima_file_mprotect - based on policy, limit mprotect change
> + * @vma: vm_area_struct protection is set to
>    * @prot: contains the protection that will be applied by the kernel.
>    *


Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

RE: [PATCH v2 1/6] ima: Fix documentation-related warnings in ima_main.c

[Roberto Sassu]

> From: Shuah Khan [mailto:skhan@linuxfoundation.org]
> Sent: Tuesday, February 15, 2022 4:46 PM
> On 2/15/22 5:40 AM, Roberto Sassu wrote:
> > Fix some warnings in ima_main.c, displayed with W=n make argument.
> >
> 
> Thank you for fixing these. Doc builds are full of them and few less
> is welcome.
> 
> Adding the warns or summary of them to change log will be good.

Hi Shuah

ok, I will add a brief description of what I fixed in the next version
of the patch set.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   security/integrity/ima/ima_main.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> > index 8c6e4514d494..946ba8a12eab 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -418,6 +418,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
> >
> >   /**
> >    * ima_file_mprotect - based on policy, limit mprotect change
> > + * @vma: vm_area_struct protection is set to
> >    * @prot: contains the protection that will be applied by the kernel.
> >    *
> 
> 
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> thanks,
> -- Shuah

Re: [PATCH v2 4/6] selftests/bpf: Add test for bpf_ima_file_hash()

[Shuah Khan]

On 2/15/22 5:40 AM, Roberto Sassu wrote:
> Modify the existing IMA test to call bpf_ima_file_hash() and update the
> expected result accordingly.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   .../selftests/bpf/prog_tests/test_ima.c       | 29 ++++++++++++++++---
>   tools/testing/selftests/bpf/progs/ima.c       | 10 +++++--
>   2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> index 97d8a6f84f4a..62bf0e830453 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> @@ -13,9 +13,10 @@
>   
>   #include "ima.skel.h"
>   
> -static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
> +static int run_measured_process(const char *measured_dir, u32 *monitored_pid,
> +				bool *use_ima_file_hash)
>   {
> -	int child_pid, child_status;
> +	int err, child_pid, child_status;
>   
>   	child_pid = fork();
>   	if (child_pid == 0) {
> @@ -24,6 +25,21 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid)
>   		       NULL);
>   		exit(errno);
>   
> +	} else if (child_pid > 0) {
> +		waitpid(child_pid, &child_status, 0);
> +		err = WEXITSTATUS(child_status);
> +		if (err)
> +			return err;
> +	}
> +
> +	child_pid = fork();
> +	if (child_pid == 0) {
> +		*monitored_pid = getpid();
> +		*use_ima_file_hash = true;
> +		execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir,
> +		       NULL);
> +		exit(errno);
> +
>   	} else if (child_pid > 0) {
>   		waitpid(child_pid, &child_status, 0);
>   		return WEXITSTATUS(child_status);
> @@ -72,12 +88,17 @@ void test_test_ima(void)
>   	if (CHECK(err, "failed to run command", "%s, errno = %d\n", cmd, errno))
>   		goto close_clean;
>   
> -	err = run_measured_process(measured_dir, &skel->bss->monitored_pid);
> +	err = run_measured_process(measured_dir, &skel->bss->monitored_pid,
> +				   &skel->bss->use_ima_file_hash);
>   	if (CHECK(err, "run_measured_process", "err = %d\n", err))
>   		goto close_clean;
>   
>   	err = ring_buffer__consume(ringbuf);
> -	ASSERT_EQ(err, 1, "num_samples_or_err");
> +	/*
> +	 * 1 sample with use_ima_file_hash = false
> +	 * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
> +	 */
> +	ASSERT_EQ(err, 3, "num_samples_or_err");
>   	ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
>   
>   close_clean:
> diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
> index 96060ff4ffc6..9bb63f96cfc0 100644
> --- a/tools/testing/selftests/bpf/progs/ima.c
> +++ b/tools/testing/selftests/bpf/progs/ima.c
> @@ -18,6 +18,8 @@ struct {
>   
>   char _license[] SEC("license") = "GPL";
>   
> +bool use_ima_file_hash;
> +

This can be statis.

>   SEC("lsm.s/bprm_committed_creds")
>   void BPF_PROG(ima, struct linux_binprm *bprm)
>   {
> @@ -28,8 +30,12 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
>   
>   	pid = bpf_get_current_pid_tgid() >> 32;
>   	if (pid == monitored_pid) {

I also noticed monitored_pid is defined in several bpf. Potentially
could be made static. This isn't introduced in this patch though.

> -		ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
> -					 sizeof(ima_hash));
> +		if (!use_ima_file_hash)
> +			ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
> +						 sizeof(ima_hash));
> +		else
> +			ret = bpf_ima_file_hash(bprm->file, &ima_hash,
> +						sizeof(ima_hash));
>   		if (ret < 0 || ima_hash == 0)
>   			return;
>   
> 

thanks,
-- Shuah

Re: [PATCH v2 6/6] selftests/bpf: Add test for bpf_lsm_kernel_read_file()

[Shuah Khan]

On 2/15/22 5:40 AM, Roberto Sassu wrote:
> Test the ability of bpf_lsm_kernel_read_file() to call the sleepable
> functions bpf_ima_inode_hash() or bpf_ima_file_hash() to obtain a
> measurement of a loaded IMA policy.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   tools/testing/selftests/bpf/ima_setup.sh      |  2 ++
>   .../selftests/bpf/prog_tests/test_ima.c       |  3 +-
>   tools/testing/selftests/bpf/progs/ima.c       | 28 ++++++++++++++++---
>   3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh
> index 8e62581113a3..82530f19f85a 100755
> --- a/tools/testing/selftests/bpf/ima_setup.sh
> +++ b/tools/testing/selftests/bpf/ima_setup.sh
> @@ -51,6 +51,7 @@ setup()
>   
>   	ensure_mount_securityfs
>   	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE}
> +	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${mount_dir}/policy_test
>   }
>   
>   cleanup() {
> @@ -74,6 +75,7 @@ run()
>   	local mount_dir="${tmp_dir}/mnt"
>   	local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
>   
> +	echo ${mount_dir}/policy_test > ${IMA_POLICY_FILE}
>   	exec "${copied_bin_path}"
>   }
>   
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> index 62bf0e830453..c4a62d7b70df 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> @@ -97,8 +97,9 @@ void test_test_ima(void)
>   	/*
>   	 * 1 sample with use_ima_file_hash = false
>   	 * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
> +	 * 1 sample with use_ima_file_hash = true (IMA policy)
>   	 */
> -	ASSERT_EQ(err, 3, "num_samples_or_err");
> +	ASSERT_EQ(err, 4, "num_samples_or_err");
>   	ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
>   
>   close_clean:
> diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c
> index 9bb63f96cfc0..9b4c03f30a1c 100644
> --- a/tools/testing/selftests/bpf/progs/ima.c
> +++ b/tools/testing/selftests/bpf/progs/ima.c
> @@ -20,8 +20,7 @@ char _license[] SEC("license") = "GPL";
>   
>   bool use_ima_file_hash;
>   
> -SEC("lsm.s/bprm_committed_creds")
> -void BPF_PROG(ima, struct linux_binprm *bprm)
> +static void ima_test_common(struct file *file)
>   {
>   	u64 ima_hash = 0;
>   	u64 *sample;
> @@ -31,10 +30,10 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
>   	pid = bpf_get_current_pid_tgid() >> 32;
>   	if (pid == monitored_pid) {
>   		if (!use_ima_file_hash)
> -			ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash,
> +			ret = bpf_ima_inode_hash(file->f_inode, &ima_hash,
>   						 sizeof(ima_hash));
>   		else
> -			ret = bpf_ima_file_hash(bprm->file, &ima_hash,
> +			ret = bpf_ima_file_hash(file, &ima_hash,
>   						sizeof(ima_hash));
>   		if (ret < 0 || ima_hash == 0)

Is this considered an error? Does it make sense for this test to be
void type and not return the error to its callers? One of the callers
below seems to care for return values.

>   			return;
> @@ -49,3 +48,24 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
>   
>   	return;
>   }
> +
> +SEC("lsm.s/bprm_committed_creds")
> +void BPF_PROG(ima, struct linux_binprm *bprm)
> +{
> +	ima_test_common(bprm->file);
> +}
> +
> +SEC("lsm.s/kernel_read_file")
> +int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id,
> +	     bool contents)
> +{
> +	if (!contents)
> +		return 0;
> +
> +	if (id != READING_POLICY)
> +		return 0;
> +
> +	ima_test_common(file);

This one here.

> +
> +	return 0;
> +}
> 

thanks,
-- Shuah

RE: [PATCH v2 6/6] selftests/bpf: Add test for bpf_lsm_kernel_read_file()

[Roberto Sassu]

> From: Shuah Khan [mailto:skhan@linuxfoundation.org]
> Sent: Tuesday, February 15, 2022 5:11 PM
> On 2/15/22 5:40 AM, Roberto Sassu wrote:
> > Test the ability of bpf_lsm_kernel_read_file() to call the sleepable
> > functions bpf_ima_inode_hash() or bpf_ima_file_hash() to obtain a
> > measurement of a loaded IMA policy.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   tools/testing/selftests/bpf/ima_setup.sh      |  2 ++
> >   .../selftests/bpf/prog_tests/test_ima.c       |  3 +-
> >   tools/testing/selftests/bpf/progs/ima.c       | 28 ++++++++++++++++---
> >   3 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/ima_setup.sh
> b/tools/testing/selftests/bpf/ima_setup.sh
> > index 8e62581113a3..82530f19f85a 100755
> > --- a/tools/testing/selftests/bpf/ima_setup.sh
> > +++ b/tools/testing/selftests/bpf/ima_setup.sh
> > @@ -51,6 +51,7 @@ setup()
> >
> >   	ensure_mount_securityfs
> >   	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" >
> ${IMA_POLICY_FILE}
> > +	echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" >
> ${mount_dir}/policy_test
> >   }
> >
> >   cleanup() {
> > @@ -74,6 +75,7 @@ run()
> >   	local mount_dir="${tmp_dir}/mnt"
> >   	local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})"
> >
> > +	echo ${mount_dir}/policy_test > ${IMA_POLICY_FILE}
> >   	exec "${copied_bin_path}"
> >   }
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c
> b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> > index 62bf0e830453..c4a62d7b70df 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c
> > @@ -97,8 +97,9 @@ void test_test_ima(void)
> >   	/*
> >   	 * 1 sample with use_ima_file_hash = false
> >   	 * 2 samples with use_ima_file_hash = true (./ima_setup.sh, /bin/true)
> > +	 * 1 sample with use_ima_file_hash = true (IMA policy)
> >   	 */
> > -	ASSERT_EQ(err, 3, "num_samples_or_err");
> > +	ASSERT_EQ(err, 4, "num_samples_or_err");
> >   	ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash");
> >
> >   close_clean:
> > diff --git a/tools/testing/selftests/bpf/progs/ima.c
> b/tools/testing/selftests/bpf/progs/ima.c
> > index 9bb63f96cfc0..9b4c03f30a1c 100644
> > --- a/tools/testing/selftests/bpf/progs/ima.c
> > +++ b/tools/testing/selftests/bpf/progs/ima.c
> > @@ -20,8 +20,7 @@ char _license[] SEC("license") = "GPL";
> >
> >   bool use_ima_file_hash;
> >
> > -SEC("lsm.s/bprm_committed_creds")
> > -void BPF_PROG(ima, struct linux_binprm *bprm)
> > +static void ima_test_common(struct file *file)
> >   {
> >   	u64 ima_hash = 0;
> >   	u64 *sample;
> > @@ -31,10 +30,10 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
> >   	pid = bpf_get_current_pid_tgid() >> 32;
> >   	if (pid == monitored_pid) {
> >   		if (!use_ima_file_hash)
> > -			ret = bpf_ima_inode_hash(bprm->file->f_inode,
> &ima_hash,
> > +			ret = bpf_ima_inode_hash(file->f_inode, &ima_hash,
> >   						 sizeof(ima_hash));
> >   		else
> > -			ret = bpf_ima_file_hash(bprm->file, &ima_hash,
> > +			ret = bpf_ima_file_hash(file, &ima_hash,
> >   						sizeof(ima_hash));
> >   		if (ret < 0 || ima_hash == 0)
> 
> Is this considered an error? Does it make sense for this test to be
> void type and not return the error to its callers? One of the callers
> below seems to care for return values.

The user space side of the test (test_ima.c) seems to check the
number of samples obtained from the ring buffer. A failure here
would result in the sample not being sent to that component.

Another test, as you suggest, could be to ensure that the
kernel_read_file hook is able to deny operations. I would check
this in a separate test.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua 

> >   			return;
> > @@ -49,3 +48,24 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
> >
> >   	return;
> >   }
> > +
> > +SEC("lsm.s/bprm_committed_creds")
> > +void BPF_PROG(ima, struct linux_binprm *bprm)
> > +{
> > +	ima_test_common(bprm->file);
> > +}
> > +
> > +SEC("lsm.s/kernel_read_file")
> > +int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id,
> > +	     bool contents)
> > +{
> > +	if (!contents)
> > +		return 0;
> > +
> > +	if (id != READING_POLICY)
> > +		return 0;
> > +
> > +	ima_test_common(file);
> 
> This one here.
> 
> > +
> > +	return 0;
> > +}
> >
> 
> thanks,
> -- Shuah

Re: [PATCH v2 3/6] bpf-lsm: Introduce new helper bpf_ima_file_hash()

[Yonghong Song]

On 2/15/22 4:40 AM, Roberto Sassu wrote:
> ima_file_hash() has been modified to calculate the measurement of a file on
> demand, if it has not been already performed by IMA. For compatibility
> reasons, ima_inode_hash() remains unchanged.
> 
> Keep the same approach in eBPF and introduce the new helper
> bpf_ima_file_hash() to take advantage of the modified behavior of
> ima_file_hash().
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   include/uapi/linux/bpf.h       | 11 +++++++++++
>   kernel/bpf/bpf_lsm.c           | 20 ++++++++++++++++++++
>   tools/include/uapi/linux/bpf.h | 11 +++++++++++
>   3 files changed, 42 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index b0383d371b9a..ba33d5718d6b 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4648,6 +4648,16 @@ union bpf_attr {
>    *		**-EOPNOTSUP** if IMA is disabled or **-EINVAL** if
>    *		invalid arguments are passed.
>    *
> + * long bpf_ima_file_hash(struct file *file, void *dst, u32 size)
> + *	Description
> + *		Returns a calculated IMA hash of the *file*.
> + *		If the hash is larger than *size*, then only *size*
> + *		bytes will be copied to *dst*
> + *	Return
> + *		The **hash_algo** is returned on success,
> + *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
> + *		invalid arguments are passed.
> + *
>    * struct socket *bpf_sock_from_file(struct file *file)
>    *	Description
>    *		If the given file represents a socket, returns the associated
> @@ -5182,6 +5192,7 @@ union bpf_attr {
>   	FN(bprm_opts_set),		\
>   	FN(ktime_get_coarse_ns),	\
>   	FN(ima_inode_hash),		\
> +	FN(ima_file_hash),		\

Please put the above FN(ima_file_hash) to the end of the list.
Otherwise, we have a backward compatability issue.

>   	FN(sock_from_file),		\
>   	FN(check_mtu),			\
>   	FN(for_each_map_elem),		\
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 9e4ecc990647..e8d27af5bbcc 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
[...]

RE: [PATCH v2 3/6] bpf-lsm: Introduce new helper bpf_ima_file_hash()

[Roberto Sassu]

> From: Yonghong Song [mailto:yhs@fb.com]
> Sent: Tuesday, February 15, 2022 6:03 PM
> On 2/15/22 4:40 AM, Roberto Sassu wrote:
> > ima_file_hash() has been modified to calculate the measurement of a file on
> > demand, if it has not been already performed by IMA. For compatibility
> > reasons, ima_inode_hash() remains unchanged.
> >
> > Keep the same approach in eBPF and introduce the new helper
> > bpf_ima_file_hash() to take advantage of the modified behavior of
> > ima_file_hash().
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >   include/uapi/linux/bpf.h       | 11 +++++++++++
> >   kernel/bpf/bpf_lsm.c           | 20 ++++++++++++++++++++
> >   tools/include/uapi/linux/bpf.h | 11 +++++++++++
> >   3 files changed, 42 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index b0383d371b9a..ba33d5718d6b 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4648,6 +4648,16 @@ union bpf_attr {
> >    *		**-EOPNOTSUP** if IMA is disabled or **-EINVAL** if
> >    *		invalid arguments are passed.
> >    *
> > + * long bpf_ima_file_hash(struct file *file, void *dst, u32 size)
> > + *	Description
> > + *		Returns a calculated IMA hash of the *file*.
> > + *		If the hash is larger than *size*, then only *size*
> > + *		bytes will be copied to *dst*
> > + *	Return
> > + *		The **hash_algo** is returned on success,
> > + *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL**
> if
> > + *		invalid arguments are passed.
> > + *
> >    * struct socket *bpf_sock_from_file(struct file *file)
> >    *	Description
> >    *		If the given file represents a socket, returns the associated
> > @@ -5182,6 +5192,7 @@ union bpf_attr {
> >   	FN(bprm_opts_set),		\
> >   	FN(ktime_get_coarse_ns),	\
> >   	FN(ima_inode_hash),		\
> > +	FN(ima_file_hash),		\
> 
> Please put the above FN(ima_file_hash) to the end of the list.
> Otherwise, we have a backward compatability issue.

Hi Yonghong

sure, will do.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> >   	FN(sock_from_file),		\
> >   	FN(check_mtu),			\
> >   	FN(for_each_map_elem),		\
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index 9e4ecc990647..e8d27af5bbcc 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> [...]

RE: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Roberto Sassu]

> From: Roberto Sassu
> Sent: Tuesday, February 15, 2022 1:41 PM
> Extend the interoperability with IMA, to give wider flexibility for the
> implementation of integrity-focused LSMs based on eBPF.
> 
> Patch 1 fixes some style issues.
> 
> Patches 2-4 gives the ability to eBPF-based LSMs to take advantage of the
> measurement capability of IMA without needing to setup a policy in IMA
> (those LSMs might implement the policy capability themselves).
> 
> Patches 5-6 allows eBPF-based LSMs to evaluate files read by the kernel.

Hi everyone

I published the new DIGLIM eBPF, that takes advantage of
the new features introduced with this patch set:

https://github.com/robertosassu/diglim-ebpf

the eBPF program is in ebpf/diglim_kern.c

If you could have a look and give me some comments
or suggestions, it would be very appreciated!

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> Changelog
> 
> v1:
> - Modify ima_file_hash() only and allow the usage of the function with the
>   modified behavior by eBPF-based LSMs through the new function
>   bpf_ima_file_hash() (suggested by Mimi)
> - Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash()
>   and bpf_ima_file_hash() can be called inside the implementation of
>   eBPF-based LSMs for this hook
> 
> Roberto Sassu (6):
>   ima: Fix documentation-related warnings in ima_main.c
>   ima: Always return a file measurement in ima_file_hash()
>   bpf-lsm: Introduce new helper bpf_ima_file_hash()
>   selftests/bpf: Add test for bpf_ima_file_hash()
>   bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable
>   selftests/bpf: Add test for bpf_lsm_kernel_read_file()
> 
>  include/uapi/linux/bpf.h                      | 11 +++++
>  kernel/bpf/bpf_lsm.c                          | 21 +++++++++
>  security/integrity/ima/ima_main.c             | 47 ++++++++++++-------
>  tools/include/uapi/linux/bpf.h                | 11 +++++
>  tools/testing/selftests/bpf/ima_setup.sh      |  2 +
>  .../selftests/bpf/prog_tests/test_ima.c       | 30 ++++++++++--
>  tools/testing/selftests/bpf/progs/ima.c       | 34 ++++++++++++--
>  7 files changed, 132 insertions(+), 24 deletions(-)
> 
> --
> 2.32.0

Re: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Mimi Zohar]

Hi Roberto,

On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> Extend the interoperability with IMA, to give wider flexibility for the
> implementation of integrity-focused LSMs based on eBPF.

I've previously requested adding eBPF module measurements and signature
verification support in IMA.  There seemed to be some interest, but
nothing has been posted.

thanks,

Mimi

RE: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, February 25, 2022 1:22 AM
> Hi Roberto,
> 
> On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > Extend the interoperability with IMA, to give wider flexibility for the
> > implementation of integrity-focused LSMs based on eBPF.
> 
> I've previously requested adding eBPF module measurements and signature
> verification support in IMA.  There seemed to be some interest, but
> nothing has been posted.

Hi Mimi

for my use case, DIGLIM eBPF, IMA integrity verification is
needed until the binary carrying the eBPF program is executed
as the init process. I've been thinking to use an appended
signature to overcome the limitation of lack of xattrs in the
initial ram disk.

At that point, the LSM is attached and it can enforce an
execution policy, allowing or denying execution and mmap
of files depending on the digest lists (reference values) read
by the user space side.

After the LSM is attached, IMA's job would be just to calculate
the file digests (currently, I'm using an audit policy to ensure
that the digest is available when the eBPF program calls
bpf_ima_inode_hash()).

The main benefit of this patch set is that the audit policy
would not be required and digests are calculated only when
requested by the eBPF program.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

Re: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Mimi Zohar]

On Fri, 2022-02-25 at 08:41 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Friday, February 25, 2022 1:22 AM
> > Hi Roberto,
> > 
> > On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > > Extend the interoperability with IMA, to give wider flexibility for the
> > > implementation of integrity-focused LSMs based on eBPF.
> > 
> > I've previously requested adding eBPF module measurements and signature
> > verification support in IMA.  There seemed to be some interest, but
> > nothing has been posted.
> 
> Hi Mimi
> 
> for my use case, DIGLIM eBPF, IMA integrity verification is
> needed until the binary carrying the eBPF program is executed
> as the init process. I've been thinking to use an appended
> signature to overcome the limitation of lack of xattrs in the
> initial ram disk.

I would still like to see xattrs supported in the initial ram disk. 
Assuming you're still interested in pursuing it, someone would need to
review and upstream it.  Greg?

> 
> At that point, the LSM is attached and it can enforce an
> execution policy, allowing or denying execution and mmap
> of files depending on the digest lists (reference values) read
> by the user space side.
> 
> After the LSM is attached, IMA's job would be just to calculate
> the file digests (currently, I'm using an audit policy to ensure
> that the digest is available when the eBPF program calls
> bpf_ima_inode_hash()).
> 
> The main benefit of this patch set is that the audit policy
> would not be required and digests are calculated only when
> requested by the eBPF program.

Roberto, there's an existing eBPF integrity gap that needs to be
closed, perhaps not for your usecase, but in general.  Is that
something you can look into?

thanks,

Mimi

Re: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Greg Kroah-Hartman]

On Fri, Feb 25, 2022 at 02:11:04PM -0500, Mimi Zohar wrote:
> On Fri, 2022-02-25 at 08:41 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Friday, February 25, 2022 1:22 AM
> > > Hi Roberto,
> > > 
> > > On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > > > Extend the interoperability with IMA, to give wider flexibility for the
> > > > implementation of integrity-focused LSMs based on eBPF.
> > > 
> > > I've previously requested adding eBPF module measurements and signature
> > > verification support in IMA.  There seemed to be some interest, but
> > > nothing has been posted.
> > 
> > Hi Mimi
> > 
> > for my use case, DIGLIM eBPF, IMA integrity verification is
> > needed until the binary carrying the eBPF program is executed
> > as the init process. I've been thinking to use an appended
> > signature to overcome the limitation of lack of xattrs in the
> > initial ram disk.
> 
> I would still like to see xattrs supported in the initial ram disk. 
> Assuming you're still interested in pursuing it, someone would need to
> review and upstream it.  Greg?

Me?  How about the filesystem maintainers and developers?  :)

There's a reason we never added xattrs support to ram disks, but I can't
remember why...

thanks,

gre gk-h

Re: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Mimi Zohar]

On Sat, 2022-02-26 at 09:07 +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 25, 2022 at 02:11:04PM -0500, Mimi Zohar wrote:
> > On Fri, 2022-02-25 at 08:41 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > Sent: Friday, February 25, 2022 1:22 AM
> > > > Hi Roberto,
> > > > 
> > > > On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > > > > Extend the interoperability with IMA, to give wider flexibility for the
> > > > > implementation of integrity-focused LSMs based on eBPF.
> > > > 
> > > > I've previously requested adding eBPF module measurements and signature
> > > > verification support in IMA.  There seemed to be some interest, but
> > > > nothing has been posted.
> > > 
> > > Hi Mimi
> > > 
> > > for my use case, DIGLIM eBPF, IMA integrity verification is
> > > needed until the binary carrying the eBPF program is executed
> > > as the init process. I've been thinking to use an appended
> > > signature to overcome the limitation of lack of xattrs in the
> > > initial ram disk.
> > 
> > I would still like to see xattrs supported in the initial ram disk. 
> > Assuming you're still interested in pursuing it, someone would need to
> > review and upstream it.  Greg?
> 
> Me?  How about the filesystem maintainers and developers?  :)
> 
> There's a reason we never added xattrs support to ram disks, but I can't
> remember why...

CPIO 'newc' format doesn't support xattrs.

thanks,

Mimi

RE: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, February 25, 2022 8:11 PM
> On Fri, 2022-02-25 at 08:41 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Friday, February 25, 2022 1:22 AM
> > > Hi Roberto,
> > >
> > > On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > > > Extend the interoperability with IMA, to give wider flexibility for the
> > > > implementation of integrity-focused LSMs based on eBPF.
> > >
> > > I've previously requested adding eBPF module measurements and signature
> > > verification support in IMA.  There seemed to be some interest, but
> > > nothing has been posted.
> >
> > Hi Mimi
> >
> > for my use case, DIGLIM eBPF, IMA integrity verification is
> > needed until the binary carrying the eBPF program is executed
> > as the init process. I've been thinking to use an appended
> > signature to overcome the limitation of lack of xattrs in the
> > initial ram disk.
> 
> I would still like to see xattrs supported in the initial ram disk.
> Assuming you're still interested in pursuing it, someone would need to
> review and upstream it.  Greg?

I could revise this work. However, since appended signatures
would work too, I would propose to extend this appraisal
mode to executables, if it is fine for you.

> > At that point, the LSM is attached and it can enforce an
> > execution policy, allowing or denying execution and mmap
> > of files depending on the digest lists (reference values) read
> > by the user space side.
> >
> > After the LSM is attached, IMA's job would be just to calculate
> > the file digests (currently, I'm using an audit policy to ensure
> > that the digest is available when the eBPF program calls
> > bpf_ima_inode_hash()).
> >
> > The main benefit of this patch set is that the audit policy
> > would not be required and digests are calculated only when
> > requested by the eBPF program.
> 
> Roberto, there's an existing eBPF integrity gap that needs to be
> closed, perhaps not for your usecase, but in general.  Is that
> something you can look into?

It could be possible I look into it.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

RE: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Roberto Sassu]

> From: Roberto Sassu
> Sent: Monday, February 28, 2022 10:08 AM
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Friday, February 25, 2022 8:11 PM
> > On Fri, 2022-02-25 at 08:41 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > Sent: Friday, February 25, 2022 1:22 AM
> > > > Hi Roberto,
> > > >
> > > > On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > > > > Extend the interoperability with IMA, to give wider flexibility for the
> > > > > implementation of integrity-focused LSMs based on eBPF.
> > > >
> > > > I've previously requested adding eBPF module measurements and signature
> > > > verification support in IMA.  There seemed to be some interest, but
> > > > nothing has been posted.
> > >
> > > Hi Mimi
> > >
> > > for my use case, DIGLIM eBPF, IMA integrity verification is
> > > needed until the binary carrying the eBPF program is executed
> > > as the init process. I've been thinking to use an appended
> > > signature to overcome the limitation of lack of xattrs in the
> > > initial ram disk.
> >
> > I would still like to see xattrs supported in the initial ram disk.
> > Assuming you're still interested in pursuing it, someone would need to
> > review and upstream it.  Greg?
> 
> I could revise this work. However, since appended signatures
> would work too, I would propose to extend this appraisal
> mode to executables, if it is fine for you.

Regarding this patch set, I kindly ask if you could accept it,
after I make the changes suggested.

The changes are simple, and waiting another kernel cycle
seems too long.

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> > > At that point, the LSM is attached and it can enforce an
> > > execution policy, allowing or denying execution and mmap
> > > of files depending on the digest lists (reference values) read
> > > by the user space side.
> > >
> > > After the LSM is attached, IMA's job would be just to calculate
> > > the file digests (currently, I'm using an audit policy to ensure
> > > that the digest is available when the eBPF program calls
> > > bpf_ima_inode_hash()).
> > >
> > > The main benefit of this patch set is that the audit policy
> > > would not be required and digests are calculated only when
> > > requested by the eBPF program.
> >
> > Roberto, there's an existing eBPF integrity gap that needs to be
> > closed, perhaps not for your usecase, but in general.  Is that
> > something you can look into?
> 
> It could be possible I look into it.
> 
> Roberto
> 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Zhong Ronghua

Re: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA

[Greg Kroah-Hartman]

On Mon, Feb 28, 2022 at 09:12:35AM +0000, Roberto Sassu wrote:
> > From: Roberto Sassu
> > Sent: Monday, February 28, 2022 10:08 AM
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Friday, February 25, 2022 8:11 PM
> > > On Fri, 2022-02-25 at 08:41 +0000, Roberto Sassu wrote:
> > > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > > Sent: Friday, February 25, 2022 1:22 AM
> > > > > Hi Roberto,
> > > > >
> > > > > On Tue, 2022-02-15 at 13:40 +0100, Roberto Sassu wrote:
> > > > > > Extend the interoperability with IMA, to give wider flexibility for the
> > > > > > implementation of integrity-focused LSMs based on eBPF.
> > > > >
> > > > > I've previously requested adding eBPF module measurements and signature
> > > > > verification support in IMA.  There seemed to be some interest, but
> > > > > nothing has been posted.
> > > >
> > > > Hi Mimi
> > > >
> > > > for my use case, DIGLIM eBPF, IMA integrity verification is
> > > > needed until the binary carrying the eBPF program is executed
> > > > as the init process. I've been thinking to use an appended
> > > > signature to overcome the limitation of lack of xattrs in the
> > > > initial ram disk.
> > >
> > > I would still like to see xattrs supported in the initial ram disk.
> > > Assuming you're still interested in pursuing it, someone would need to
> > > review and upstream it.  Greg?
> > 
> > I could revise this work. However, since appended signatures
> > would work too, I would propose to extend this appraisal
> > mode to executables, if it is fine for you.
> 
> Regarding this patch set, I kindly ask if you could accept it,
> after I make the changes suggested.
> 
> The changes are simple, and waiting another kernel cycle
> seems too long.

3 months is not a long time, get it right first, there is no deadline
here.

thanks,

greg k-h