linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 1/3] ima: Implement ima_inode_hash
@ 2020-11-20 13:17 KP Singh
  2020-11-20 13:17 ` [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: KP Singh @ 2020-11-20 13:17 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 +++
 scripts/bpf_helpers_doc.py        |  1 +
 security/integrity/ima/ima_main.c | 74 ++++++++++++++++++++++---------
 3 files changed, 59 insertions(+), 22 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/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index c5bc947a70ad..add7fcb32dcd 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -478,6 +478,7 @@ class PrinterHelpers(Printer):
             'struct tcp_request_sock',
             'struct udp6_sock',
             'struct task_struct',
+            'struct inode',
             'struct path',
             'struct btf_ptr',
     }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2d1af8899cab..1dd2123b5b43 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -501,37 +501,17 @@ 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)
+	if (!inode)
 		return -EINVAL;
 
 	if (!ima_policy_flag)
 		return -EOPNOTSUPP;
 
-	inode = file_inode(file);
 	iint = integrity_iint_find(inode);
 	if (!iint)
 		return -EOPNOTSUPP;
@@ -558,8 +538,58 @@ 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)
+{
+	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] 12+ messages in thread

* [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-20 13:17 [PATCH bpf-next 1/3] ima: Implement ima_inode_hash KP Singh
@ 2020-11-20 13:17 ` KP Singh
  2020-11-20 17:47   ` Yonghong Song
  2020-11-24  4:02   ` Alexei Starovoitov
  2020-11-20 13:17 ` [PATCH bpf-next 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
  2020-11-20 17:32 ` [PATCH bpf-next 1/3] ima: Implement ima_inode_hash Yonghong Song
  2 siblings, 2 replies; 12+ messages in thread
From: KP Singh @ 2020-11-20 13:17 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     |  1 +
 tools/include/uapi/linux/bpf.h | 11 +++++++++++
 4 files changed, 49 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3ca6146f001a..dd5b8622bb89 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** of is returned on success,
+ *		**-EOPNOTSUP** if IMA is disabled and **-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..51c36f61339e 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_OR_ZERO,
+	.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 add7fcb32dcd..cb16687acb66 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -430,6 +430,7 @@ class PrinterHelpers(Printer):
             'struct tcp_request_sock',
             'struct udp6_sock',
             'struct task_struct',
+            'struct inode',
 
             'struct __sk_buff',
             'struct sk_msg_md',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 3ca6146f001a..dd5b8622bb89 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** of is returned on success,
+ *		**-EOPNOTSUP** if IMA is disabled and **-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] 12+ messages in thread

* [PATCH bpf-next 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-20 13:17 [PATCH bpf-next 1/3] ima: Implement ima_inode_hash KP Singh
  2020-11-20 13:17 ` [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
@ 2020-11-20 13:17 ` KP Singh
  2020-11-20 18:11   ` Yonghong Song
  2020-11-20 17:32 ` [PATCH bpf-next 1/3] ima: Implement ima_inode_hash Yonghong Song
  2 siblings, 1 reply; 12+ messages in thread
From: KP Singh @ 2020-11-20 13:17 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>

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

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..3f5d64adb233 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 != 0, "update_ima_policy", "error = %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 = %d\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..b0f9639e4b0a 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;
+int 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] 12+ messages in thread

* Re: [PATCH bpf-next 1/3] ima: Implement ima_inode_hash
  2020-11-20 13:17 [PATCH bpf-next 1/3] ima: Implement ima_inode_hash KP Singh
  2020-11-20 13:17 ` [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
  2020-11-20 13:17 ` [PATCH bpf-next 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
@ 2020-11-20 17:32 ` Yonghong Song
  2020-11-21  0:08   ` KP Singh
  2 siblings, 1 reply; 12+ messages in thread
From: Yonghong Song @ 2020-11-20 17:32 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 5:17 AM, 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>
> ---
>   include/linux/ima.h               |  6 +++
>   scripts/bpf_helpers_doc.py        |  1 +
>   security/integrity/ima/ima_main.c | 74 ++++++++++++++++++++++---------
>   3 files changed, 59 insertions(+), 22 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/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
> index c5bc947a70ad..add7fcb32dcd 100755
> --- a/scripts/bpf_helpers_doc.py
> +++ b/scripts/bpf_helpers_doc.py
> @@ -478,6 +478,7 @@ class PrinterHelpers(Printer):
>               'struct tcp_request_sock',
>               'struct udp6_sock',
>               'struct task_struct',
> +            'struct inode',

This change (bpf_helpers_doc.py) belongs to patch #2.

>               'struct path',
>               'struct btf_ptr',
>       }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2d1af8899cab..1dd2123b5b43 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -501,37 +501,17 @@ 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)
> +	if (!inode)
>   		return -EINVAL;

