[RFC] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()

  From: Roberto Sassu <roberto.sassu@huawei.com>

BPF LSM allows security modules to directly attach to the security hooks,
with the potential of not meeting the kernel expectation.

This is the case for the inode_init_security hook, for which the kernel
expects that name and value are set if the hook implementation returns
zero.

Consequently, not meeting the kernel expectation can cause the kernel to
crash. One example is evm_protected_xattr_common() which expects the
req_xattr_name parameter to be always not NULL.

Introduce a level of indirection in BPF LSM, for the inode_init_security
hook, to check the validity of the name and value set by security modules.

Encapsulate bpf_lsm_inode_init_security(), the existing attachment point,
with bpf_inode_init_security(), the new function. After the attachment
point is called, return -EOPNOTSUPP if the xattr name is not set, -ENOMEM
if the xattr value is not set.

As the name still cannot be set, rely on future patches to the eBPF
verifier or introducing new kfuncs/helpers to ensure its correctness.

Finally, as proposed by Nicolas, update the LSM hook documentation for the
inode_init_security hook, to reflect the current behavior (only the xattr
value is allocated).

Cc: stable@vger.kernel.org
Fixes: 520b7aa00d8cd ("bpf: lsm: Initialize the BPF LSM hooks")
Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/lsm_hooks.h |  4 ++--
 security/bpf/hooks.c      | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)
  

On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> BPF LSM allows security modules to directly attach to the security hooks,
> with the potential of not meeting the kernel expectation.
>
> This is the case for the inode_init_security hook, for which the kernel
> expects that name and value are set if the hook implementation returns
> zero.
>
> Consequently, not meeting the kernel expectation can cause the kernel to
> crash. One example is evm_protected_xattr_common() which expects the
> req_xattr_name parameter to be always not NULL.

Sounds like a bug in evm_protected_xattr_common.

> Introduce a level of indirection in BPF LSM, for the inode_init_security
> hook, to check the validity of the name and value set by security modules.

Doesn't make sense.
You probably meant security_old_inode_init_security,
because the hook without _old_ doesn't have such args:
int security_inode_init_security(struct inode *inode, struct inode *dir,
                                 const struct qstr *qstr,
                                 initxattrs initxattrs, void *fs_data);

> Encapsulate bpf_lsm_inode_init_security(), the existing attachment point,
> with bpf_inode_init_security(), the new function. After the attachment
> point is called, return -EOPNOTSUPP if the xattr name is not set, -ENOMEM
> if the xattr value is not set.
>
> As the name still cannot be set, rely on future patches to the eBPF
> verifier or introducing new kfuncs/helpers to ensure its correctness.
>
> Finally, as proposed by Nicolas, update the LSM hook documentation for the
> inode_init_security hook, to reflect the current behavior (only the xattr
> value is allocated).
>
> Cc: stable@vger.kernel.org
> Fixes: 520b7aa00d8cd ("bpf: lsm: Initialize the BPF LSM hooks")
> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/linux/lsm_hooks.h |  4 ++--
>  security/bpf/hooks.c      | 25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e..f44d45f4737f 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -229,8 +229,8 @@
>   *     This hook is called by the fs code as part of the inode creation
>   *     transaction and provides for atomic labeling of the inode, unlike
>   *     the post_create/mkdir/... hooks called by the VFS.  The hook function
> - *     is expected to allocate the name and value via kmalloc, with the caller
> - *     being responsible for calling kfree after using them.
> + *     is expected to allocate the value via kmalloc, with the caller
> + *     being responsible for calling kfree after using it.

must be an obsolete comment.

>   *     If the security module does not use security attributes or does
>   *     not wish to put a security attribute on this particular inode,
>   *     then it should return -EOPNOTSUPP to skip this processing.
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> index e5971fa74fd7..492c07ba6722 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -6,11 +6,36 @@
>  #include <linux/lsm_hooks.h>
>  #include <linux/bpf_lsm.h>
>
> +static int bpf_inode_init_security(struct inode *inode, struct inode *dir,
> +                                  const struct qstr *qstr, const char **name,
> +                                  void **value, size_t *len)
> +{
> +       int ret;
> +
> +       ret = bpf_lsm_inode_init_security(inode, dir, qstr, name, value, len);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * As the name cannot be set by the eBPF programs directly, eBPF will
> +        * be responsible for its correctness through the verifier or
> +        * appropriate kfuncs/helpers.
> +        */
> +       if (name && !*name)
> +               return -EOPNOTSUPP;

bpf cannot write into such pointers.
It won't be able to use kfuncs to kmalloc and write into them either.
None of it makes sense to me.

> +
> +       if (value && !*value)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
>  static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
>         #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
>         LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
>         #include <linux/lsm_hook_defs.h>
>         #undef LSM_HOOK
> +       LSM_HOOK_INIT(inode_init_security, bpf_inode_init_security),
>         LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
>         LSM_HOOK_INIT(task_free, bpf_task_storage_free),
>  };
> --
> 2.25.1
>
  

On Sun, 2022-10-23 at 16:36 -0700, Alexei Starovoitov wrote:

Sorry, forgot to CC Mimi and linux-integrity.

> On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > BPF LSM allows security modules to directly attach to the security
> > hooks,
> > with the potential of not meeting the kernel expectation.
> > 
> > This is the case for the inode_init_security hook, for which the
> > kernel
> > expects that name and value are set if the hook implementation
> > returns
> > zero.
> > 
> > Consequently, not meeting the kernel expectation can cause the
> > kernel to
> > crash. One example is evm_protected_xattr_common() which expects
> > the
> > req_xattr_name parameter to be always not NULL.
> 
> Sounds like a bug in evm_protected_xattr_common.

If an LSM implementing the inode_init_security hook returns -EOPNOTSUPP
or -ENOMEM, evm_protected_xattr_common() is not going to be executed.

This is documented in include/linux/lsm_hooks.h

Why it would be a bug in evm_protected_xattr_common()?

> > Introduce a level of indirection in BPF LSM, for the
> > inode_init_security
> > hook, to check the validity of the name and value set by security
> > modules.
> 
> Doesn't make sense.

Look at this example. The LSM infrastructure has a convention on return
values for the hooks (maybe there is something similar for other
hooks). The code calling the hooks relies on such conventions. If
conventions are not followed a panic occurs.

If LSMs go to the kernel, their code is checked for compliance with the
conventions. However, this does not happen for security modules
attached to the BPF LSM, because BPF LSM directly executes the eBPF
programs without further checks.

I was able to trigger the panic with this simple eBPF program:

SEC("lsm/inode_init_security")
int BPF_PROG(test_int_hook, struct inode *inode,
	 struct inode *dir, const struct qstr *qstr, const char **name,
	 void **value, size_t *len)
{
	return 0;
}

