linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash
@ 2020-11-21  0:50 KP Singh
  2020-11-21  0:50 ` [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: KP Singh @ 2020-11-21  0:50 UTC (permalink / raw)
  To: James Morris, linux-kernel, bpf, linux-security-module
  Cc: Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

From: KP Singh <kpsingh@google.com>

This is in preparation to add a helper for BPF LSM programs to use
IMA hashes when attached to LSM hooks. There are LSM hooks like
inode_unlink which do not have a struct file * argument and cannot
use the existing ima_file_hash API.

An inode based API is, therefore, useful in LSM based detections like an
executable trying to delete itself which rely on the inode_unlink LSM
hook.

Moreover, the ima_file_hash function does nothing with the struct file
pointer apart from calling file_inode on it and converting it to an
inode.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/ima.h               |  6 +++
 security/integrity/ima/ima_main.c | 78 +++++++++++++++++++++----------
 2 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 8fa7bcfb2da2..7233a2751754 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -29,6 +29,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
+extern int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size);
 extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
 
 #ifdef CONFIG_IMA_KEXEC
@@ -115,6 +116,11 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 	return -EOPNOTSUPP;
 }
 
+static inline int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2d1af8899cab..cb2deaa188e7 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -501,37 +501,14 @@ int ima_file_check(struct file *file, int mask)
 }
 EXPORT_SYMBOL_GPL(ima_file_check);
 
-/**
- * ima_file_hash - return the stored measurement if a file has been hashed and
- * is in the iint cache.
- * @file: pointer to the file
- * @buf: buffer in which to store the hash
- * @buf_size: length of the buffer
- *
- * On success, return the hash algorithm (as defined in the enum hash_algo).
- * If buf is not NULL, this function also outputs the hash into buf.
- * If the hash is larger than buf_size, then only buf_size bytes will be copied.
- * It generally just makes sense to pass a buffer capable of holding the largest
- * possible hash: IMA_MAX_DIGEST_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 parameters are incorrect, return -EINVAL.
- */
-int ima_file_hash(struct file *file, char *buf, size_t buf_size)
+static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
 {
-	struct inode *inode;
 	struct integrity_iint_cache *iint;
 	int hash_algo;
 
-	if (!file)
-		return -EINVAL;
-
 	if (!ima_policy_flag)
 		return -EOPNOTSUPP;
 
-	inode = file_inode(file);
 	iint = integrity_iint_find(inode);
 	if (!iint)
 		return -EOPNOTSUPP;
@@ -558,8 +535,61 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 
 	return hash_algo;
 }
+
+/**
+ * ima_file_hash - return the stored measurement if a file has been hashed and
+ * is in the iint cache.
+ * @file: pointer to the file
+ * @buf: buffer in which to store the hash
+ * @buf_size: length of the buffer
+ *
+ * On success, return the hash algorithm (as defined in the enum hash_algo).
+ * If buf is not NULL, this function also outputs the hash into buf.
+ * If the hash is larger than buf_size, then only buf_size bytes will be copied.
+ * It generally just makes sense to pass a buffer capable of holding the largest
+ * possible hash: IMA_MAX_DIGEST_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 parameters are incorrect, return -EINVAL.
+ */
+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);
+}
 EXPORT_SYMBOL_GPL(ima_file_hash);
 
+/**
+ * ima_inode_hash - return the stored measurement if the inode has been hashed
+ * and is in the iint cache.
+ * @inode: pointer to the inode
+ * @buf: buffer in which to store the hash
+ * @buf_size: length of the buffer
+ *
+ * On success, return the hash algorithm (as defined in the enum hash_algo).
+ * If buf is not NULL, this function also outputs the hash into buf.
+ * If the hash is larger than buf_size, then only buf_size bytes will be copied.
+ * It generally just makes sense to pass a buffer capable of holding the largest
+ * possible hash: IMA_MAX_DIGEST_SIZE.
+ * The hash returned is based on the entire contents, including the appended
+ * signature.
+ *
+ * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
+ * If the parameters are incorrect, return -EINVAL.
+ */
+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);
+}
+EXPORT_SYMBOL_GPL(ima_inode_hash);
+
 /**
  * ima_post_create_tmpfile - mark newly created tmpfile as new
  * @file : newly created tmpfile
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-21  0:50 [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash KP Singh
@ 2020-11-21  0:50 ` KP Singh
  2020-11-21  6:54   ` Yonghong Song
  2020-11-21  0:50 ` [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
  2020-11-21  6:48 ` [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash Yonghong Song
  2 siblings, 1 reply; 13+ messages in thread
From: KP Singh @ 2020-11-21  0:50 UTC (permalink / raw)
  To: James Morris, linux-kernel, bpf, linux-security-module
  Cc: Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

From: KP Singh <kpsingh@google.com>

Provide a wrapper function to get the IMA hash of an inode. This helper
is useful in fingerprinting files (e.g executables on execution) and
using these fingerprints in detections like an executable unlinking
itself.

Since the ima_inode_hash can sleep, it's only allowed for sleepable
LSM hooks.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/uapi/linux/bpf.h       | 11 +++++++++++
 kernel/bpf/bpf_lsm.c           | 26 ++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  2 ++
 tools/include/uapi/linux/bpf.h | 11 +++++++++++
 4 files changed, 50 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3ca6146f001a..c3458ec1f30a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3807,6 +3807,16 @@ union bpf_attr {
  * 		See: **clock_gettime**\ (**CLOCK_MONOTONIC_COARSE**)
  * 	Return
  * 		Current *ktime*.
+ *
+ * long bpf_ima_inode_hash(struct inode *inode, void *dst, u32 size)
+ *	Description
+ *		Returns the stored IMA hash of the *inode* (if it's avaialable).
+ *		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 IMA is disabled or **-EINVAL** if
+ *		invalid arguments are passed.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3970,6 +3980,7 @@ union bpf_attr {
 	FN(get_current_task_btf),	\
 	FN(bprm_opts_set),		\
 	FN(ktime_get_coarse_ns),	\
+	FN(ima_inode_hash),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index b4f27a874092..bec1f164ba58 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -15,6 +15,7 @@
 #include <net/bpf_sk_storage.h>
 #include <linux/bpf_local_storage.h>
 #include <linux/btf_ids.h>
+#include <linux/ima.h>
 
 /* For every LSM hook that allows attachment of BPF programs, declare a nop
  * function where a BPF program can be attached.
@@ -75,6 +76,29 @@ const static struct bpf_func_proto bpf_bprm_opts_set_proto = {
 	.arg2_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_3(bpf_ima_inode_hash, struct inode *, inode, void *, dst, u32, size)
+{
+	return ima_inode_hash(inode, dst, size);
+}
+
+static bool bpf_ima_inode_hash_allowed(const struct bpf_prog *prog)
+{
+	return bpf_lsm_is_sleepable_hook(prog->aux->attach_btf_id);
+}
+
+BTF_ID_LIST_SINGLE(bpf_ima_inode_hash_btf_ids, struct, inode)
+
+const static struct bpf_func_proto bpf_ima_inode_hash_proto = {
+	.func		= bpf_ima_inode_hash,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &bpf_ima_inode_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)
 {
@@ -97,6 +121,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_task_storage_delete_proto;
 	case BPF_FUNC_bprm_opts_set:
 		return &bpf_bprm_opts_set_proto;
+	case BPF_FUNC_ima_inode_hash:
+		return &bpf_ima_inode_hash_proto;
 	default:
 		return tracing_prog_func_proto(func_id, prog);
 	}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index c5bc947a70ad..8b829748d488 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -436,6 +436,7 @@ class PrinterHelpers(Printer):
             'struct xdp_md',
             'struct path',
             'struct btf_ptr',
+            'struct inode',
     ]
     known_types = {
             '...',
@@ -480,6 +481,7 @@ class PrinterHelpers(Printer):
             'struct task_struct',
             'struct path',
             'struct btf_ptr',
+            'struct inode',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 3ca6146f001a..c3458ec1f30a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3807,6 +3807,16 @@ union bpf_attr {
  * 		See: **clock_gettime**\ (**CLOCK_MONOTONIC_COARSE**)
  * 	Return
  * 		Current *ktime*.
+ *
+ * long bpf_ima_inode_hash(struct inode *inode, void *dst, u32 size)
+ *	Description
+ *		Returns the stored IMA hash of the *inode* (if it's avaialable).
+ *		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 IMA is disabled or **-EINVAL** if
+ *		invalid arguments are passed.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3970,6 +3980,7 @@ union bpf_attr {
 	FN(get_current_task_btf),	\
 	FN(bprm_opts_set),		\
 	FN(ktime_get_coarse_ns),	\
+	FN(ima_inode_hash),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-21  0:50 [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash KP Singh
  2020-11-21  0:50 ` [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
@ 2020-11-21  0:50 ` KP Singh
  2020-11-23 13:24   ` Mimi Zohar
  2020-11-21  6:48 ` [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash Yonghong Song
  2 siblings, 1 reply; 13+ messages in thread
From: KP Singh @ 2020-11-21  0:50 UTC (permalink / raw)
  To: James Morris, linux-kernel, bpf, linux-security-module
  Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman, Mimi Zohar

From: KP Singh <kpsingh@google.com>

- Update the IMA policy before executing the test binary (this is not an
  override of the policy, just an append that ensures that hashes are
  calculated on executions).

- Call the bpf_ima_inode_hash in the bprm_committed_creds hook and check
  if the call succeeded and a hash was calculated.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: KP Singh <kpsingh@google.com>
---
 tools/testing/selftests/bpf/config            |  3 ++
 .../selftests/bpf/prog_tests/test_lsm.c       | 32 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/lsm.c       |  7 +++-
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 2118e23ac07a..4b5764031368 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -39,3 +39,6 @@ CONFIG_BPF_JIT=y
 CONFIG_BPF_LSM=y
 CONFIG_SECURITY=y
 CONFIG_LIRC=y
+CONFIG_IMA=y
+CONFIG_IMA_WRITE_POLICY=y
+CONFIG_IMA_READ_POLICY=y
diff --git a/tools/testing/selftests/bpf/prog_tests/test_lsm.c b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
index 6ab29226c99b..bcb050a296a4 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_lsm.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_lsm.c
@@ -52,6 +52,28 @@ int exec_cmd(int *monitored_pid)
 	return -EINVAL;
 }
 
+#define IMA_POLICY "measure func=BPRM_CHECK"
+
+/* This does not override the policy, IMA policy updates are
+ * append only, so this just ensures that "measure func=BPRM_CHECK"
+ * is in the policy. IMA does not allow us to remove this line once
+ * it is added.
+ */
+static int update_ima_policy(void)
+{
+	int fd, ret = 0;
+
+	fd = open("/sys/kernel/security/ima/policy", O_WRONLY);
+	if (fd < 0)
+		return -errno;
+
+	if (write(fd, IMA_POLICY, sizeof(IMA_POLICY)) == -1)
+		ret = -errno;
+
+	close(fd);
+	return ret;
+}
+
 void test_test_lsm(void)
 {
 	struct lsm *skel = NULL;
@@ -66,6 +88,10 @@ void test_test_lsm(void)
 	if (CHECK(err, "attach", "lsm attach failed: %d\n", err))
 		goto close_prog;
 
+	err = update_ima_policy();
+	if (CHECK(err, "update_ima_policy", "err %d\n", err))
+		goto close_prog;
+
 	err = exec_cmd(&skel->bss->monitored_pid);
 	if (CHECK(err < 0, "exec_cmd", "err %d errno %d\n", err, errno))
 		goto close_prog;
@@ -83,6 +109,12 @@ void test_test_lsm(void)
 	CHECK(skel->bss->mprotect_count != 1, "mprotect_count",
 	      "mprotect_count = %d\n", skel->bss->mprotect_count);
 
+	CHECK(skel->data->ima_hash_ret < 0, "ima_hash_ret",
+	      "ima_hash_ret = %ld\n", skel->data->ima_hash_ret);
+
+	CHECK(skel->bss->ima_hash == 0, "ima_hash",
+	      "ima_hash = %lu\n", skel->bss->ima_hash);
+
 	syscall(__NR_setdomainname, &buf, -2L);
 	syscall(__NR_setdomainname, 0, -3L);
 	syscall(__NR_setdomainname, ~0L, -4L);
diff --git a/tools/testing/selftests/bpf/progs/lsm.c b/tools/testing/selftests/bpf/progs/lsm.c
index ff4d343b94b5..5adc193e414d 100644
--- a/tools/testing/selftests/bpf/progs/lsm.c
+++ b/tools/testing/selftests/bpf/progs/lsm.c
@@ -35,6 +35,8 @@ char _license[] SEC("license") = "GPL";
 int monitored_pid = 0;
 int mprotect_count = 0;
 int bprm_count = 0;
+long ima_hash_ret = -1;
+u64 ima_hash = 0;
 
 SEC("lsm/file_mprotect")
 int BPF_PROG(test_int_hook, struct vm_area_struct *vma,
@@ -65,8 +67,11 @@ int BPF_PROG(test_void_hook, struct linux_binprm *bprm)
 	__u32 key = 0;
 	__u64 *value;
 
-	if (monitored_pid == pid)
+	if (monitored_pid == pid) {
 		bprm_count++;
+		ima_hash_ret = bpf_ima_inode_hash(bprm->file->f_inode,
+						  &ima_hash, sizeof(ima_hash));
+	}
 
 	bpf_copy_from_user(args, sizeof(args), (void *)bprm->vma->vm_mm->arg_start);
 	bpf_copy_from_user(args, sizeof(args), (void *)bprm->mm->arg_start);
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash
  2020-11-21  0:50 [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash KP Singh
  2020-11-21  0:50 ` [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
  2020-11-21  0:50 ` [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
@ 2020-11-21  6:48 ` Yonghong Song
  2 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2020-11-21  6:48 UTC (permalink / raw)
  To: KP Singh, James Morris, linux-kernel, bpf, linux-security-module
  Cc: Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar



On 11/20/20 4:50 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> This is in preparation to add a helper for BPF LSM programs to use
> IMA hashes when attached to LSM hooks. There are LSM hooks like
> inode_unlink which do not have a struct file * argument and cannot
> use the existing ima_file_hash API.
> 
> An inode based API is, therefore, useful in LSM based detections like an
> executable trying to delete itself which rely on the inode_unlink LSM
> hook.
> 
> Moreover, the ima_file_hash function does nothing with the struct file
> pointer apart from calling file_inode on it and converting it to an
> inode.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>

Acked-by: Yonghong Song <yhs@fb.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-21  0:50 ` [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
@ 2020-11-21  6:54   ` Yonghong Song
  0 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2020-11-21  6:54 UTC (permalink / raw)
  To: KP Singh, James Morris, linux-kernel, bpf, linux-security-module
  Cc: Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar



On 11/20/20 4:50 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> Provide a wrapper function to get the IMA hash of an inode. This helper
> is useful in fingerprinting files (e.g executables on execution) and
> using these fingerprints in detections like an executable unlinking
> itself.
> 
> Since the ima_inode_hash can sleep, it's only allowed for sleepable
> LSM hooks.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
Acked-by: Yonghong Song <yhs@fb.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-21  0:50 ` [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
@ 2020-11-23 13:24   ` Mimi Zohar
  2020-11-23 14:06     ` KP Singh
  0 siblings, 1 reply; 13+ messages in thread
From: Mimi Zohar @ 2020-11-23 13:24 UTC (permalink / raw)
  To: KP Singh, James Morris, linux-kernel, bpf, linux-security-module
  Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman

On Sat, 2020-11-21 at 00:50 +0000, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> - Update the IMA policy before executing the test binary (this is not an
>   override of the policy, just an append that ensures that hashes are
>   calculated on executions).

Assuming the builtin policy has been replaced with a custom policy and
CONFIG_IMA_WRITE_POLICY is enabled, then yes the rule is appended.   If
a custom policy has not yet been loaded, loading this rule becomes the
defacto custom policy.

Even if a custom policy has been loaded, potentially additional
measurements unrelated to this test would be included the measurement
list.  One way of limiting a rule to a specific test is by loopback
mounting a file system and defining a policy rule based on the loopback
mount unique uuid.
 
Mimi


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 13:24   ` Mimi Zohar
@ 2020-11-23 14:06     ` KP Singh
  2020-11-23 15:10       ` Mimi Zohar
  0 siblings, 1 reply; 13+ messages in thread
From: KP Singh @ 2020-11-23 14:06 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman

On Mon, Nov 23, 2020 at 2:24 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Sat, 2020-11-21 at 00:50 +0000, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > - Update the IMA policy before executing the test binary (this is not an
> >   override of the policy, just an append that ensures that hashes are
> >   calculated on executions).
>
> Assuming the builtin policy has been replaced with a custom policy and
> CONFIG_IMA_WRITE_POLICY is enabled, then yes the rule is appended.   If
> a custom policy has not yet been loaded, loading this rule becomes the
> defacto custom policy.
>
> Even if a custom policy has been loaded, potentially additional
> measurements unrelated to this test would be included the measurement
> list.  One way of limiting a rule to a specific test is by loopback
> mounting a file system and defining a policy rule based on the loopback
> mount unique uuid.

Thanks Mimi!

I wonder if we simply limit this to policy to /tmp and run an executable
from /tmp (like test_local_storage.c does).

The only side effect would be of extra hashes being calculated on
binaries run from /tmp which is not too bad I guess?

We could do the loop mount too, but I am guessing the most clean way
would be to shell out to mount from the test? Are there some other examples
of IMA we could look at?

- KP

>
> Mimi
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 14:06     ` KP Singh
@ 2020-11-23 15:10       ` Mimi Zohar
  2020-11-23 18:27         ` KP Singh
  0 siblings, 1 reply; 13+ messages in thread
From: Mimi Zohar @ 2020-11-23 15:10 UTC (permalink / raw)
  To: KP Singh
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman, Petr Vorel

[Cc'ing Petr Vorel]

On Mon, 2020-11-23 at 15:06 +0100, KP Singh wrote:
> On Mon, Nov 23, 2020 at 2:24 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Sat, 2020-11-21 at 00:50 +0000, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > - Update the IMA policy before executing the test binary (this is not an
> > >   override of the policy, just an append that ensures that hashes are
> > >   calculated on executions).
> >
> > Assuming the builtin policy has been replaced with a custom policy and
> > CONFIG_IMA_WRITE_POLICY is enabled, then yes the rule is appended.   If
> > a custom policy has not yet been loaded, loading this rule becomes the
> > defacto custom policy.
> >
> > Even if a custom policy has been loaded, potentially additional
> > measurements unrelated to this test would be included the measurement
> > list.  One way of limiting a rule to a specific test is by loopback
> > mounting a file system and defining a policy rule based on the loopback
> > mount unique uuid.
> 
> Thanks Mimi!
> 
> I wonder if we simply limit this to policy to /tmp and run an executable
> from /tmp (like test_local_storage.c does).
> 
> The only side effect would be of extra hashes being calculated on
> binaries run from /tmp which is not too bad I guess?

The builtin measurement policy (ima_policy=tcb") explicitly defines a
rule to not measure /tmp files.  Measuring /tmp results in a lot of
measurements.

{.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC},

> 
> We could do the loop mount too, but I am guessing the most clean way
> would be to shell out to mount from the test? Are there some other examples
> of IMA we could look at?

LTP loopback mounts a filesystem, since /tmp is not being measured with
the builtin "tcb" policy.  Defining new policy rules should be limited
to the loopback mount.  This would pave the way for defining IMA-
appraisal signature verification policy rules, without impacting the
running system.

Mimi


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 15:10       ` Mimi Zohar
@ 2020-11-23 18:27         ` KP Singh
  2020-11-23 18:36           ` Yonghong Song
  0 siblings, 1 reply; 13+ messages in thread
From: KP Singh @ 2020-11-23 18:27 UTC (permalink / raw)
  To: Mimi Zohar, Andrii Nakryiko
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Yonghong Song, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman, Petr Vorel

[...]

> > >
> > > Even if a custom policy has been loaded, potentially additional
> > > measurements unrelated to this test would be included the measurement
> > > list.  One way of limiting a rule to a specific test is by loopback
> > > mounting a file system and defining a policy rule based on the loopback
> > > mount unique uuid.
> >
> > Thanks Mimi!
> >
> > I wonder if we simply limit this to policy to /tmp and run an executable
> > from /tmp (like test_local_storage.c does).
> >
> > The only side effect would be of extra hashes being calculated on
> > binaries run from /tmp which is not too bad I guess?
>
> The builtin measurement policy (ima_policy=tcb") explicitly defines a
> rule to not measure /tmp files.  Measuring /tmp results in a lot of
> measurements.
>
> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC},
>
> >
> > We could do the loop mount too, but I am guessing the most clean way
> > would be to shell out to mount from the test? Are there some other examples
> > of IMA we could look at?
>
> LTP loopback mounts a filesystem, since /tmp is not being measured with
> the builtin "tcb" policy.  Defining new policy rules should be limited
> to the loopback mount.  This would pave the way for defining IMA-
> appraisal signature verification policy rules, without impacting the
> running system.

+Andrii

Do you think we can split the IMA test out,
have a little shell script that does the loopback mount, gets the
FS UUID, updates the IMA policy and then runs a C program?

This would also allow "test_progs" to be independent of CONFIG_IMA.

I am guessing the structure would be something similar
to test_xdp_redirect.sh

- KP

>
> Mimi
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 18:27         ` KP Singh
@ 2020-11-23 18:36           ` Yonghong Song
  2020-11-23 18:46             ` KP Singh
  0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2020-11-23 18:36 UTC (permalink / raw)
  To: KP Singh, Mimi Zohar, Andrii Nakryiko
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Petr Vorel



On 11/23/20 10:27 AM, KP Singh wrote:
> [...]
> 
>>>>
>>>> Even if a custom policy has been loaded, potentially additional
>>>> measurements unrelated to this test would be included the measurement
>>>> list.  One way of limiting a rule to a specific test is by loopback
>>>> mounting a file system and defining a policy rule based on the loopback
>>>> mount unique uuid.
>>>
>>> Thanks Mimi!
>>>
>>> I wonder if we simply limit this to policy to /tmp and run an executable
>>> from /tmp (like test_local_storage.c does).
>>>
>>> The only side effect would be of extra hashes being calculated on
>>> binaries run from /tmp which is not too bad I guess?
>>
>> The builtin measurement policy (ima_policy=tcb") explicitly defines a
>> rule to not measure /tmp files.  Measuring /tmp results in a lot of
>> measurements.
>>
>> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC},
>>
>>>
>>> We could do the loop mount too, but I am guessing the most clean way
>>> would be to shell out to mount from the test? Are there some other examples
>>> of IMA we could look at?
>>
>> LTP loopback mounts a filesystem, since /tmp is not being measured with
>> the builtin "tcb" policy.  Defining new policy rules should be limited
>> to the loopback mount.  This would pave the way for defining IMA-
>> appraisal signature verification policy rules, without impacting the
>> running system.
> 
> +Andrii
> 
> Do you think we can split the IMA test out,
> have a little shell script that does the loopback mount, gets the
> FS UUID, updates the IMA policy and then runs a C program?
> 
> This would also allow "test_progs" to be independent of CONFIG_IMA.
> 
> I am guessing the structure would be something similar
> to test_xdp_redirect.sh

Look at sk_assign test.

sk_assign.c:    if (CHECK_FAIL(system("ip link set dev lo up")))
sk_assign.c:    if (CHECK_FAIL(system("ip route add local default dev lo")))
sk_assign.c:    if (CHECK_FAIL(system("ip -6 route add local default dev 
lo")))
sk_assign.c:    if (CHECK_FAIL(system("tc qdisc add dev lo clsact")))
sk_assign.c:    if (CHECK(system(tc_cmd), "BPF load failed;"

You can use "system" to invoke some bash commands to simulate a script
in the tests.

> 
> - KP
> 
>>
>> Mimi
>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 18:36           ` Yonghong Song
@ 2020-11-23 18:46             ` KP Singh
  2020-11-23 18:54               ` Yonghong Song
  0 siblings, 1 reply; 13+ messages in thread
From: KP Singh @ 2020-11-23 18:46 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Mimi Zohar, Andrii Nakryiko, James Morris, open list, bpf,
	Linux Security Module list, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman, Petr Vorel

On Mon, Nov 23, 2020 at 7:36 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 11/23/20 10:27 AM, KP Singh wrote:
> > [...]
> >
> >>>>
> >>>> Even if a custom policy has been loaded, potentially additional
> >>>> measurements unrelated to this test would be included the measurement
> >>>> list.  One way of limiting a rule to a specific test is by loopback
> >>>> mounting a file system and defining a policy rule based on the loopback
> >>>> mount unique uuid.
> >>>
> >>> Thanks Mimi!
> >>>
> >>> I wonder if we simply limit this to policy to /tmp and run an executable
> >>> from /tmp (like test_local_storage.c does).
> >>>
> >>> The only side effect would be of extra hashes being calculated on
> >>> binaries run from /tmp which is not too bad I guess?
> >>
> >> The builtin measurement policy (ima_policy=tcb") explicitly defines a
> >> rule to not measure /tmp files.  Measuring /tmp results in a lot of
> >> measurements.
> >>
> >> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC},
> >>
> >>>
> >>> We could do the loop mount too, but I am guessing the most clean way
> >>> would be to shell out to mount from the test? Are there some other examples
> >>> of IMA we could look at?
> >>
> >> LTP loopback mounts a filesystem, since /tmp is not being measured with
> >> the builtin "tcb" policy.  Defining new policy rules should be limited
> >> to the loopback mount.  This would pave the way for defining IMA-
> >> appraisal signature verification policy rules, without impacting the
> >> running system.
> >
> > +Andrii
> >
> > Do you think we can split the IMA test out,
> > have a little shell script that does the loopback mount, gets the
> > FS UUID, updates the IMA policy and then runs a C program?
> >
> > This would also allow "test_progs" to be independent of CONFIG_IMA.
> >
> > I am guessing the structure would be something similar
> > to test_xdp_redirect.sh
>
> Look at sk_assign test.
>
> sk_assign.c:    if (CHECK_FAIL(system("ip link set dev lo up")))
> sk_assign.c:    if (CHECK_FAIL(system("ip route add local default dev lo")))
> sk_assign.c:    if (CHECK_FAIL(system("ip -6 route add local default dev
> lo")))
> sk_assign.c:    if (CHECK_FAIL(system("tc qdisc add dev lo clsact")))
> sk_assign.c:    if (CHECK(system(tc_cmd), "BPF load failed;"
>
> You can use "system" to invoke some bash commands to simulate a script
> in the tests.

Heh, that's what I was trying to avoid, I need to parse the output to the get
the name of which loop device was assigned and then call a command like:

# blkid /dev/loop0
/dev/loop0: UUID="607ed7ce-3fad-4236-8faf-8ab744f23e01" TYPE="ext3"

Running simple commands with "system" seems okay but parsing output
is a bit too much :)

I read about:

https://man7.org/linux/man-pages/man4/loop.4.html

But I still need to create a backing file, format it and then get the UUID.

Any simple trick that I may be missing?

- KP

>
> >
> > - KP
> >
> >>
> >> Mimi
> >>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 18:46             ` KP Singh
@ 2020-11-23 18:54               ` Yonghong Song
  2020-11-23 19:00                 ` Yonghong Song
  0 siblings, 1 reply; 13+ messages in thread
From: Yonghong Song @ 2020-11-23 18:54 UTC (permalink / raw)
  To: KP Singh
  Cc: Mimi Zohar, Andrii Nakryiko, James Morris, open list, bpf,
	Linux Security Module list, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman, Petr Vorel



On 11/23/20 10:46 AM, KP Singh wrote:
> On Mon, Nov 23, 2020 at 7:36 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 11/23/20 10:27 AM, KP Singh wrote:
>>> [...]
>>>
>>>>>>
>>>>>> Even if a custom policy has been loaded, potentially additional
>>>>>> measurements unrelated to this test would be included the measurement
>>>>>> list.  One way of limiting a rule to a specific test is by loopback
>>>>>> mounting a file system and defining a policy rule based on the loopback
>>>>>> mount unique uuid.
>>>>>
>>>>> Thanks Mimi!
>>>>>
>>>>> I wonder if we simply limit this to policy to /tmp and run an executable
>>>>> from /tmp (like test_local_storage.c does).
>>>>>
>>>>> The only side effect would be of extra hashes being calculated on
>>>>> binaries run from /tmp which is not too bad I guess?
>>>>
>>>> The builtin measurement policy (ima_policy=tcb") explicitly defines a
>>>> rule to not measure /tmp files.  Measuring /tmp results in a lot of
>>>> measurements.
>>>>
>>>> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = IMA_FSMAGIC},
>>>>
>>>>>
>>>>> We could do the loop mount too, but I am guessing the most clean way
>>>>> would be to shell out to mount from the test? Are there some other examples
>>>>> of IMA we could look at?
>>>>
>>>> LTP loopback mounts a filesystem, since /tmp is not being measured with
>>>> the builtin "tcb" policy.  Defining new policy rules should be limited
>>>> to the loopback mount.  This would pave the way for defining IMA-
>>>> appraisal signature verification policy rules, without impacting the
>>>> running system.
>>>
>>> +Andrii
>>>
>>> Do you think we can split the IMA test out,
>>> have a little shell script that does the loopback mount, gets the
>>> FS UUID, updates the IMA policy and then runs a C program?
>>>
>>> This would also allow "test_progs" to be independent of CONFIG_IMA.
>>>
>>> I am guessing the structure would be something similar
>>> to test_xdp_redirect.sh
>>
>> Look at sk_assign test.
>>
>> sk_assign.c:    if (CHECK_FAIL(system("ip link set dev lo up")))
>> sk_assign.c:    if (CHECK_FAIL(system("ip route add local default dev lo")))
>> sk_assign.c:    if (CHECK_FAIL(system("ip -6 route add local default dev
>> lo")))
>> sk_assign.c:    if (CHECK_FAIL(system("tc qdisc add dev lo clsact")))
>> sk_assign.c:    if (CHECK(system(tc_cmd), "BPF load failed;"
>>
>> You can use "system" to invoke some bash commands to simulate a script
>> in the tests.
> 
> Heh, that's what I was trying to avoid, I need to parse the output to the get
> the name of which loop device was assigned and then call a command like:
> 
> # blkid /dev/loop0
> /dev/loop0: UUID="607ed7ce-3fad-4236-8faf-8ab744f23e01" TYPE="ext3"
> 
> Running simple commands with "system" seems okay but parsing output
> is a bit too much :)
> 
> I read about:
> 
> https://man7.org/linux/man-pages/man4/loop.4.html
> 
> But I still need to create a backing file, format it and then get the UUID.
> 
> Any simple trick that I may be missing?

Maybe you can create a bash script on your prog_test files and do
system("./<>.sh"). In the shell script, you can use all the bash magic
(sed, awk, etc) to parse and store the needed result in a temp file, and
after a successful system(""), you just read that temp file. Does this work?

> - KP
> 
>>
>>>
>>> - KP
>>>
>>>>
>>>> Mimi
>>>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-23 18:54               ` Yonghong Song
@ 2020-11-23 19:00                 ` Yonghong Song
  0 siblings, 0 replies; 13+ messages in thread
From: Yonghong Song @ 2020-11-23 19:00 UTC (permalink / raw)
  To: KP Singh
  Cc: Mimi Zohar, Andrii Nakryiko, James Morris, open list, bpf,
	Linux Security Module list, Alexei Starovoitov, Daniel Borkmann,
	Florent Revest, Brendan Jackman, Petr Vorel



On 11/23/20 10:54 AM, Yonghong Song wrote:
> 
> 
> On 11/23/20 10:46 AM, KP Singh wrote:
>> On Mon, Nov 23, 2020 at 7:36 PM Yonghong Song <yhs@fb.com> wrote:
>>>
>>>
>>>
>>> On 11/23/20 10:27 AM, KP Singh wrote:
>>>> [...]
>>>>
>>>>>>>
>>>>>>> Even if a custom policy has been loaded, potentially additional
>>>>>>> measurements unrelated to this test would be included the 
>>>>>>> measurement
>>>>>>> list.  One way of limiting a rule to a specific test is by loopback
>>>>>>> mounting a file system and defining a policy rule based on the 
>>>>>>> loopback
>>>>>>> mount unique uuid.
>>>>>>
>>>>>> Thanks Mimi!
>>>>>>
>>>>>> I wonder if we simply limit this to policy to /tmp and run an 
>>>>>> executable
>>>>>> from /tmp (like test_local_storage.c does).
>>>>>>
>>>>>> The only side effect would be of extra hashes being calculated on
>>>>>> binaries run from /tmp which is not too bad I guess?
>>>>>
>>>>> The builtin measurement policy (ima_policy=tcb") explicitly defines a
>>>>> rule to not measure /tmp files.  Measuring /tmp results in a lot of
>>>>> measurements.
>>>>>
>>>>> {.action = DONT_MEASURE, .fsmagic = TMPFS_MAGIC, .flags = 
>>>>> IMA_FSMAGIC},
>>>>>
>>>>>>
>>>>>> We could do the loop mount too, but I am guessing the most clean way
>>>>>> would be to shell out to mount from the test? Are there some other 
>>>>>> examples
>>>>>> of IMA we could look at?
>>>>>
>>>>> LTP loopback mounts a filesystem, since /tmp is not being measured 
>>>>> with
>>>>> the builtin "tcb" policy.  Defining new policy rules should be limited
>>>>> to the loopback mount.  This would pave the way for defining IMA-
>>>>> appraisal signature verification policy rules, without impacting the
>>>>> running system.
>>>>
>>>> +Andrii
>>>>
>>>> Do you think we can split the IMA test out,
>>>> have a little shell script that does the loopback mount, gets the
>>>> FS UUID, updates the IMA policy and then runs a C program?
>>>>
>>>> This would also allow "test_progs" to be independent of CONFIG_IMA.
>>>>
>>>> I am guessing the structure would be something similar
>>>> to test_xdp_redirect.sh
>>>
>>> Look at sk_assign test.
>>>
>>> sk_assign.c:    if (CHECK_FAIL(system("ip link set dev lo up")))
>>> sk_assign.c:    if (CHECK_FAIL(system("ip route add local default dev 
>>> lo")))
>>> sk_assign.c:    if (CHECK_FAIL(system("ip -6 route add local default dev
>>> lo")))
>>> sk_assign.c:    if (CHECK_FAIL(system("tc qdisc add dev lo clsact")))
>>> sk_assign.c:    if (CHECK(system(tc_cmd), "BPF load failed;"
>>>
>>> You can use "system" to invoke some bash commands to simulate a script
>>> in the tests.
>>
>> Heh, that's what I was trying to avoid, I need to parse the output to 
>> the get
>> the name of which loop device was assigned and then call a command like:
>>
>> # blkid /dev/loop0
>> /dev/loop0: UUID="607ed7ce-3fad-4236-8faf-8ab744f23e01" TYPE="ext3"
>>
>> Running simple commands with "system" seems okay but parsing output
>> is a bit too much :)
>>
>> I read about:
>>
>> https://man7.org/linux/man-pages/man4/loop.4.html 
>>
>> But I still need to create a backing file, format it and then get the 
>> UUID.
>>
>> Any simple trick that I may be missing?
> 
> Maybe you can create a bash script on your prog_test files and do
> system("./<>.sh"). In the shell script, you can use all the bash magic
> (sed, awk, etc) to parse and store the needed result in a temp file, and
> after a successful system(""), you just read that temp file. Does this 
> work?

I guess under the current framework, you can also create a .sh file
manually and place it into tools/testing/selftests/bpf directory
and call it in your prog_tests .c file with system("./<>.sh")...

> 
>> - KP
>>
>>>
>>>>
>>>> - KP
>>>>
>>>>>
>>>>> Mimi
>>>>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-11-23 19:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-21  0:50 [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash KP Singh
2020-11-21  0:50 ` [PATCH bpf-next v2 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
2020-11-21  6:54   ` Yonghong Song
2020-11-21  0:50 ` [PATCH bpf-next v2 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
2020-11-23 13:24   ` Mimi Zohar
2020-11-23 14:06     ` KP Singh
2020-11-23 15:10       ` Mimi Zohar
2020-11-23 18:27         ` KP Singh
2020-11-23 18:36           ` Yonghong Song
2020-11-23 18:46             ` KP Singh
2020-11-23 18:54               ` Yonghong Song
2020-11-23 19:00                 ` Yonghong Song
2020-11-21  6:48 ` [PATCH bpf-next v2 1/3] ima: Implement ima_inode_hash Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).