Based on original code, for ima_file_hash(), inode cannot be NULL,
so I prefer to remove this change here and add !inode test in
ima_inode_hash.


>   
>   	if (!ima_policy_flag)
>   		return -EOPNOTSUPP;
>   
> -	inode = file_inode(file);
>   	iint = integrity_iint_find(inode);
>   	if (!iint)
>   		return -EOPNOTSUPP;
> @@ -558,8 +538,58 @@ 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)
> +{

Add
	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
> 

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

* Re: [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-20 13:17 ` [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
@ 2020-11-20 17:47   ` Yonghong Song
  2020-11-21  0:14     ` KP Singh
  2020-11-24  4:02   ` Alexei Starovoitov
  1 sibling, 1 reply; 12+ messages in thread
From: Yonghong Song @ 2020-11-20 17:47 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 5:17 AM, 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>
> ---
>   include/uapi/linux/bpf.h       | 11 +++++++++++
>   kernel/bpf/bpf_lsm.c           | 26 ++++++++++++++++++++++++++
>   scripts/bpf_helpers_doc.py     |  1 +
>   tools/include/uapi/linux/bpf.h | 11 +++++++++++
>   4 files changed, 49 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3ca6146f001a..dd5b8622bb89 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** of is returned on success,

of => if?

> + *		**-EOPNOTSUP** if IMA is disabled and **-EINVAL** if

and => or

> + *		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..51c36f61339e 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_OR_ZERO,

I know ARG_CONST_SIZE_OR_ZERO provides some flexibility and may
make verifier easier to verify programs. But beyond that did
you see any real use case user will pass a zero size buf to
get hash value?

> +	.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 add7fcb32dcd..cb16687acb66 100755
> --- a/scripts/bpf_helpers_doc.py
> +++ b/scripts/bpf_helpers_doc.py
> @@ -430,6 +430,7 @@ class PrinterHelpers(Printer):
>               'struct tcp_request_sock',
>               'struct udp6_sock',
>               'struct task_struct',
> +            'struct inode',
>   
>               'struct __sk_buff',
>               'struct sk_msg_md',
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 3ca6146f001a..dd5b8622bb89 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** of is returned on success,

of => if?

> + *		**-EOPNOTSUP** if IMA is disabled and **-EINVAL** if

and => or.

> + *		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
> 

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

* Re: [PATCH bpf-next 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash
  2020-11-20 13:17 ` [PATCH bpf-next 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
@ 2020-11-20 18:11   ` Yonghong Song
  2020-11-21  0:20     ` KP Singh
  0 siblings, 1 reply; 12+ messages in thread
From: Yonghong Song @ 2020-11-20 18:11 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 5:17 AM, 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).
> 
> - Call the bpf_ima_inode_hash in the bprm_committed_creds hook and check
>    if the call succeeded and a hash was calculated.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>

LGTM with a few nits below.

Acked-by: Yonghong Song <yhs@fb.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..3f5d64adb233 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;
>   }
>   
[...]
> +
>   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 != 0, "update_ima_policy", "error = %d\n", err))
> +		goto close_prog;

"err != 0" => err?
"error = %d" => "err %d" for consistency with other usage in this function.

> +
>   	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 = %d\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..b0f9639e4b0a 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;
> +int ima_hash_ret = -1;

The helper returns type "long", but "int" type here should be fine too.

> +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);
> 

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

* Re: [PATCH bpf-next 1/3] ima: Implement ima_inode_hash
  2020-11-20 17:32 ` [PATCH bpf-next 1/3] ima: Implement ima_inode_hash Yonghong Song
@ 2020-11-21  0:08   ` KP Singh
  0 siblings, 0 replies; 12+ messages in thread
From: KP Singh @ 2020-11-21  0:08 UTC (permalink / raw)
  To: Yonghong Song
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

[...]

> >
> > diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
> > index c5bc947a70ad..add7fcb32dcd 100755
> > --- a/scripts/bpf_helpers_doc.py
> > +++ b/scripts/bpf_helpers_doc.py
> > @@ -478,6 +478,7 @@ class PrinterHelpers(Printer):
> >               'struct tcp_request_sock',
> >               'struct udp6_sock',
> >               'struct task_struct',
> > +            'struct inode',
>
> This change (bpf_helpers_doc.py) belongs to patch #2.

Indeed, I missed it during a rebase. Thanks!


>
> >               'struct path',
> >               'struct btf_ptr',
> >       }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 2d1af8899cab..1dd2123b5b43 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -501,37 +501,17 @@ 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)
> > +     if (!inode)
> >               return -EINVAL;
>
> Based on original code, for ima_file_hash(), inode cannot be NULL,
> so I prefer to remove this change here and add !inode test in
> ima_inode_hash.

Makes sense. Thanks!

>
>
> >

[...]


> > + * If the parameters are incorrect, return -EINVAL.
> > + */
> > +int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size)
> > +{
>
> Add
>         if (!inode)
>                 return -EINVAL;

Done.

>
>
> > +     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
> >

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

* Re: [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-20 17:47   ` Yonghong Song
@ 2020-11-21  0:14     ` KP Singh
  0 siblings, 0 replies; 12+ messages in thread
From: KP Singh @ 2020-11-21  0:14 UTC (permalink / raw)
  To: Yonghong Song
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

[...]

> > + * 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** of is returned on success,
>
> of => if?

Just changed it to:

"The **hash_algo** is returned on success"

>
> > + *           **-EOPNOTSUP** if IMA is disabled and **-EINVAL** if
>
> and => or

Done. (and the same for tools/)

>

[...]

> > +     .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_OR_ZERO,
>
> I know ARG_CONST_SIZE_OR_ZERO provides some flexibility and may
> make verifier easier to verify programs. But beyond that did
> you see any real use case user will pass a zero size buf to
> get hash value?
>

I agree, in this case it makes more sense to ARG_CONST_SIZE.

> > +     .allowed        = bpf_ima_inode_hash_allowed,
> > +};

[...]

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

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

On Fri, Nov 20, 2020 at 7:11 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 11/20/20 5:17 AM, 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).
> >
> > - Call the bpf_ima_inode_hash in the bprm_committed_creds hook and check
> >    if the call succeeded and a hash was calculated.
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
>
> LGTM with a few nits below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
> > ---
> >   tools/testing/selftests/bpf/config            |  3 ++

[...]

> >   }
> >
> [...]
> > +
> >   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 != 0, "update_ima_policy", "error = %d\n", err))
> > +             goto close_prog;
>
> "err != 0" => err?
> "error = %d" => "err %d" for consistency with other usage in this function.

Done.

>
> > +
> >       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)

[...]

> >   int mprotect_count = 0;
> >   int bprm_count = 0;
> > +int ima_hash_ret = -1;
>
> The helper returns type "long", but "int" type here should be fine too.

Changed it to long for correctness.

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

* Re: [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-20 13:17 ` [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
  2020-11-20 17:47   ` Yonghong Song
@ 2020-11-24  4:02   ` Alexei Starovoitov
  2020-11-24 11:04     ` KP Singh
  1 sibling, 1 reply; 12+ messages in thread
From: Alexei Starovoitov @ 2020-11-24  4:02 UTC (permalink / raw)
  To: KP Singh
  Cc: James Morris, linux-kernel, bpf, linux-security-module,
	Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

On Fri, Nov 20, 2020 at 01:17:07PM +0000, KP Singh wrote:
> +
> +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_OR_ZERO,
> +	.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;

That's not enough for correctness.
Not only hook has to sleepable, but the program has to be sleepable too.
The patch 3 should be causing all sort of kernel warnings
for calling mutex from preempt disabled.
There it calls bpf_ima_inode_hash() from SEC("lsm/file_mprotect") program.
"lsm/" is non-sleepable. "lsm.s/" is.
please enable CONFIG_DEBUG_ATOMIC_SLEEP=y in your config.

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

* Re: [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-24  4:02   ` Alexei Starovoitov
@ 2020-11-24 11:04     ` KP Singh
  2020-11-24 15:01       ` KP Singh
  0 siblings, 1 reply; 12+ messages in thread
From: KP Singh @ 2020-11-24 11:04 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

On Tue, Nov 24, 2020 at 5:02 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 01:17:07PM +0000, KP Singh wrote:
> > +
> > +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_OR_ZERO,
> > +     .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;
>
> That's not enough for correctness.
> Not only hook has to sleepable, but the program has to be sleepable too.
> The patch 3 should be causing all sort of kernel warnings
> for calling mutex from preempt disabled.
> There it calls bpf_ima_inode_hash() from SEC("lsm/file_mprotect") program.

I did actually mean to use SEC("lsm.s/bprm_committed_creds"), my bad.

> "lsm/" is non-sleepable. "lsm.s/" is.
> please enable CONFIG_DEBUG_ATOMIC_SLEEP=y in your config.

Oops, yes I did notice that during recent work on the test cases.

Since we need a stronger check than just warnings, I am doing
something similar to
what we do for bpf_copy_from_user i.e.

     return prog->aux->sleepable ? &bpf_ima_inode_hash_proto : NULL;

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

* Re: [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode
  2020-11-24 11:04     ` KP Singh
@ 2020-11-24 15:01       ` KP Singh
  0 siblings, 0 replies; 12+ messages in thread
From: KP Singh @ 2020-11-24 15:01 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: James Morris, open list, bpf, Linux Security Module list,
	Alexei Starovoitov, Daniel Borkmann, Florent Revest,
	Brendan Jackman, Mimi Zohar

On Tue, Nov 24, 2020 at 12:04 PM KP Singh <kpsingh@chromium.org> wrote:
>
> On Tue, Nov 24, 2020 at 5:02 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Fri, Nov 20, 2020 at 01:17:07PM +0000, KP Singh wrote:
> > > +
> > > +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_OR_ZERO,
> > > +     .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;
> >
> > That's not enough for correctness.
> > Not only hook has to sleepable, but the program has to be sleepable too.
> > The patch 3 should be causing all sort of kernel warnings
> > for calling mutex from preempt disabled.
> > There it calls bpf_ima_inode_hash() from SEC("lsm/file_mprotect") program.

Okay I dug into why I did not get any warnings, I do have
CONFIG_DEBUG_ATOMIC_SLEEP
and friends enabled and I do look at dmesg and... I think you misread
the diff of my patch :)

it's indeed attaching to "lsm.s/bprm_committed_creds":

[https://lore.kernel.org/bpf/CACYkzJ7Oi8wXf=9a-e=fFHJirRbD=u47z+3+M2cRTCy_1fwtgw@mail.gmail.com/T/#m8d55bf0cdda614338cecd7154476497628612f6a]

 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);
-- 

The diff makes it look like it is attaching to "lsm/file_mprotect" but
it's actually attaching to
"lsm.s/bprm_committed_creds".

Now we can either check for prod->aux->sleepable in
bpf_ima_inode_hash_allowed or
just not expose the helper to non-sleepable hooks. I went with the
latter as this is what
we do for bpf_copy_from_user.

- KP

>
> I did actually mean to use SEC("lsm.s/bprm_committed_creds"), my bad.
>
> > "lsm/" is non-sleepable. "lsm.s/" is.
> > please enable CONFIG_DEBUG_ATOMIC_SLEEP=y in your config.
>
> Oops, yes I did notice that during recent work on the test cases.
>
> Since we need a stronger check than just warnings, I am doing
> something similar to
> what we do for bpf_copy_from_user i.e.
>
>      return prog->aux->sleepable ? &bpf_ima_inode_hash_proto : NULL;

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

end of thread, other threads:[~2020-11-24 15:01 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 13:17 [PATCH bpf-next 1/3] ima: Implement ima_inode_hash KP Singh
2020-11-20 13:17 ` [PATCH bpf-next 2/3] bpf: Add a BPF helper for getting the IMA hash of an inode KP Singh
2020-11-20 17:47   ` Yonghong Song
2020-11-21  0:14     ` KP Singh
2020-11-24  4:02   ` Alexei Starovoitov
2020-11-24 11:04     ` KP Singh
2020-11-24 15:01       ` KP Singh
2020-11-20 13:17 ` [PATCH bpf-next 3/3] bpf: Update LSM selftests for bpf_ima_inode_hash KP Singh
2020-11-20 18:11   ` Yonghong Song
2020-11-21  0:20     ` KP Singh
2020-11-20 17:32 ` [PATCH bpf-next 1/3] ima: Implement ima_inode_hash Yonghong Song
2020-11-21  0:08   ` KP Singh

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