In my opinion, the level of indirection is necessary to ensure that
kernel expectations are met.

> You probably meant security_old_inode_init_security,
> because the hook without _old_ doesn't have such args:
> int security_inode_init_security(struct inode *inode, struct inode
> *dir,
>                                  const struct qstr *qstr,
>                                  initxattrs initxattrs, void
> *fs_data);

I meant inode_init_security. The signature of the hook is different
from that of security_inode_init_security():

LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
	 struct inode *dir, const struct qstr *qstr, const char **name,
	 void **value, size_t *len)

BPF LSM programs attach to the attachment points defined with:

#define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
{						\
	return DEFAULT;				\
}

#include <linux/lsm_hook_defs.h>
#undef LSM_HOOK

> Encapsulate bpf_lsm_inode_init_security(), the existing attachment
> > point,
> > with bpf_inode_init_security(), the new function. After the
> > attachment
> > point is called, return -EOPNOTSUPP if the xattr name is not set,
> > -ENOMEM
> > if the xattr value is not set.
> > 
> > As the name still cannot be set, rely on future patches to the eBPF
> > verifier or introducing new kfuncs/helpers to ensure its
> > correctness.
> > 
> > Finally, as proposed by Nicolas, update the LSM hook documentation
> > for the
> > inode_init_security hook, to reflect the current behavior (only the
> > xattr
> > value is allocated).
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 520b7aa00d8cd ("bpf: lsm: Initialize the BPF LSM hooks")
> > Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  include/linux/lsm_hooks.h |  4 ++--
> >  security/bpf/hooks.c      | 25 +++++++++++++++++++++++++
> >  2 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 4ec80b96c22e..f44d45f4737f 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -229,8 +229,8 @@
> >   *     This hook is called by the fs code as part of the inode
> > creation
> >   *     transaction and provides for atomic labeling of the inode,
> > unlike
> >   *     the post_create/mkdir/... hooks called by the VFS.  The
> > hook function
> > - *     is expected to allocate the name and value via kmalloc,
> > with the caller
> > - *     being responsible for calling kfree after using them.
> > + *     is expected to allocate the value via kmalloc, with the
> > caller
> > + *     being responsible for calling kfree after using it.
> 
> must be an obsolete comment.
> 
> >   *     If the security module does not use security attributes or
> > does
> >   *     not wish to put a security attribute on this particular
> > inode,
> >   *     then it should return -EOPNOTSUPP to skip this processing.
> > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> > index e5971fa74fd7..492c07ba6722 100644
> > --- a/security/bpf/hooks.c
> > +++ b/security/bpf/hooks.c
> > @@ -6,11 +6,36 @@
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/bpf_lsm.h>
> > 
> > +static int bpf_inode_init_security(struct inode *inode, struct
> > inode *dir,
> > +                                  const struct qstr *qstr, const
> > char **name,
> > +                                  void **value, size_t *len)
> > +{
> > +       int ret;
> > +
> > +       ret = bpf_lsm_inode_init_security(inode, dir, qstr, name,
> > value, len);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /*
> > +        * As the name cannot be set by the eBPF programs directly,
> > eBPF will
> > +        * be responsible for its correctness through the verifier
> > or
> > +        * appropriate kfuncs/helpers.
> > +        */
> > +       if (name && !*name)
> > +               return -EOPNOTSUPP;
> 
> bpf cannot write into such pointers.
> It won't be able to use kfuncs to kmalloc and write into them either.
> None of it makes sense to me.

Ok, so it is a technical limitation not being able to implement the
inode_init_security hook in eBPF. Should we always return -EOPNOTSUPP
even if eBPF programs are successully attached to inode_init_security?

Thanks

Roberto

> > +
> > +       if (value && !*value)
> > +               return -ENOMEM;
> > +
> > +       return 0;
> > +}
> > +
> >  static struct security_hook_list bpf_lsm_hooks[]
> > __lsm_ro_after_init = {
> >         #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> >         LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
> >         #include <linux/lsm_hook_defs.h>
> >         #undef LSM_HOOK
> > +       LSM_HOOK_INIT(inode_init_security,
> > bpf_inode_init_security),
> >         LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
> >         LSM_HOOK_INIT(task_free, bpf_task_storage_free),
> >  };
> > --
> > 2.25.1
> >
  

On Mon, 2022-10-24 at 11:25 +0200, Roberto Sassu wrote:
> On Sun, 2022-10-23 at 16:36 -0700, Alexei Starovoitov wrote:
> 
> Sorry, forgot to CC Mimi and linux-integrity.
> 
> > On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > BPF LSM allows security modules to directly attach to the security
> > > hooks,
> > > with the potential of not meeting the kernel expectation.
> > > 
> > > This is the case for the inode_init_security hook, for which the
> > > kernel
> > > expects that name and value are set if the hook implementation
> > > returns
> > > zero.
> > > 
> > > Consequently, not meeting the kernel expectation can cause the
> > > kernel to
> > > crash. One example is evm_protected_xattr_common() which expects
> > > the
> > > req_xattr_name parameter to be always not NULL.
> > 
> > Sounds like a bug in evm_protected_xattr_common.
> 
> If an LSM implementing the inode_init_security hook returns -EOPNOTSUPP
> or -ENOMEM, evm_protected_xattr_common() is not going to be executed.
> 
> This is documented in include/linux/lsm_hooks.h
> 
> Why it would be a bug in evm_protected_xattr_common()?
> 
> > > Introduce a level of indirection in BPF LSM, for the
> > > inode_init_security
> > > hook, to check the validity of the name and value set by security
> > > modules.
> > 
> > Doesn't make sense.
> 
> Look at this example. The LSM infrastructure has a convention on return
> values for the hooks (maybe there is something similar for other
> hooks). The code calling the hooks relies on such conventions. If
> conventions are not followed a panic occurs.
> 
> If LSMs go to the kernel, their code is checked for compliance with the
> conventions. However, this does not happen for security modules
> attached to the BPF LSM, because BPF LSM directly executes the eBPF
> programs without further checks.
> 
> I was able to trigger the panic with this simple eBPF program:
> 
> SEC("lsm/inode_init_security")
> int BPF_PROG(test_int_hook, struct inode *inode,
> 	 struct inode *dir, const struct qstr *qstr, const char **name,
> 	 void **value, size_t *len)
> {
> 	return 0;
> }
> 
> In my opinion, the level of indirection is necessary to ensure that
> kernel expectations are met.

I investigated further. Instead of returning zero, I return one. This
causes a crash even with the most recent kernel (lsm=bpf):

