diff mbox series

[bpf-next,2/3] bpf: Add a BPF helper for getting the IMA hash of an inode

Message ID 20201120131708.3237864-2-kpsingh@chromium.org (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next,1/3] ima: Implement ima_inode_hash | expand

Checks

Context Check Description
netdev/cover_letter warning Series does not have a cover letter
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 15752 this patch: 15753
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 97 lines checked
netdev/build_allmodconfig_warn fail Errors and warnings before: 15664 this patch: 15665
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

KP Singh Nov. 20, 2020, 1:17 p.m. UTC 
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(+)

Comments

Yonghong Song Nov. 20, 2020, 5:47 p.m. UTC | #1 
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
>
KP Singh Nov. 21, 2020, 12:14 a.m. UTC | #2 
[...]

> > + * 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,
> > +};

[...]
Alexei Starovoitov Nov. 24, 2020, 4:02 a.m. UTC | #3 
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.
KP Singh Nov. 24, 2020, 11:04 a.m. UTC | #4 
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;
KP Singh Nov. 24, 2020, 3:01 p.m. UTC | #5 
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);
diff mbox series

Patch

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