[   27.685704] BUG: kernel NULL pointer dereference, address: 00000000000000e1
[   27.686445] #PF: supervisor read access in kernel mode
[   27.686964] #PF: error_code(0x0000) - not-present page
[   27.687465] PGD 0 P4D 0 
[   27.687724] Oops: 0000 [#1] PREEMPT SMP NOPTI
[   27.688155] CPU: 9 PID: 897 Comm: in:imjournal Not tainted 6.1.0-rc2 #255
[   27.688807] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[   27.689652] RIP: 0010:fsnotify+0x71a/0x780
[   27.690056] Code: ff 48 85 db 74 54 48 83 bb 68 04 00 00 00 74 4a 41 8b 92 98 06 00 00 4d 85 ed
0f 85 a6 f9 ff ff e9 ad f9 ff ff 48 8b 44 24 08 <4c> 8b 90 e0 00 00 00 e9 00 fa ff ff 48 c7 c2 b8 12
78 82 be 81 01
[   27.691809] RSP: 0018:ffffc90001307ca0 EFLAGS: 00010246
[   27.692313] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffff88811d73b4a8
[   27.692998] RDX: 0000000000000003 RSI: 0000000000000001 RDI: 0000000000000100
[   27.693682] RBP: ffff888100441c08 R08: 0000000000000059 R09: 0000000000000000
[   27.694371] R10: 0000000000000000 R11: ffff88846fc72d30 R12: 0000000000000100
[   27.695073] R13: ffff88811a2a5200 R14: ffffc90001307dc0 R15: 0000000000000001
[   27.695738] FS:  00007ff791000640(0000) GS:ffff88846fc40000(0000) knlGS:0000000000000000
[   27.696137] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.696430] CR2: 00000000000000e1 CR3: 0000000112aa6000 CR4: 0000000000350ee0
[   27.696782] Call Trace:
[   27.696909]  <TASK>
[   27.697026]  path_openat+0x484/0xa00
[   27.697218]  ? rcu_read_lock_held_common+0xe/0x50
[   27.697461]  do_filp_open+0x9f/0xf0
[   27.697643]  ? rcu_read_lock_sched_held+0x13/0x70
[   27.697888]  ? lock_release+0x1e1/0x2a0
[   27.698085]  ? _raw_spin_unlock+0x29/0x50
[   27.698291]  do_sys_openat2+0x226/0x300
[   27.698491]  do_sys_open+0x34/0x60
[   27.698667]  do_syscall_64+0x3b/0x90
[   27.698861]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Beeing positive, instead of negative, the return code is converted
to a legitimate pointer instead of an error pointer, causing a crash
in fsnotify().

Roberto
  

On Mon, Oct 24, 2022 at 8:28 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Mon, 2022-10-24 at 11:25 +0200, Roberto Sassu wrote:
> > On Sun, 2022-10-23 at 16:36 -0700, Alexei Starovoitov wrote:
> >
> > Sorry, forgot to CC Mimi and linux-integrity.
> >
> > > On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
> > > <roberto.sassu@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > BPF LSM allows security modules to directly attach to the security
> > > > hooks,
> > > > with the potential of not meeting the kernel expectation.
> > > >
> > > > This is the case for the inode_init_security hook, for which the
> > > > kernel
> > > > expects that name and value are set if the hook implementation
> > > > returns
> > > > zero.
> > > >
> > > > Consequently, not meeting the kernel expectation can cause the
> > > > kernel to
> > > > crash. One example is evm_protected_xattr_common() which expects
> > > > the
> > > > req_xattr_name parameter to be always not NULL.
> > >
> > > Sounds like a bug in evm_protected_xattr_common.
> >
> > If an LSM implementing the inode_init_security hook returns -EOPNOTSUPP
> > or -ENOMEM, evm_protected_xattr_common() is not going to be executed.
> >
> > This is documented in include/linux/lsm_hooks.h
> >
> > Why it would be a bug in evm_protected_xattr_common()?
> >
> > > > Introduce a level of indirection in BPF LSM, for the
> > > > inode_init_security
> > > > hook, to check the validity of the name and value set by security
> > > > modules.
> > >
> > > Doesn't make sense.
> >
> > Look at this example. The LSM infrastructure has a convention on return
> > values for the hooks (maybe there is something similar for other
> > hooks). The code calling the hooks relies on such conventions. If
> > conventions are not followed a panic occurs.
> >
> > If LSMs go to the kernel, their code is checked for compliance with the
> > conventions. However, this does not happen for security modules
> > attached to the BPF LSM, because BPF LSM directly executes the eBPF
> > programs without further checks.
> >
> > I was able to trigger the panic with this simple eBPF program:
> >
> > SEC("lsm/inode_init_security")
> > int BPF_PROG(test_int_hook, struct inode *inode,
> >        struct inode *dir, const struct qstr *qstr, const char **name,
> >        void **value, size_t *len)
> > {
> >       return 0;
> > }
> >
> > In my opinion, the level of indirection is necessary to ensure that
> > kernel expectations are met.
>
> I investigated further. Instead of returning zero, I return one. This
> causes a crash even with the most recent kernel (lsm=bpf):
>
> [   27.685704] BUG: kernel NULL pointer dereference, address: 00000000000000e1
> [   27.686445] #PF: supervisor read access in kernel mode
> [   27.686964] #PF: error_code(0x0000) - not-present page
> [   27.687465] PGD 0 P4D 0
> [   27.687724] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [   27.688155] CPU: 9 PID: 897 Comm: in:imjournal Not tainted 6.1.0-rc2 #255
> [   27.688807] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> [   27.689652] RIP: 0010:fsnotify+0x71a/0x780
> [   27.690056] Code: ff 48 85 db 74 54 48 83 bb 68 04 00 00 00 74 4a 41 8b 92 98 06 00 00 4d 85 ed
> 0f 85 a6 f9 ff ff e9 ad f9 ff ff 48 8b 44 24 08 <4c> 8b 90 e0 00 00 00 e9 00 fa ff ff 48 c7 c2 b8 12
> 78 82 be 81 01
> [   27.691809] RSP: 0018:ffffc90001307ca0 EFLAGS: 00010246
> [   27.692313] RAX: 0000000000000001 RBX: 0000000000000000 RCX: ffff88811d73b4a8
> [   27.692998] RDX: 0000000000000003 RSI: 0000000000000001 RDI: 0000000000000100
> [   27.693682] RBP: ffff888100441c08 R08: 0000000000000059 R09: 0000000000000000
> [   27.694371] R10: 0000000000000000 R11: ffff88846fc72d30 R12: 0000000000000100
> [   27.695073] R13: ffff88811a2a5200 R14: ffffc90001307dc0 R15: 0000000000000001
> [   27.695738] FS:  00007ff791000640(0000) GS:ffff88846fc40000(0000) knlGS:0000000000000000
> [   27.696137] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   27.696430] CR2: 00000000000000e1 CR3: 0000000112aa6000 CR4: 0000000000350ee0
> [   27.696782] Call Trace:
> [   27.696909]  <TASK>
> [   27.697026]  path_openat+0x484/0xa00
> [   27.697218]  ? rcu_read_lock_held_common+0xe/0x50
> [   27.697461]  do_filp_open+0x9f/0xf0
> [   27.697643]  ? rcu_read_lock_sched_held+0x13/0x70
> [   27.697888]  ? lock_release+0x1e1/0x2a0
> [   27.698085]  ? _raw_spin_unlock+0x29/0x50
> [   27.698291]  do_sys_openat2+0x226/0x300
> [   27.698491]  do_sys_open+0x34/0x60
> [   27.698667]  do_syscall_64+0x3b/0x90
> [   27.698861]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Beeing positive, instead of negative, the return code is converted
> to a legitimate pointer instead of an error pointer, causing a crash
> in fsnotify().

Could you point to the code that does that?

I'm looking at security_inode_init_security() and it is indeed messy.
Per file system initxattrs callback that processes kmalloc-ed strings.
Yikes.

In the short term we should denylist inode_init_security hook to
disallow attaching bpf-lsm there. set/getxattr should be done
through kfuncs instead of such kmalloc-a-string hack.
  

On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
> On Mon, Oct 24, 2022 at 8:28 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Mon, 2022-10-24 at 11:25 +0200, Roberto Sassu wrote:
> > > On Sun, 2022-10-23 at 16:36 -0700, Alexei Starovoitov wrote:
> > > 
> > > Sorry, forgot to CC Mimi and linux-integrity.
> > > 
> > > > On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
> > > > <roberto.sassu@huaweicloud.com> wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > BPF LSM allows security modules to directly attach to the
> > > > > security
> > > > > hooks,
> > > > > with the potential of not meeting the kernel expectation.
> > > > > 
> > > > > This is the case for the inode_init_security hook, for which
> > > > > the
> > > > > kernel
> > > > > expects that name and value are set if the hook
> > > > > implementation
> > > > > returns
> > > > > zero.
> > > > > 
> > > > > Consequently, not meeting the kernel expectation can cause
> > > > > the
> > > > > kernel to
> > > > > crash. One example is evm_protected_xattr_common() which
> > > > > expects
> > > > > the
> > > > > req_xattr_name parameter to be always not NULL.
> > > > 
> > > > Sounds like a bug in evm_protected_xattr_common.
> > > 
> > > If an LSM implementing the inode_init_security hook returns
> > > -EOPNOTSUPP
> > > or -ENOMEM, evm_protected_xattr_common() is not going to be
> > > executed.
> > > 
> > > This is documented in include/linux/lsm_hooks.h
> > > 
> > > Why it would be a bug in evm_protected_xattr_common()?
> > > 
> > > > > Introduce a level of indirection in BPF LSM, for the
> > > > > inode_init_security
> > > > > hook, to check the validity of the name and value set by
> > > > > security
> > > > > modules.
> > > > 
> > > > Doesn't make sense.
> > > 
> > > Look at this example. The LSM infrastructure has a convention on
> > > return
> > > values for the hooks (maybe there is something similar for other
> > > hooks). The code calling the hooks relies on such conventions. If
> > > conventions are not followed a panic occurs.
> > > 
> > > If LSMs go to the kernel, their code is checked for compliance
> > > with the
> > > conventions. However, this does not happen for security modules
> > > attached to the BPF LSM, because BPF LSM directly executes the
> > > eBPF
> > > programs without further checks.
> > > 
> > > I was able to trigger the panic with this simple eBPF program:
> > > 
> > > SEC("lsm/inode_init_security")
> > > int BPF_PROG(test_int_hook, struct inode *inode,
> > >        struct inode *dir, const struct qstr *qstr, const char
> > > **name,
> > >        void **value, size_t *len)
> > > {
> > >       return 0;
> > > }
> > > 
> > > In my opinion, the level of indirection is necessary to ensure
> > > that
> > > kernel expectations are met.
> > 
> > I investigated further. Instead of returning zero, I return one.
> > This
> > causes a crash even with the most recent kernel (lsm=bpf):
> > 
> > [   27.685704] BUG: kernel NULL pointer dereference, address:
> > 00000000000000e1
> > [   27.686445] #PF: supervisor read access in kernel mode
> > [   27.686964] #PF: error_code(0x0000) - not-present page
> > [   27.687465] PGD 0 P4D 0
> > [   27.687724] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [   27.688155] CPU: 9 PID: 897 Comm: in:imjournal Not tainted
> > 6.1.0-rc2 #255
> > [   27.688807] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> > BIOS 1.13.0-1ubuntu1.1 04/01/2014
> > [   27.689652] RIP: 0010:fsnotify+0x71a/0x780
> > [   27.690056] Code: ff 48 85 db 74 54 48 83 bb 68 04 00 00 00 74
> > 4a 41 8b 92 98 06 00 00 4d 85 ed
> > 0f 85 a6 f9 ff ff e9 ad f9 ff ff 48 8b 44 24 08 <4c> 8b 90 e0 00 00
> > 00 e9 00 fa ff ff 48 c7 c2 b8 12
> > 78 82 be 81 01
> > [   27.691809] RSP: 0018:ffffc90001307ca0 EFLAGS: 00010246
> > [   27.692313] RAX: 0000000000000001 RBX: 0000000000000000 RCX:
> > ffff88811d73b4a8
> > [   27.692998] RDX: 0000000000000003 RSI: 0000000000000001 RDI:
> > 0000000000000100
> > [   27.693682] RBP: ffff888100441c08 R08: 0000000000000059 R09:
> > 0000000000000000
> > [   27.694371] R10: 0000000000000000 R11: ffff88846fc72d30 R12:
> > 0000000000000100
> > [   27.695073] R13: ffff88811a2a5200 R14: ffffc90001307dc0 R15:
> > 0000000000000001
> > [   27.695738] FS:  00007ff791000640(0000)
> > GS:ffff88846fc40000(0000) knlGS:0000000000000000
> > [   27.696137] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   27.696430] CR2: 00000000000000e1 CR3: 0000000112aa6000 CR4:
> > 0000000000350ee0
> > [   27.696782] Call Trace:
> > [   27.696909]  <TASK>
> > [   27.697026]  path_openat+0x484/0xa00
> > [   27.697218]  ? rcu_read_lock_held_common+0xe/0x50
> > [   27.697461]  do_filp_open+0x9f/0xf0
> > [   27.697643]  ? rcu_read_lock_sched_held+0x13/0x70
> > [   27.697888]  ? lock_release+0x1e1/0x2a0
> > [   27.698085]  ? _raw_spin_unlock+0x29/0x50
> > [   27.698291]  do_sys_openat2+0x226/0x300
> > [   27.698491]  do_sys_open+0x34/0x60
> > [   27.698667]  do_syscall_64+0x3b/0x90
> > [   27.698861]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > 
> > Beeing positive, instead of negative, the return code is converted
> > to a legitimate pointer instead of an error pointer, causing a
> > crash
> > in fsnotify().
> 
> Could you point to the code that does that?

It happens when a new file is created:

#0  xfs_generic_create at fs/xfs/xfs_iops.c:253
#1  0xffffffff813f4508 in lookup_open at fs/namei.c:3413
#2  0xffffffff813f9b61 in open_last_lookups at fs/namei.c:3481

In open_last_lookups(), we have:

	if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED))
		fsnotify_create(dir->d_inode, dentry);

But dentry is equal to 1:

(gdb) p dentry
$7 = (struct dentry *) 0x1 <fixed_percpu_data+1>

Continuing to debug, we encounter:

fsnotify_data_sb (data_type=3, data=0x1 <fixed_percpu_data+1>) at
./include/linux/fsnotify_backend.h:347
347			return ((struct dentry *)data)->d_sb;

which is an invalid access.

> I'm looking at security_inode_init_security() and it is indeed messy.
> Per file system initxattrs callback that processes kmalloc-ed
> strings.
> Yikes.
> 
> In the short term we should denylist inode_init_security hook to
> disallow attaching bpf-lsm there. set/getxattr should be done
> through kfuncs instead of such kmalloc-a-string hack.

Inode_init_security is an example. It could be that the other hooks are
affected too. What happens if they get arbitrary positive values too?

Thanks

Roberto
  

On 10/25/2022 12:43 AM, Roberto Sassu wrote:
> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
>> I'm looking at security_inode_init_security() and it is indeed messy.
>> Per file system initxattrs callback that processes kmalloc-ed
>> strings.
>> Yikes.
>>
>> In the short term we should denylist inode_init_security hook to
>> disallow attaching bpf-lsm there. set/getxattr should be done
>> through kfuncs instead of such kmalloc-a-string hack.
> Inode_init_security is an example. It could be that the other hooks are
> affected too. What happens if they get arbitrary positive values too?

TL;DR - Things will go cattywampus.

The LSM infrastructure is an interface that has "grown organically",
and isn't necessarily consistent in what it requires of the security
module implementations. There are cases where it assumes that the
security module hooks are well behaved, as you've discovered. I have
no small amount of fear that someone is going to provide an eBPF
program for security_secid_to_secctx(). There has been an assumption,
oft stated, that all security modules are going to be reviewed as
part of the upstream process. The review process ought to catch hooks
that return unacceptable values. Alas, we've lost that with BPF.

It would take a(nother) major overhaul of the LSM infrastructure to
make it safe against hooks that are not well behaved. From what I have
seen so far it wouldn't be easy/convenient/performant to do it in the
BPF security module either. I personally think that BPF needs to
ensure that the eBPF implementations don't return inappropriate values,
but I understand why that is problematic.

> Thanks
>
> Roberto
>
  

On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
> > On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
> >> I'm looking at security_inode_init_security() and it is indeed messy.
> >> Per file system initxattrs callback that processes kmalloc-ed
> >> strings.
> >> Yikes.
> >>
> >> In the short term we should denylist inode_init_security hook to
> >> disallow attaching bpf-lsm there. set/getxattr should be done
> >> through kfuncs instead of such kmalloc-a-string hack.
> > Inode_init_security is an example. It could be that the other hooks are
> > affected too. What happens if they get arbitrary positive values too?
>
> TL;DR - Things will go cattywampus.
>
> The LSM infrastructure is an interface that has "grown organically",
> and isn't necessarily consistent in what it requires of the security
> module implementations. There are cases where it assumes that the
> security module hooks are well behaved, as you've discovered. I have
> no small amount of fear that someone is going to provide an eBPF
> program for security_secid_to_secctx(). There has been an assumption,
> oft stated, that all security modules are going to be reviewed as
> part of the upstream process. The review process ought to catch hooks
> that return unacceptable values. Alas, we've lost that with BPF.
>
> It would take a(nother) major overhaul of the LSM infrastructure to
> make it safe against hooks that are not well behaved. From what I have
> seen so far it wouldn't be easy/convenient/performant to do it in the
> BPF security module either. I personally think that BPF needs to
> ensure that the eBPF implementations don't return inappropriate values,
> but I understand why that is problematic.

That's an accurate statement. Thank you.

Going back to the original question...
We fix bugs when we discover them.
Regardless of the subsystem they belong to.
No finger pointing.
  

On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
>>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
>>>> I'm looking at security_inode_init_security() and it is indeed messy.
>>>> Per file system initxattrs callback that processes kmalloc-ed
>>>> strings.
>>>> Yikes.
>>>>
>>>> In the short term we should denylist inode_init_security hook to
>>>> disallow attaching bpf-lsm there. set/getxattr should be done
>>>> through kfuncs instead of such kmalloc-a-string hack.
>>> Inode_init_security is an example. It could be that the other hooks are
>>> affected too. What happens if they get arbitrary positive values too?
>>
>> TL;DR - Things will go cattywampus.
>>
>> The LSM infrastructure is an interface that has "grown organically",
>> and isn't necessarily consistent in what it requires of the security
>> module implementations. There are cases where it assumes that the
>> security module hooks are well behaved, as you've discovered. I have
>> no small amount of fear that someone is going to provide an eBPF
>> program for security_secid_to_secctx(). There has been an assumption,
>> oft stated, that all security modules are going to be reviewed as
>> part of the upstream process. The review process ought to catch hooks
>> that return unacceptable values. Alas, we've lost that with BPF.
>>
>> It would take a(nother) major overhaul of the LSM infrastructure to
>> make it safe against hooks that are not well behaved. From what I have
>> seen so far it wouldn't be easy/convenient/performant to do it in the
>> BPF security module either. I personally think that BPF needs to
>> ensure that the eBPF implementations don't return inappropriate values,
>> but I understand why that is problematic.
> 
> That's an accurate statement. Thank you.
> 
> Going back to the original question...
> We fix bugs when we discover them.
> Regardless of the subsystem they belong to.
> No finger pointing.

I'm concerned about the following situation:

struct <something> *function()
{

	ret = security_*();
	if (ret)
		return ERR_PTR(ret);

}

int caller()
{
	ptr = function()
	if (IS_ERR(ptr)
		goto out;

	<use of invalid pointer>
}

I quickly found an occurrence of this:

static int lookup_one_common()
{

[...]

	return inode_permission();
}

struct dentry *try_lookup_one_len()
{

[...]

         err = lookup_one_common(&init_user_ns, name, base, len, &this);
         if (err)
                 return ERR_PTR(err);


Unfortunately, attaching to inode_permission causes the kernel
to crash immediately (it does not happen with negative return
values).

So, I think the fix should be broader, and not limited to the
inode_init_security hook. Will try to see how it can be fixed.

Roberto
  

On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> > On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>
> >> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
> >>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
> >>>> I'm looking at security_inode_init_security() and it is indeed messy.
> >>>> Per file system initxattrs callback that processes kmalloc-ed
> >>>> strings.
> >>>> Yikes.
> >>>>
> >>>> In the short term we should denylist inode_init_security hook to
> >>>> disallow attaching bpf-lsm there. set/getxattr should be done
> >>>> through kfuncs instead of such kmalloc-a-string hack.
> >>> Inode_init_security is an example. It could be that the other hooks are
> >>> affected too. What happens if they get arbitrary positive values too?
> >>
> >> TL;DR - Things will go cattywampus.
> >>
> >> The LSM infrastructure is an interface that has "grown organically",
> >> and isn't necessarily consistent in what it requires of the security
> >> module implementations. There are cases where it assumes that the
> >> security module hooks are well behaved, as you've discovered. I have
> >> no small amount of fear that someone is going to provide an eBPF
> >> program for security_secid_to_secctx(). There has been an assumption,
> >> oft stated, that all security modules are going to be reviewed as
> >> part of the upstream process. The review process ought to catch hooks
> >> that return unacceptable values. Alas, we've lost that with BPF.
> >>
> >> It would take a(nother) major overhaul of the LSM infrastructure to
> >> make it safe against hooks that are not well behaved. From what I have
> >> seen so far it wouldn't be easy/convenient/performant to do it in the
> >> BPF security module either. I personally think that BPF needs to
> >> ensure that the eBPF implementations don't return inappropriate values,
> >> but I understand why that is problematic.
> >
> > That's an accurate statement. Thank you.
> >
> > Going back to the original question...
> > We fix bugs when we discover them.
> > Regardless of the subsystem they belong to.
> > No finger pointing.
>
> I'm concerned about the following situation:
>
> struct <something> *function()
> {
>
>         ret = security_*();
>         if (ret)
>                 return ERR_PTR(ret);
>
> }
>
> int caller()
> {
>         ptr = function()
>         if (IS_ERR(ptr)
>                 goto out;
>
>         <use of invalid pointer>
> }
>
> I quickly found an occurrence of this:
>
> static int lookup_one_common()
> {
>
> [...]
>
>         return inode_permission();
> }
>
> struct dentry *try_lookup_one_len()
> {
>
> [...]
>
>          err = lookup_one_common(&init_user_ns, name, base, len, &this);
>          if (err)
>                  return ERR_PTR(err);
>
>
> Unfortunately, attaching to inode_permission causes the kernel
> to crash immediately (it does not happen with negative return
> values).
>
> So, I think the fix should be broader, and not limited to the
> inode_init_security hook. Will try to see how it can be fixed.

I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
Trivial verifier change.
  

On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> >
> > On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> > > On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >>
> > >> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
> > >>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
> > >>>> I'm looking at security_inode_init_security() and it is indeed messy.
> > >>>> Per file system initxattrs callback that processes kmalloc-ed
> > >>>> strings.
> > >>>> Yikes.
> > >>>>
> > >>>> In the short term we should denylist inode_init_security hook to
> > >>>> disallow attaching bpf-lsm there. set/getxattr should be done
> > >>>> through kfuncs instead of such kmalloc-a-string hack.
> > >>> Inode_init_security is an example. It could be that the other hooks are
> > >>> affected too. What happens if they get arbitrary positive values too?
> > >>
> > >> TL;DR - Things will go cattywampus.
> > >>
> > >> The LSM infrastructure is an interface that has "grown organically",
> > >> and isn't necessarily consistent in what it requires of the security
> > >> module implementations. There are cases where it assumes that the
> > >> security module hooks are well behaved, as you've discovered. I have
> > >> no small amount of fear that someone is going to provide an eBPF
> > >> program for security_secid_to_secctx(). There has been an assumption,
> > >> oft stated, that all security modules are going to be reviewed as
> > >> part of the upstream process. The review process ought to catch hooks
> > >> that return unacceptable values. Alas, we've lost that with BPF.
> > >>
> > >> It would take a(nother) major overhaul of the LSM infrastructure to
> > >> make it safe against hooks that are not well behaved. From what I have
> > >> seen so far it wouldn't be easy/convenient/performant to do it in the
> > >> BPF security module either. I personally think that BPF needs to
> > >> ensure that the eBPF implementations don't return inappropriate values,
> > >> but I understand why that is problematic.
> > >
> > > That's an accurate statement. Thank you.
> > >
> > > Going back to the original question...
> > > We fix bugs when we discover them.
> > > Regardless of the subsystem they belong to.
> > > No finger pointing.
> >
> > I'm concerned about the following situation:
> >
> > struct <something> *function()
> > {
> >
> >         ret = security_*();
> >         if (ret)
> >                 return ERR_PTR(ret);
> >
> > }
> >
> > int caller()
> > {
> >         ptr = function()
> >         if (IS_ERR(ptr)
> >                 goto out;
> >
> >         <use of invalid pointer>
> > }
> >
> > I quickly found an occurrence of this:
> >
> > static int lookup_one_common()
> > {
> >
> > [...]
> >
> >         return inode_permission();
> > }
> >
> > struct dentry *try_lookup_one_len()
> > {
> >
> > [...]
> >
> >          err = lookup_one_common(&init_user_ns, name, base, len, &this);
> >          if (err)
> >                  return ERR_PTR(err);
> >
> >
> > Unfortunately, attaching to inode_permission causes the kernel
> > to crash immediately (it does not happen with negative return
> > values).
> >
> > So, I think the fix should be broader, and not limited to the
> > inode_init_security hook. Will try to see how it can be fixed.
>
> I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
> Trivial verifier change.

Thanks, yes this indeed is an issue. We need to do a few things:

1. Restrict some hooks that we know the BPF LSM will never need.
2. A verifier function that checks return values of LSM
hooks.
For most LSK hooks IS_ERR_VALUE is fine, however, there are some hooks
like *xattr hooks that use a return value of 1 to indicate a
capability check is required which might need special handling.
  

On 10/27/2022 3:39 AM, KP Singh wrote:
> On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
>> <roberto.sassu@huaweicloud.com> wrote:
>>> On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
>>>> On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
>>>>>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
>>>>>>> I'm looking at security_inode_init_security() and it is indeed messy.
>>>>>>> Per file system initxattrs callback that processes kmalloc-ed
>>>>>>> strings.
>>>>>>> Yikes.
>>>>>>>
>>>>>>> In the short term we should denylist inode_init_security hook to
>>>>>>> disallow attaching bpf-lsm there. set/getxattr should be done
>>>>>>> through kfuncs instead of such kmalloc-a-string hack.
>>>>>> Inode_init_security is an example. It could be that the other hooks are
>>>>>> affected too. What happens if they get arbitrary positive values too?
>>>>> TL;DR - Things will go cattywampus.
>>>>>
>>>>> The LSM infrastructure is an interface that has "grown organically",
>>>>> and isn't necessarily consistent in what it requires of the security
>>>>> module implementations. There are cases where it assumes that the
>>>>> security module hooks are well behaved, as you've discovered. I have
>>>>> no small amount of fear that someone is going to provide an eBPF
>>>>> program for security_secid_to_secctx(). There has been an assumption,
>>>>> oft stated, that all security modules are going to be reviewed as
>>>>> part of the upstream process. The review process ought to catch hooks
>>>>> that return unacceptable values. Alas, we've lost that with BPF.
>>>>>
>>>>> It would take a(nother) major overhaul of the LSM infrastructure to
>>>>> make it safe against hooks that are not well behaved. From what I have
>>>>> seen so far it wouldn't be easy/convenient/performant to do it in the
>>>>> BPF security module either. I personally think that BPF needs to
>>>>> ensure that the eBPF implementations don't return inappropriate values,
>>>>> but I understand why that is problematic.
>>>> That's an accurate statement. Thank you.
>>>>
>>>> Going back to the original question...
>>>> We fix bugs when we discover them.
>>>> Regardless of the subsystem they belong to.
>>>> No finger pointing.
>>> I'm concerned about the following situation:
>>>
>>> struct <something> *function()
>>> {
>>>
>>>         ret = security_*();
>>>         if (ret)
>>>                 return ERR_PTR(ret);
>>>
>>> }
>>>
>>> int caller()
>>> {
>>>         ptr = function()
>>>         if (IS_ERR(ptr)
>>>                 goto out;
>>>
>>>         <use of invalid pointer>
>>> }
>>>
>>> I quickly found an occurrence of this:
>>>
>>> static int lookup_one_common()
>>> {
>>>
>>> [...]
>>>
>>>         return inode_permission();
>>> }
>>>
>>> struct dentry *try_lookup_one_len()
>>> {
>>>
>>> [...]
>>>
>>>          err = lookup_one_common(&init_user_ns, name, base, len, &this);
>>>          if (err)
>>>                  return ERR_PTR(err);
>>>
>>>
>>> Unfortunately, attaching to inode_permission causes the kernel
>>> to crash immediately (it does not happen with negative return
>>> values).
>>>
>>> So, I think the fix should be broader, and not limited to the
>>> inode_init_security hook. Will try to see how it can be fixed.
>> I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
>> Trivial verifier change.
> Thanks, yes this indeed is an issue. We need to do a few things:
>
> 1. Restrict some hooks that we know the BPF LSM will never need.

It might be difficult to identify which hooks will never be useful
in a general purpose programming system like BPF. I do suggest that,
if at all possible, you restrict any hook that uses or provides a
secid. That will take out the bulk of the "dangerous" hooks.

> 2. A verifier function that checks return values of LSM
> hooks.

That would be grand.

> For most LSK hooks IS_ERR_VALUE is fine, however, there are some hooks
> like *xattr hooks that use a return value of 1 to indicate a
> capability check is required which might need special handling.

The exceptions are pretty rare, and I don't see a reason why
we couldn't "normalize", or at least more clearly document the
outliers.
  

On Thu, 2022-10-27 at 12:39 +0200, KP Singh wrote:
> On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> > > > On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <
> > > > casey@schaufler-ca.com> wrote:
> > > > > On 10/25/2022 12:43 AM, Roberto Sassu wrote:
> > > > > > On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov
> > > > > > wrote:
> > > > > > > I'm looking at security_inode_init_security() and it is
> > > > > > > indeed messy.
> > > > > > > Per file system initxattrs callback that processes
> > > > > > > kmalloc-ed
> > > > > > > strings.
> > > > > > > Yikes.
> > > > > > > 
> > > > > > > In the short term we should denylist inode_init_security
> > > > > > > hook to
> > > > > > > disallow attaching bpf-lsm there. set/getxattr should be
> > > > > > > done
> > > > > > > through kfuncs instead of such kmalloc-a-string hack.
> > > > > > Inode_init_security is an example. It could be that the
> > > > > > other hooks are
> > > > > > affected too. What happens if they get arbitrary positive
> > > > > > values too?
> > > > > 
> > > > > TL;DR - Things will go cattywampus.
> > > > > 
> > > > > The LSM infrastructure is an interface that has "grown
> > > > > organically",
> > > > > and isn't necessarily consistent in what it requires of the
> > > > > security
> > > > > module implementations. There are cases where it assumes that
> > > > > the
> > > > > security module hooks are well behaved, as you've discovered.
> > > > > I have
> > > > > no small amount of fear that someone is going to provide an
> > > > > eBPF
> > > > > program for security_secid_to_secctx(). There has been an
> > > > > assumption,
> > > > > oft stated, that all security modules are going to be
> > > > > reviewed as
> > > > > part of the upstream process. The review process ought to
> > > > > catch hooks
> > > > > that return unacceptable values. Alas, we've lost that with
> > > > > BPF.
> > > > > 
> > > > > It would take a(nother) major overhaul of the LSM
> > > > > infrastructure to
> > > > > make it safe against hooks that are not well behaved. From
> > > > > what I have
> > > > > seen so far it wouldn't be easy/convenient/performant to do
> > > > > it in the
> > > > > BPF security module either. I personally think that BPF needs
> > > > > to
> > > > > ensure that the eBPF implementations don't return
> > > > > inappropriate values,
> > > > > but I understand why that is problematic.
> > > > 
> > > > That's an accurate statement. Thank you.
> > > > 
> > > > Going back to the original question...
> > > > We fix bugs when we discover them.
> > > > Regardless of the subsystem they belong to.
> > > > No finger pointing.
> > > 
> > > I'm concerned about the following situation:
> > > 
> > > struct <something> *function()
> > > {
> > > 
> > >         ret = security_*();
> > >         if (ret)
> > >                 return ERR_PTR(ret);
> > > 
> > > }
> > > 
> > > int caller()
> > > {
> > >         ptr = function()
> > >         if (IS_ERR(ptr)
> > >                 goto out;
> > > 
> > >         <use of invalid pointer>
> > > }
> > > 
> > > I quickly found an occurrence of this:
> > > 
> > > static int lookup_one_common()
> > > {
> > > 
> > > [...]
> > > 
> > >         return inode_permission();
> > > }
> > > 
> > > struct dentry *try_lookup_one_len()
> > > {
> > > 
> > > [...]
> > > 
> > >          err = lookup_one_common(&init_user_ns, name, base, len,
> > > &this);
> > >          if (err)
> > >                  return ERR_PTR(err);
> > > 
> > > 
> > > Unfortunately, attaching to inode_permission causes the kernel
> > > to crash immediately (it does not happen with negative return
> > > values).
> > > 
> > > So, I think the fix should be broader, and not limited to the
> > > inode_init_security hook. Will try to see how it can be fixed.
> > 
> > I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
> > Trivial verifier change.
> 
> Thanks, yes this indeed is an issue. We need to do a few things:
> 
> 1. Restrict some hooks that we know the BPF LSM will never need.
> 2. A verifier function that checks return values of LSM
> hooks.
> For most LSK hooks IS_ERR_VALUE is fine, however, there are some
> hooks
> like *xattr hooks that use a return value of 1 to indicate a
> capability check is required which might need special handling.

I looked at security.c:

/*
 * SELinux and Smack integrate the cap call,
 * so assume that all LSMs supplying this call do so.
 */

Other than checking the return value, probably we should also wrap
bpf_lsm_inode_{set,remove}xattr() to do the capability check, right?

Roberto
  

On 10/28/2022 1:48 AM, Roberto Sassu wrote:
> On Thu, 2022-10-27 at 12:39 +0200, KP Singh wrote:
>> On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
>>> <roberto.sassu@huaweicloud.com> wrote:
>>>> On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
>>>>> On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <
>>>>> casey@schaufler-ca.com> wrote:
>>>>>> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
>>>>>>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov
>>>>>>> wrote:
>>>>>>>> I'm looking at security_inode_init_security() and it is
>>>>>>>> indeed messy.
>>>>>>>> Per file system initxattrs callback that processes
>>>>>>>> kmalloc-ed
>>>>>>>> strings.
>>>>>>>> Yikes.
>>>>>>>>
>>>>>>>> In the short term we should denylist inode_init_security
>>>>>>>> hook to
>>>>>>>> disallow attaching bpf-lsm there. set/getxattr should be
>>>>>>>> done
>>>>>>>> through kfuncs instead of such kmalloc-a-string hack.
>>>>>>> Inode_init_security is an example. It could be that the
>>>>>>> other hooks are
>>>>>>> affected too. What happens if they get arbitrary positive
>>>>>>> values too?
>>>>>> TL;DR - Things will go cattywampus.
>>>>>>
>>>>>> The LSM infrastructure is an interface that has "grown
>>>>>> organically",
>>>>>> and isn't necessarily consistent in what it requires of the
>>>>>> security
>>>>>> module implementations. There are cases where it assumes that
>>>>>> the
>>>>>> security module hooks are well behaved, as you've discovered.
>>>>>> I have
>>>>>> no small amount of fear that someone is going to provide an
>>>>>> eBPF
>>>>>> program for security_secid_to_secctx(). There has been an
>>>>>> assumption,
>>>>>> oft stated, that all security modules are going to be
>>>>>> reviewed as
>>>>>> part of the upstream process. The review process ought to
>>>>>> catch hooks
>>>>>> that return unacceptable values. Alas, we've lost that with
>>>>>> BPF.
>>>>>>
>>>>>> It would take a(nother) major overhaul of the LSM
>>>>>> infrastructure to
>>>>>> make it safe against hooks that are not well behaved. From
>>>>>> what I have
>>>>>> seen so far it wouldn't be easy/convenient/performant to do
>>>>>> it in the
>>>>>> BPF security module either. I personally think that BPF needs
>>>>>> to
>>>>>> ensure that the eBPF implementations don't return
>>>>>> inappropriate values,
>>>>>> but I understand why that is problematic.
>>>>> That's an accurate statement. Thank you.
>>>>>
>>>>> Going back to the original question...
>>>>> We fix bugs when we discover them.
>>>>> Regardless of the subsystem they belong to.
>>>>> No finger pointing.
>>>> I'm concerned about the following situation:
>>>>
>>>> struct <something> *function()
>>>> {
>>>>
>>>>         ret = security_*();
>>>>         if (ret)
>>>>                 return ERR_PTR(ret);
>>>>
>>>> }
>>>>
>>>> int caller()
>>>> {
>>>>         ptr = function()
>>>>         if (IS_ERR(ptr)
>>>>                 goto out;
>>>>
>>>>         <use of invalid pointer>
>>>> }
>>>>
>>>> I quickly found an occurrence of this:
>>>>
>>>> static int lookup_one_common()
>>>> {
>>>>
>>>> [...]
>>>>
>>>>         return inode_permission();
>>>> }
>>>>
>>>> struct dentry *try_lookup_one_len()
>>>> {
>>>>
>>>> [...]
>>>>
>>>>          err = lookup_one_common(&init_user_ns, name, base, len,
>>>> &this);
>>>>          if (err)
>>>>                  return ERR_PTR(err);
>>>>
>>>>
>>>> Unfortunately, attaching to inode_permission causes the kernel
>>>> to crash immediately (it does not happen with negative return
>>>> values).
>>>>
>>>> So, I think the fix should be broader, and not limited to the
>>>> inode_init_security hook. Will try to see how it can be fixed.
>>> I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
>>> Trivial verifier change.
>> Thanks, yes this indeed is an issue. We need to do a few things:
>>
>> 1. Restrict some hooks that we know the BPF LSM will never need.
>> 2. A verifier function that checks return values of LSM
>> hooks.
>> For most LSK hooks IS_ERR_VALUE is fine, however, there are some
>> hooks
>> like *xattr hooks that use a return value of 1 to indicate a
>> capability check is required which might need special handling.
> I looked at security.c:
>
> /*
>  * SELinux and Smack integrate the cap call,
>  * so assume that all LSMs supplying this call do so.
>  */
>
> Other than checking the return value, probably we should also wrap
> bpf_lsm_inode_{set,remove}xattr() to do the capability check, right?

Long term I hope to fix the way capabilities are dealt with in
this hook, but for now your suggestion seems reasonable. 

>
> Roberto
>