[bpf-next,v5,4/7] bpf: lsm: Implement attach, detach and execution

Commit Message

KP Singh March 23, 2020, 4:44 p.m. UTC

From: KP Singh <[e-mail]>

JITed BPF programs are dynamically attached to the LSM hooks
using BPF trampolines. The trampoline prologue generates code to handle
conversion of the signature of the hook to the appropriate BPF context.

The allocated trampoline programs are attached to the nop functions
initialized as LSM hooks.

BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
and need CAP_SYS_ADMIN (required for loading eBPF programs).

Upon attachment:

* A BPF fexit trampoline is used for LSM hooks with a void return type.
* A BPF fmod_ret trampoline is used for LSM hooks which return an
  int. The attached programs can override the return value of the
  bpf LSM hook to indicate a MAC Policy decision.

Signed-off-by: KP Singh <[e-mail]>
Reviewed-by: Brendan Jackman <[e-mail]>
Reviewed-by: Florent Revest <[e-mail]>
---
 include/linux/bpf.h     |  4 ++++
 include/linux/bpf_lsm.h | 11 +++++++++++
 kernel/bpf/bpf_lsm.c    | 29 +++++++++++++++++++++++++++++
 kernel/bpf/btf.c        |  9 ++++++++-
 kernel/bpf/syscall.c    | 26 ++++++++++++++++++++++----
 kernel/bpf/trampoline.c | 17 +++++++++++++----
 kernel/bpf/verifier.c   | 19 +++++++++++++++----
 7 files changed, 102 insertions(+), 13 deletions(-)

Comments

Yonghong Song March 23, 2020, 7:16 p.m. UTC | #1

On 3/23/20 9:44 AM, KP Singh wrote:
> From: KP Singh <[e-mail]>
> 
> JITed BPF programs are dynamically attached to the LSM hooks
> using BPF trampolines. The trampoline prologue generates code to handle
> conversion of the signature of the hook to the appropriate BPF context.
> 
> The allocated trampoline programs are attached to the nop functions
> initialized as LSM hooks.
> 
> BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
> and need CAP_SYS_ADMIN (required for loading eBPF programs).
> 
> Upon attachment:
> 
> * A BPF fexit trampoline is used for LSM hooks with a void return type.
> * A BPF fmod_ret trampoline is used for LSM hooks which return an
>    int. The attached programs can override the return value of the
>    bpf LSM hook to indicate a MAC Policy decision.
> 
> Signed-off-by: KP Singh <[e-mail]>
> Reviewed-by: Brendan Jackman <[e-mail]>
> Reviewed-by: Florent Revest <[e-mail]>
> ---
>   include/linux/bpf.h     |  4 ++++
>   include/linux/bpf_lsm.h | 11 +++++++++++
>   kernel/bpf/bpf_lsm.c    | 29 +++++++++++++++++++++++++++++
>   kernel/bpf/btf.c        |  9 ++++++++-
>   kernel/bpf/syscall.c    | 26 ++++++++++++++++++++++----
>   kernel/bpf/trampoline.c | 17 +++++++++++++----
>   kernel/bpf/verifier.c   | 19 +++++++++++++++----
>   7 files changed, 102 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index af81ec7b783c..adf2e5a6de4b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -433,6 +433,10 @@ struct btf_func_model {
>    * programs only. Should not be used with normal calls and indirect calls.
>    */
>   #define BPF_TRAMP_F_SKIP_FRAME		BIT(2)
> +/* Override the return value of the original function. This flag only makes
> + * sense for fexit trampolines.
> + */
> +#define BPF_TRAMP_F_OVERRIDE_RETURN     BIT(3)

Whether the return value is overridable is determined by hook return 
type as below. Do we still need this flag?

>   
>   /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
>    * bytes on x86.  Pick a number to fit into BPF_IMAGE_SIZE / 2
[...]

KP Singh March 23, 2020, 7:44 p.m. UTC | #2

On 23-Mär 12:16, Yonghong Song wrote:
> 
> 
> On 3/23/20 9:44 AM, KP Singh wrote:
> > From: KP Singh <[e-mail]>
> > 
> > JITed BPF programs are dynamically attached to the LSM hooks
> > using BPF trampolines. The trampoline prologue generates code to handle
> > conversion of the signature of the hook to the appropriate BPF context.
> > 
> > The allocated trampoline programs are attached to the nop functions
> > initialized as LSM hooks.
> > 
> > BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
> > and need CAP_SYS_ADMIN (required for loading eBPF programs).
> > 
> > Upon attachment:
> > 
> > * A BPF fexit trampoline is used for LSM hooks with a void return type.
> > * A BPF fmod_ret trampoline is used for LSM hooks which return an
> >    int. The attached programs can override the return value of the
> >    bpf LSM hook to indicate a MAC Policy decision.
> > 
> > Signed-off-by: KP Singh <[e-mail]>
> > Reviewed-by: Brendan Jackman <[e-mail]>
> > Reviewed-by: Florent Revest <[e-mail]>
> > ---
> >   include/linux/bpf.h     |  4 ++++
> >   include/linux/bpf_lsm.h | 11 +++++++++++
> >   kernel/bpf/bpf_lsm.c    | 29 +++++++++++++++++++++++++++++
> >   kernel/bpf/btf.c        |  9 ++++++++-
> >   kernel/bpf/syscall.c    | 26 ++++++++++++++++++++++----
> >   kernel/bpf/trampoline.c | 17 +++++++++++++----
> >   kernel/bpf/verifier.c   | 19 +++++++++++++++----
> >   7 files changed, 102 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index af81ec7b783c..adf2e5a6de4b 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -433,6 +433,10 @@ struct btf_func_model {
> >    * programs only. Should not be used with normal calls and indirect calls.
> >    */
> >   #define BPF_TRAMP_F_SKIP_FRAME		BIT(2)
> > +/* Override the return value of the original function. This flag only makes
> > + * sense for fexit trampolines.
> > + */
> > +#define BPF_TRAMP_F_OVERRIDE_RETURN     BIT(3)
> 
> Whether the return value is overridable is determined by hook return type as
> below. Do we still need this flag?

Apologies, this is a relic and should not have been there, will send a
new revision with this removed.

- KP

> 
> >   /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
> >    * bytes on x86.  Pick a number to fit into BPF_IMAGE_SIZE / 2
> [...]

Andrii Nakryiko March 23, 2020, 8:18 p.m. UTC | #3

On Mon, Mar 23, 2020 at 9:45 AM KP Singh <[e-mail]> wrote:
>
> From: KP Singh <[e-mail]>
>
> JITed BPF programs are dynamically attached to the LSM hooks
> using BPF trampolines. The trampoline prologue generates code to handle
> conversion of the signature of the hook to the appropriate BPF context.
>
> The allocated trampoline programs are attached to the nop functions
> initialized as LSM hooks.
>
> BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
> and need CAP_SYS_ADMIN (required for loading eBPF programs).
>
> Upon attachment:
>
> * A BPF fexit trampoline is used for LSM hooks with a void return type.
> * A BPF fmod_ret trampoline is used for LSM hooks which return an
>   int. The attached programs can override the return value of the
>   bpf LSM hook to indicate a MAC Policy decision.
>
> Signed-off-by: KP Singh <[e-mail]>
> Reviewed-by: Brendan Jackman <[e-mail]>
> Reviewed-by: Florent Revest <[e-mail]>
> ---
>  include/linux/bpf.h     |  4 ++++
>  include/linux/bpf_lsm.h | 11 +++++++++++
>  kernel/bpf/bpf_lsm.c    | 29 +++++++++++++++++++++++++++++
>  kernel/bpf/btf.c        |  9 ++++++++-
>  kernel/bpf/syscall.c    | 26 ++++++++++++++++++++++----
>  kernel/bpf/trampoline.c | 17 +++++++++++++----
>  kernel/bpf/verifier.c   | 19 +++++++++++++++----
>  7 files changed, 102 insertions(+), 13 deletions(-)
>

[...]

>
> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> +
> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> +                       const struct bpf_prog *prog)
> +{
> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> +        */
> +       if (!capable(CAP_MAC_ADMIN))
> +               return -EPERM;
> +
> +       if (!prog->gpl_compatible) {
> +               bpf_log(vlog,
> +                       "LSM programs must have a GPL compatible license\n");
> +               return -EINVAL;
> +       }
> +
> +       if (strncmp(BPF_LSM_SYM_PREFX, prog->aux->attach_func_name,
> +                   strlen(BPF_LSM_SYM_PREFX))) {

sizeof(BPF_LSM_SYM_PREFIX) - 1?

> +               bpf_log(vlog, "attach_btf_id %u points to wrong type name %s\n",
> +                       prog->aux->attach_btf_id, prog->aux->attach_func_name);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +

[...]

> @@ -2367,10 +2369,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
>         struct file *link_file;
>         int link_fd, err;
>
> -       if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
> -           prog->expected_attach_type != BPF_TRACE_FEXIT &&
> -           prog->expected_attach_type != BPF_MODIFY_RETURN &&
> -           prog->type != BPF_PROG_TYPE_EXT) {
> +       switch (prog->type) {
> +       case BPF_PROG_TYPE_TRACING:
> +               if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
> +                   prog->expected_attach_type != BPF_TRACE_FEXIT &&
> +                   prog->expected_attach_type != BPF_MODIFY_RETURN) {
> +                       err = -EINVAL;
> +                       goto out_put_prog;
> +               }
> +               break;
> +       case BPF_PROG_TYPE_EXT:

It looks like an omission that we don't enforce expected_attach_type
to be 0 here. Should we fix it until it's too late?

> +               break;
> +       case BPF_PROG_TYPE_LSM:
> +               if (prog->expected_attach_type != BPF_LSM_MAC) {
> +                       err = -EINVAL;
> +                       goto out_put_prog;
> +               }
> +               break;
> +       default:
>                 err = -EINVAL;
>                 goto out_put_prog;
>         }
> @@ -2452,12 +2468,14 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
>         if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
>             prog->type != BPF_PROG_TYPE_TRACING &&
>             prog->type != BPF_PROG_TYPE_EXT &&
> +           prog->type != BPF_PROG_TYPE_LSM &&
>             prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
>                 err = -EINVAL;
>                 goto out_put_prog;
>         }
>
>         if (prog->type == BPF_PROG_TYPE_TRACING ||
> +           prog->type == BPF_PROG_TYPE_LSM ||
>             prog->type == BPF_PROG_TYPE_EXT) {


can you please refactor this into a nicer explicit switch instead of
combination of if/elses?

>                 if (attr->raw_tracepoint.name) {
>                         /* The attach point for this category of programs
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index f30bca2a4d01..9be85aa4ec5f 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -6,6 +6,7 @@
>  #include <linux/ftrace.h>
>  #include <linux/rbtree_latch.h>
>  #include <linux/perf_event.h>
> +#include <linux/btf.h>
>

[...]

Stephen Smalley March 24, 2020, 2:35 p.m. UTC | #4

On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
>
> From: KP Singh <[e-mail]>
>
> JITed BPF programs are dynamically attached to the LSM hooks
> using BPF trampolines. The trampoline prologue generates code to handle
> conversion of the signature of the hook to the appropriate BPF context.
>
> The allocated trampoline programs are attached to the nop functions
> initialized as LSM hooks.
>
> BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
> and need CAP_SYS_ADMIN (required for loading eBPF programs).
>
> Upon attachment:
>
> * A BPF fexit trampoline is used for LSM hooks with a void return type.
> * A BPF fmod_ret trampoline is used for LSM hooks which return an
>   int. The attached programs can override the return value of the
>   bpf LSM hook to indicate a MAC Policy decision.
>
> Signed-off-by: KP Singh <[e-mail]>
> Reviewed-by: Brendan Jackman <[e-mail]>
> Reviewed-by: Florent Revest <[e-mail]>
> ---

> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 530d137f7a84..2a8131b640b8 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -9,6 +9,9 @@
>  #include <linux/btf.h>
>  #include <linux/lsm_hooks.h>
>  #include <linux/bpf_lsm.h>
> +#include <linux/jump_label.h>
> +#include <linux/kallsyms.h>
> +#include <linux/bpf_verifier.h>
>
>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
>   * function where a BPF program can be attached as an fexit trampoline.
> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
>  #include <linux/lsm_hook_names.h>
>  #undef LSM_HOOK
>
> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> +
> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> +                       const struct bpf_prog *prog)
> +{
> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> +        */
> +       if (!capable(CAP_MAC_ADMIN))
> +               return -EPERM;

I had asked before, and will ask again: please provide an explicit LSM
hook for mediating whether one can make changes to the LSM hooks.
Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.

KP Singh March 24, 2020, 2:50 p.m. UTC | #5

On 24-Mär 10:35, Stephen Smalley wrote:
> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> >
> > From: KP Singh <[e-mail]>
> >
> > JITed BPF programs are dynamically attached to the LSM hooks
> > using BPF trampolines. The trampoline prologue generates code to handle
> > conversion of the signature of the hook to the appropriate BPF context.
> >
> > The allocated trampoline programs are attached to the nop functions
> > initialized as LSM hooks.
> >
> > BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
> > and need CAP_SYS_ADMIN (required for loading eBPF programs).
> >
> > Upon attachment:
> >
> > * A BPF fexit trampoline is used for LSM hooks with a void return type.
> > * A BPF fmod_ret trampoline is used for LSM hooks which return an
> >   int. The attached programs can override the return value of the
> >   bpf LSM hook to indicate a MAC Policy decision.
> >
> > Signed-off-by: KP Singh <[e-mail]>
> > Reviewed-by: Brendan Jackman <[e-mail]>
> > Reviewed-by: Florent Revest <[e-mail]>
> > ---
> 
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index 530d137f7a84..2a8131b640b8 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -9,6 +9,9 @@
> >  #include <linux/btf.h>
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/bpf_lsm.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/kallsyms.h>
> > +#include <linux/bpf_verifier.h>
> >
> >  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> >   * function where a BPF program can be attached as an fexit trampoline.
> > @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> >  #include <linux/lsm_hook_names.h>
> >  #undef LSM_HOOK
> >
> > +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > +
> > +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > +                       const struct bpf_prog *prog)
> > +{
> > +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > +        */
> > +       if (!capable(CAP_MAC_ADMIN))
> > +               return -EPERM;
> 
> I had asked before, and will ask again: please provide an explicit LSM
> hook for mediating whether one can make changes to the LSM hooks.
> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.

What do you think about:

  int security_check_mutable_hooks(void)

Do you have any suggestions on the signature of this hook? Does this
hook need to be BPF specific?

- KP

Stephen Smalley March 24, 2020, 2:58 p.m. UTC | #6

On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
>
> On 24-Mär 10:35, Stephen Smalley wrote:
> > On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> > >
> > > From: KP Singh <[e-mail]>
> > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > index 530d137f7a84..2a8131b640b8 100644
> > > --- a/kernel/bpf/bpf_lsm.c
> > > +++ b/kernel/bpf/bpf_lsm.c
> > > @@ -9,6 +9,9 @@
> > >  #include <linux/btf.h>
> > >  #include <linux/lsm_hooks.h>
> > >  #include <linux/bpf_lsm.h>
> > > +#include <linux/jump_label.h>
> > > +#include <linux/kallsyms.h>
> > > +#include <linux/bpf_verifier.h>
> > >
> > >  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > >   * function where a BPF program can be attached as an fexit trampoline.
> > > @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> > >  #include <linux/lsm_hook_names.h>
> > >  #undef LSM_HOOK
> > >
> > > +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > > +
> > > +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > +                       const struct bpf_prog *prog)
> > > +{
> > > +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > > +        */
> > > +       if (!capable(CAP_MAC_ADMIN))
> > > +               return -EPERM;
> >
> > I had asked before, and will ask again: please provide an explicit LSM
> > hook for mediating whether one can make changes to the LSM hooks.
> > Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
>
> What do you think about:
>
>   int security_check_mutable_hooks(void)
>
> Do you have any suggestions on the signature of this hook? Does this
> hook need to be BPF specific?

I'd do something like int security_bpf_prog_attach_security(const
struct bpf_prog *prog) or similar.
Then the security module can do a check based on the current task
and/or the prog.  We already have some bpf-specific hooks.

Casey Schaufler March 24, 2020, 4:25 p.m. UTC | #7

On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
>> On 24-Mär 10:35, Stephen Smalley wrote:
>>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
>>>> From: KP Singh <[e-mail]>
>>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
>>>> index 530d137f7a84..2a8131b640b8 100644
>>>> --- a/kernel/bpf/bpf_lsm.c
>>>> +++ b/kernel/bpf/bpf_lsm.c
>>>> @@ -9,6 +9,9 @@
>>>>  #include <linux/btf.h>
>>>>  #include <linux/lsm_hooks.h>
>>>>  #include <linux/bpf_lsm.h>
>>>> +#include <linux/jump_label.h>
>>>> +#include <linux/kallsyms.h>
>>>> +#include <linux/bpf_verifier.h>
>>>>
>>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
>>>>   * function where a BPF program can be attached as an fexit trampoline.
>>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
>>>>  #include <linux/lsm_hook_names.h>
>>>>  #undef LSM_HOOK
>>>>
>>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
>>>> +
>>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
>>>> +                       const struct bpf_prog *prog)
>>>> +{
>>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
>>>> +        */
>>>> +       if (!capable(CAP_MAC_ADMIN))
>>>> +               return -EPERM;
>>> I had asked before, and will ask again: please provide an explicit LSM
>>> hook for mediating whether one can make changes to the LSM hooks.
>>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
>> What do you think about:
>>
>>   int security_check_mutable_hooks(void)
>>
>> Do you have any suggestions on the signature of this hook? Does this
>> hook need to be BPF specific?
> I'd do something like int security_bpf_prog_attach_security(const
> struct bpf_prog *prog) or similar.
> Then the security module can do a check based on the current task
> and/or the prog.  We already have some bpf-specific hooks.

I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
Just as Yama policy is independent of SELinux policy so KRSI policy should
be independent of SELinux policy. I understand the argument that BDF programs
ought to be constrained by SELinux, but I don't think it's right. Further,
we've got unholy layering when security modules call security_ functions.
I'm not saying there is no case where it would be appropriate, but this is not
one of them.

Stephen Smalley March 24, 2020, 5:49 p.m. UTC | #8

On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler <[e-mail]> wrote:
>
> On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> > On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
> >> On 24-Mär 10:35, Stephen Smalley wrote:
> >>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> >>>> From: KP Singh <[e-mail]>
> >>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> >>>> index 530d137f7a84..2a8131b640b8 100644
> >>>> --- a/kernel/bpf/bpf_lsm.c
> >>>> +++ b/kernel/bpf/bpf_lsm.c
> >>>> @@ -9,6 +9,9 @@
> >>>>  #include <linux/btf.h>
> >>>>  #include <linux/lsm_hooks.h>
> >>>>  #include <linux/bpf_lsm.h>
> >>>> +#include <linux/jump_label.h>
> >>>> +#include <linux/kallsyms.h>
> >>>> +#include <linux/bpf_verifier.h>
> >>>>
> >>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> >>>>   * function where a BPF program can be attached as an fexit trampoline.
> >>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> >>>>  #include <linux/lsm_hook_names.h>
> >>>>  #undef LSM_HOOK
> >>>>
> >>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> >>>> +
> >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> >>>> +                       const struct bpf_prog *prog)
> >>>> +{
> >>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> >>>> +        */
> >>>> +       if (!capable(CAP_MAC_ADMIN))
> >>>> +               return -EPERM;
> >>> I had asked before, and will ask again: please provide an explicit LSM
> >>> hook for mediating whether one can make changes to the LSM hooks.
> >>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
> >> What do you think about:
> >>
> >>   int security_check_mutable_hooks(void)
> >>
> >> Do you have any suggestions on the signature of this hook? Does this
> >> hook need to be BPF specific?
> > I'd do something like int security_bpf_prog_attach_security(const
> > struct bpf_prog *prog) or similar.
> > Then the security module can do a check based on the current task
> > and/or the prog.  We already have some bpf-specific hooks.
>
> I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
> Just as Yama policy is independent of SELinux policy so KRSI policy should
> be independent of SELinux policy. I understand the argument that BDF programs
> ought to be constrained by SELinux, but I don't think it's right. Further,
> we've got unholy layering when security modules call security_ functions.
> I'm not saying there is no case where it would be appropriate, but this is not
> one of them.

I explained this previously.  The difference is that the BPF programs
are loaded from a userspace
process, not a kernel-resident module.  They already recognize there
is a difference here or
they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
problem with that
check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
privileged with respect to
SELinux, which is why I want an explicit hook.  This gets a NAK from
me until there is such a hook.

Kees Cook March 24, 2020, 6:01 p.m. UTC | #9

On Tue, Mar 24, 2020 at 01:49:34PM -0400, Stephen Smalley wrote:
> On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler <[e-mail]> wrote:
> >
> > On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> > > On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
> > >> On 24-Mär 10:35, Stephen Smalley wrote:
> > >>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> > >>>> From: KP Singh <[e-mail]>
> > >>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > >>>> index 530d137f7a84..2a8131b640b8 100644
> > >>>> --- a/kernel/bpf/bpf_lsm.c
> > >>>> +++ b/kernel/bpf/bpf_lsm.c
> > >>>> @@ -9,6 +9,9 @@
> > >>>>  #include <linux/btf.h>
> > >>>>  #include <linux/lsm_hooks.h>
> > >>>>  #include <linux/bpf_lsm.h>
> > >>>> +#include <linux/jump_label.h>
> > >>>> +#include <linux/kallsyms.h>
> > >>>> +#include <linux/bpf_verifier.h>
> > >>>>
> > >>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > >>>>   * function where a BPF program can be attached as an fexit trampoline.
> > >>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> > >>>>  #include <linux/lsm_hook_names.h>
> > >>>>  #undef LSM_HOOK
> > >>>>
> > >>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > >>>> +
> > >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > >>>> +                       const struct bpf_prog *prog)
> > >>>> +{
> > >>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > >>>> +        */
> > >>>> +       if (!capable(CAP_MAC_ADMIN))
> > >>>> +               return -EPERM;
> > >>> I had asked before, and will ask again: please provide an explicit LSM
> > >>> hook for mediating whether one can make changes to the LSM hooks.
> > >>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
> > >> What do you think about:
> > >>
> > >>   int security_check_mutable_hooks(void)
> > >>
> > >> Do you have any suggestions on the signature of this hook? Does this
> > >> hook need to be BPF specific?
> > > I'd do something like int security_bpf_prog_attach_security(const
> > > struct bpf_prog *prog) or similar.
> > > Then the security module can do a check based on the current task
> > > and/or the prog.  We already have some bpf-specific hooks.
> >
> > I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
> > Just as Yama policy is independent of SELinux policy so KRSI policy should
> > be independent of SELinux policy. I understand the argument that BDF programs
> > ought to be constrained by SELinux, but I don't think it's right. Further,
> > we've got unholy layering when security modules call security_ functions.
> > I'm not saying there is no case where it would be appropriate, but this is not
> > one of them.
> 
> I explained this previously.  The difference is that the BPF programs
> are loaded from a userspace
> process, not a kernel-resident module.  They already recognize there
> is a difference here or
> they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
> problem with that
> check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
> privileged with respect to
> SELinux, which is why I want an explicit hook.  This gets a NAK from
> me until there is such a hook.

Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
SELinux's need here? I.e. it can already examine that a hook is being
created for the LSM (since it has a distinct type, etc)?

KP Singh March 24, 2020, 6:06 p.m. UTC | #10

On 24-Mär 11:01, Kees Cook wrote:
> On Tue, Mar 24, 2020 at 01:49:34PM -0400, Stephen Smalley wrote:
> > On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler <[e-mail]> wrote:
> > >
> > > On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> > > > On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
> > > >> On 24-Mär 10:35, Stephen Smalley wrote:
> > > >>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> > > >>>> From: KP Singh <[e-mail]>
> > > >>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > >>>> index 530d137f7a84..2a8131b640b8 100644
> > > >>>> --- a/kernel/bpf/bpf_lsm.c
> > > >>>> +++ b/kernel/bpf/bpf_lsm.c
> > > >>>> @@ -9,6 +9,9 @@
> > > >>>>  #include <linux/btf.h>
> > > >>>>  #include <linux/lsm_hooks.h>
> > > >>>>  #include <linux/bpf_lsm.h>
> > > >>>> +#include <linux/jump_label.h>
> > > >>>> +#include <linux/kallsyms.h>
> > > >>>> +#include <linux/bpf_verifier.h>
> > > >>>>
> > > >>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > > >>>>   * function where a BPF program can be attached as an fexit trampoline.
> > > >>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> > > >>>>  #include <linux/lsm_hook_names.h>
> > > >>>>  #undef LSM_HOOK
> > > >>>>
> > > >>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > > >>>> +
> > > >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > >>>> +                       const struct bpf_prog *prog)
> > > >>>> +{
> > > >>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > > >>>> +        */
> > > >>>> +       if (!capable(CAP_MAC_ADMIN))
> > > >>>> +               return -EPERM;
> > > >>> I had asked before, and will ask again: please provide an explicit LSM
> > > >>> hook for mediating whether one can make changes to the LSM hooks.
> > > >>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
> > > >> What do you think about:
> > > >>
> > > >>   int security_check_mutable_hooks(void)
> > > >>
> > > >> Do you have any suggestions on the signature of this hook? Does this
> > > >> hook need to be BPF specific?
> > > > I'd do something like int security_bpf_prog_attach_security(const
> > > > struct bpf_prog *prog) or similar.
> > > > Then the security module can do a check based on the current task
> > > > and/or the prog.  We already have some bpf-specific hooks.
> > >
> > > I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
> > > Just as Yama policy is independent of SELinux policy so KRSI policy should
> > > be independent of SELinux policy. I understand the argument that BDF programs
> > > ought to be constrained by SELinux, but I don't think it's right. Further,
> > > we've got unholy layering when security modules call security_ functions.
> > > I'm not saying there is no case where it would be appropriate, but this is not
> > > one of them.
> > 
> > I explained this previously.  The difference is that the BPF programs
> > are loaded from a userspace
> > process, not a kernel-resident module.  They already recognize there
> > is a difference here or
> > they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
> > problem with that
> > check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
> > privileged with respect to
> > SELinux, which is why I want an explicit hook.  This gets a NAK from
> > me until there is such a hook.
> 
> Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
> SELinux's need here? I.e. it can already examine that a hook is being
> created for the LSM (since it has a distinct type, etc)?

I was about to say the same, specifically for the BPF use-case, we do
have the "bpf_prog" i.e. :

"Do a check when the kernel generate and return a file descriptor for
eBPF programs."

SELinux can implement its policy logic for BPF_PROG_TYPE_LSM by
providing a callback for this hook.

- KP

> 
> -- 
> Kees Cook

Stephen Smalley March 24, 2020, 6:21 p.m. UTC | #11

On Tue, Mar 24, 2020 at 2:06 PM KP Singh <[e-mail]> wrote:
>
> On 24-Mär 11:01, Kees Cook wrote:
> > On Tue, Mar 24, 2020 at 01:49:34PM -0400, Stephen Smalley wrote:
> > > On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler <[e-mail]> wrote:
> > > >
> > > > On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> > > > > On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
> > > > >> On 24-Mär 10:35, Stephen Smalley wrote:
> > > > >>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> > > > >>>> From: KP Singh <[e-mail]>
> > > > >>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > > >>>> index 530d137f7a84..2a8131b640b8 100644
> > > > >>>> --- a/kernel/bpf/bpf_lsm.c
> > > > >>>> +++ b/kernel/bpf/bpf_lsm.c
> > > > >>>> @@ -9,6 +9,9 @@
> > > > >>>>  #include <linux/btf.h>
> > > > >>>>  #include <linux/lsm_hooks.h>
> > > > >>>>  #include <linux/bpf_lsm.h>
> > > > >>>> +#include <linux/jump_label.h>
> > > > >>>> +#include <linux/kallsyms.h>
> > > > >>>> +#include <linux/bpf_verifier.h>
> > > > >>>>
> > > > >>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > > > >>>>   * function where a BPF program can be attached as an fexit trampoline.
> > > > >>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> > > > >>>>  #include <linux/lsm_hook_names.h>
> > > > >>>>  #undef LSM_HOOK
> > > > >>>>
> > > > >>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > > > >>>> +
> > > > >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > > >>>> +                       const struct bpf_prog *prog)
> > > > >>>> +{
> > > > >>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > > > >>>> +        */
> > > > >>>> +       if (!capable(CAP_MAC_ADMIN))
> > > > >>>> +               return -EPERM;
> > > > >>> I had asked before, and will ask again: please provide an explicit LSM
> > > > >>> hook for mediating whether one can make changes to the LSM hooks.
> > > > >>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
> > > > >> What do you think about:
> > > > >>
> > > > >>   int security_check_mutable_hooks(void)
> > > > >>
> > > > >> Do you have any suggestions on the signature of this hook? Does this
> > > > >> hook need to be BPF specific?
> > > > > I'd do something like int security_bpf_prog_attach_security(const
> > > > > struct bpf_prog *prog) or similar.
> > > > > Then the security module can do a check based on the current task
> > > > > and/or the prog.  We already have some bpf-specific hooks.
> > > >
> > > > I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
> > > > Just as Yama policy is independent of SELinux policy so KRSI policy should
> > > > be independent of SELinux policy. I understand the argument that BDF programs
> > > > ought to be constrained by SELinux, but I don't think it's right. Further,
> > > > we've got unholy layering when security modules call security_ functions.
> > > > I'm not saying there is no case where it would be appropriate, but this is not
> > > > one of them.
> > >
> > > I explained this previously.  The difference is that the BPF programs
> > > are loaded from a userspace
> > > process, not a kernel-resident module.  They already recognize there
> > > is a difference here or
> > > they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
> > > problem with that
> > > check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
> > > privileged with respect to
> > > SELinux, which is why I want an explicit hook.  This gets a NAK from
> > > me until there is such a hook.
> >
> > Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
> > SELinux's need here? I.e. it can already examine that a hook is being
> > created for the LSM (since it has a distinct type, etc)?
>
> I was about to say the same, specifically for the BPF use-case, we do
> have the "bpf_prog" i.e. :
>
> "Do a check when the kernel generate and return a file descriptor for
> eBPF programs."
>
> SELinux can implement its policy logic for BPF_PROG_TYPE_LSM by
> providing a callback for this hook.

Ok.  In that case do we really need the capable() check here at all?

KP Singh March 24, 2020, 6:27 p.m. UTC | #12

On 24-Mär 14:21, Stephen Smalley wrote:
> On Tue, Mar 24, 2020 at 2:06 PM KP Singh <[e-mail]> wrote:
> >
> > On 24-Mär 11:01, Kees Cook wrote:
> > > On Tue, Mar 24, 2020 at 01:49:34PM -0400, Stephen Smalley wrote:
> > > > On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler <[e-mail]> wrote:
> > > > >
> > > > > On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> > > > > > On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
> > > > > >> On 24-Mär 10:35, Stephen Smalley wrote:
> > > > > >>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> > > > > >>>> From: KP Singh <[e-mail]>
> > > > > >>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > > > >>>> index 530d137f7a84..2a8131b640b8 100644
> > > > > >>>> --- a/kernel/bpf/bpf_lsm.c
> > > > > >>>> +++ b/kernel/bpf/bpf_lsm.c
> > > > > >>>> @@ -9,6 +9,9 @@
> > > > > >>>>  #include <linux/btf.h>
> > > > > >>>>  #include <linux/lsm_hooks.h>
> > > > > >>>>  #include <linux/bpf_lsm.h>
> > > > > >>>> +#include <linux/jump_label.h>
> > > > > >>>> +#include <linux/kallsyms.h>
> > > > > >>>> +#include <linux/bpf_verifier.h>
> > > > > >>>>
> > > > > >>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > > > > >>>>   * function where a BPF program can be attached as an fexit trampoline.
> > > > > >>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> > > > > >>>>  #include <linux/lsm_hook_names.h>
> > > > > >>>>  #undef LSM_HOOK
> > > > > >>>>
> > > > > >>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > > > > >>>> +
> > > > > >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > > > >>>> +                       const struct bpf_prog *prog)
> > > > > >>>> +{
> > > > > >>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > > > > >>>> +        */
> > > > > >>>> +       if (!capable(CAP_MAC_ADMIN))
> > > > > >>>> +               return -EPERM;
> > > > > >>> I had asked before, and will ask again: please provide an explicit LSM
> > > > > >>> hook for mediating whether one can make changes to the LSM hooks.
> > > > > >>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
> > > > > >> What do you think about:
> > > > > >>
> > > > > >>   int security_check_mutable_hooks(void)
> > > > > >>
> > > > > >> Do you have any suggestions on the signature of this hook? Does this
> > > > > >> hook need to be BPF specific?
> > > > > > I'd do something like int security_bpf_prog_attach_security(const
> > > > > > struct bpf_prog *prog) or similar.
> > > > > > Then the security module can do a check based on the current task
> > > > > > and/or the prog.  We already have some bpf-specific hooks.
> > > > >
> > > > > I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
> > > > > Just as Yama policy is independent of SELinux policy so KRSI policy should
> > > > > be independent of SELinux policy. I understand the argument that BDF programs
> > > > > ought to be constrained by SELinux, but I don't think it's right. Further,
> > > > > we've got unholy layering when security modules call security_ functions.
> > > > > I'm not saying there is no case where it would be appropriate, but this is not
> > > > > one of them.
> > > >
> > > > I explained this previously.  The difference is that the BPF programs
> > > > are loaded from a userspace
> > > > process, not a kernel-resident module.  They already recognize there
> > > > is a difference here or
> > > > they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
> > > > problem with that
> > > > check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
> > > > privileged with respect to
> > > > SELinux, which is why I want an explicit hook.  This gets a NAK from
> > > > me until there is such a hook.
> > >
> > > Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
> > > SELinux's need here? I.e. it can already examine that a hook is being
> > > created for the LSM (since it has a distinct type, etc)?
> >
> > I was about to say the same, specifically for the BPF use-case, we do
> > have the "bpf_prog" i.e. :
> >
> > "Do a check when the kernel generate and return a file descriptor for
> > eBPF programs."
> >
> > SELinux can implement its policy logic for BPF_PROG_TYPE_LSM by
> > providing a callback for this hook.
> 
> Ok.  In that case do we really need the capable() check here at all?

We do not have a specific capable check for BPF_PROG_TYPE_LSM programs
now. There is a general check which requires CAP_SYS_ADMIN when
unprivileged BPF is disabled:

in kernel/bpf/sycall.c:

        if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
	        return -EPERM;

AFAIK, Most distros disable unprivileged eBPF.

Now that I look at this, I think we might need a CAP_MAC_ADMIN check
though as unprivileged BPF being enabled will result in an
unprivileged user being able to load MAC policies.

- KP

KP Singh March 24, 2020, 6:31 p.m. UTC | #13

On 24-Mär 19:27, KP Singh wrote:
> On 24-Mär 14:21, Stephen Smalley wrote:
> > On Tue, Mar 24, 2020 at 2:06 PM KP Singh <[e-mail]> wrote:
> > >
> > > On 24-Mär 11:01, Kees Cook wrote:
> > > > On Tue, Mar 24, 2020 at 01:49:34PM -0400, Stephen Smalley wrote:
> > > > > On Tue, Mar 24, 2020 at 12:25 PM Casey Schaufler <[e-mail]> wrote:
> > > > > >
> > > > > > On 3/24/2020 7:58 AM, Stephen Smalley wrote:
> > > > > > > On Tue, Mar 24, 2020 at 10:50 AM KP Singh <[e-mail]> wrote:
> > > > > > >> On 24-Mär 10:35, Stephen Smalley wrote:
> > > > > > >>> On Mon, Mar 23, 2020 at 12:46 PM KP Singh <[e-mail]> wrote:
> > > > > > >>>> From: KP Singh <[e-mail]>
> > > > > > >>>> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > > > > >>>> index 530d137f7a84..2a8131b640b8 100644
> > > > > > >>>> --- a/kernel/bpf/bpf_lsm.c
> > > > > > >>>> +++ b/kernel/bpf/bpf_lsm.c
> > > > > > >>>> @@ -9,6 +9,9 @@
> > > > > > >>>>  #include <linux/btf.h>
> > > > > > >>>>  #include <linux/lsm_hooks.h>
> > > > > > >>>>  #include <linux/bpf_lsm.h>
> > > > > > >>>> +#include <linux/jump_label.h>
> > > > > > >>>> +#include <linux/kallsyms.h>
> > > > > > >>>> +#include <linux/bpf_verifier.h>
> > > > > > >>>>
> > > > > > >>>>  /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
> > > > > > >>>>   * function where a BPF program can be attached as an fexit trampoline.
> > > > > > >>>> @@ -27,6 +30,32 @@ noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
> > > > > > >>>>  #include <linux/lsm_hook_names.h>
> > > > > > >>>>  #undef LSM_HOOK
> > > > > > >>>>
> > > > > > >>>> +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > > > > > >>>> +
> > > > > > >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > > > > >>>> +                       const struct bpf_prog *prog)
> > > > > > >>>> +{
> > > > > > >>>> +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > > > > > >>>> +        */
> > > > > > >>>> +       if (!capable(CAP_MAC_ADMIN))
> > > > > > >>>> +               return -EPERM;
> > > > > > >>> I had asked before, and will ask again: please provide an explicit LSM
> > > > > > >>> hook for mediating whether one can make changes to the LSM hooks.
> > > > > > >>> Neither CAP_MAC_ADMIN nor CAP_SYS_ADMIN suffices to check this for SELinux.
> > > > > > >> What do you think about:
> > > > > > >>
> > > > > > >>   int security_check_mutable_hooks(void)
> > > > > > >>
> > > > > > >> Do you have any suggestions on the signature of this hook? Does this
> > > > > > >> hook need to be BPF specific?
> > > > > > > I'd do something like int security_bpf_prog_attach_security(const
> > > > > > > struct bpf_prog *prog) or similar.
> > > > > > > Then the security module can do a check based on the current task
> > > > > > > and/or the prog.  We already have some bpf-specific hooks.
> > > > > >
> > > > > > I *strongly* disagree with Stephen on this. KRSI and SELinux are peers.
> > > > > > Just as Yama policy is independent of SELinux policy so KRSI policy should
> > > > > > be independent of SELinux policy. I understand the argument that BDF programs
> > > > > > ought to be constrained by SELinux, but I don't think it's right. Further,
> > > > > > we've got unholy layering when security modules call security_ functions.
> > > > > > I'm not saying there is no case where it would be appropriate, but this is not
> > > > > > one of them.
> > > > >
> > > > > I explained this previously.  The difference is that the BPF programs
> > > > > are loaded from a userspace
> > > > > process, not a kernel-resident module.  They already recognize there
> > > > > is a difference here or
> > > > > they wouldn't have the CAP_MAC_ADMIN check above in their patch.  The
> > > > > problem with that
> > > > > check is just that CAP_MAC_ADMIN doesn't necessarily mean fully
> > > > > privileged with respect to
> > > > > SELinux, which is why I want an explicit hook.  This gets a NAK from
> > > > > me until there is such a hook.
> > > >
> > > > Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
> > > > SELinux's need here? I.e. it can already examine that a hook is being
> > > > created for the LSM (since it has a distinct type, etc)?
> > >
> > > I was about to say the same, specifically for the BPF use-case, we do
> > > have the "bpf_prog" i.e. :
> > >
> > > "Do a check when the kernel generate and return a file descriptor for
> > > eBPF programs."
> > >
> > > SELinux can implement its policy logic for BPF_PROG_TYPE_LSM by
> > > providing a callback for this hook.
> > 
> > Ok.  In that case do we really need the capable() check here at all?
> 
> We do not have a specific capable check for BPF_PROG_TYPE_LSM programs
> now. There is a general check which requires CAP_SYS_ADMIN when
> unprivileged BPF is disabled:
> 
> in kernel/bpf/sycall.c:
> 
>         if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> 	        return -EPERM;
> 
> AFAIK, Most distros disable unprivileged eBPF.
> 
> Now that I look at this, I think we might need a CAP_MAC_ADMIN check
> though as unprivileged BPF being enabled will result in an
> unprivileged user being able to load MAC policies.

Actually we do have an extra check for loading BPF programs:


in kernel/bpf/syscall.c:bpf_prog_load

	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
	    type != BPF_PROG_TYPE_CGROUP_SKB &&
	    !capable(CAP_SYS_ADMIN))
		return -EPERM;

Do you think we still need a CAP_MAC_ADMIN check for LSM programs?

- KP

> 
> - KP

Kees Cook March 24, 2020, 6:33 p.m. UTC | #14

On Tue, Mar 24, 2020 at 02:21:30PM -0400, Stephen Smalley wrote:
> On Tue, Mar 24, 2020 at 2:06 PM KP Singh <[e-mail]> wrote:
> >
> > On 24-Mär 11:01, Kees Cook wrote:
> > > Doesn't the existing int (*bpf_prog)(struct bpf_prog *prog); cover
> > > SELinux's need here? I.e. it can already examine that a hook is being
> > > created for the LSM (since it has a distinct type, etc)?
> >
> > I was about to say the same, specifically for the BPF use-case, we do
> > have the "bpf_prog" i.e. :
> >
> > "Do a check when the kernel generate and return a file descriptor for
> > eBPF programs."
> >
> > SELinux can implement its policy logic for BPF_PROG_TYPE_LSM by
> > providing a callback for this hook.
> 
> Ok.  In that case do we really need the capable() check here at all?

IMO, this is for systems without SELinux, where they're using the
capabilities as the basic policy for MAC management.

Kees Cook March 24, 2020, 6:34 p.m. UTC | #15

On Tue, Mar 24, 2020 at 07:31:30PM +0100, KP Singh wrote:
> On 24-Mär 19:27, KP Singh wrote:
> > We do not have a specific capable check for BPF_PROG_TYPE_LSM programs
> > now. There is a general check which requires CAP_SYS_ADMIN when
> > unprivileged BPF is disabled:
> > 
> > in kernel/bpf/sycall.c:
> > 
> >         if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> > 	        return -EPERM;
> > 
> > AFAIK, Most distros disable unprivileged eBPF.
> > 
> > Now that I look at this, I think we might need a CAP_MAC_ADMIN check
> > though as unprivileged BPF being enabled will result in an
> > unprivileged user being able to load MAC policies.
> 
> Actually we do have an extra check for loading BPF programs:
> 
> 
> in kernel/bpf/syscall.c:bpf_prog_load
> 
> 	if (type != BPF_PROG_TYPE_SOCKET_FILTER &&
> 	    type != BPF_PROG_TYPE_CGROUP_SKB &&
> 	    !capable(CAP_SYS_ADMIN))
> 		return -EPERM;
> 
> Do you think we still need a CAP_MAC_ADMIN check for LSM programs?

IMO, these are distinct privileges on the non-SELinux system. I think
your patch is fine as-is.

KP Singh March 24, 2020, 7 p.m. UTC | #16

On 23-Mär 13:18, Andrii Nakryiko wrote:
> On Mon, Mar 23, 2020 at 9:45 AM KP Singh <[e-mail]> wrote:
> >
> > From: KP Singh <[e-mail]>
> >
> > JITed BPF programs are dynamically attached to the LSM hooks
> > using BPF trampolines. The trampoline prologue generates code to handle
> > conversion of the signature of the hook to the appropriate BPF context.
> >
> > The allocated trampoline programs are attached to the nop functions
> > initialized as LSM hooks.
> >
> > BPF_PROG_TYPE_LSM programs must have a GPL compatible license and
> > and need CAP_SYS_ADMIN (required for loading eBPF programs).
> >
> > Upon attachment:
> >
> > * A BPF fexit trampoline is used for LSM hooks with a void return type.
> > * A BPF fmod_ret trampoline is used for LSM hooks which return an
> >   int. The attached programs can override the return value of the
> >   bpf LSM hook to indicate a MAC Policy decision.
> >
> > Signed-off-by: KP Singh <[e-mail]>
> > Reviewed-by: Brendan Jackman <[e-mail]>
> > Reviewed-by: Florent Revest <[e-mail]>
> > ---
> >  include/linux/bpf.h     |  4 ++++
> >  include/linux/bpf_lsm.h | 11 +++++++++++
> >  kernel/bpf/bpf_lsm.c    | 29 +++++++++++++++++++++++++++++
> >  kernel/bpf/btf.c        |  9 ++++++++-
> >  kernel/bpf/syscall.c    | 26 ++++++++++++++++++++++----
> >  kernel/bpf/trampoline.c | 17 +++++++++++++----
> >  kernel/bpf/verifier.c   | 19 +++++++++++++++----
> >  7 files changed, 102 insertions(+), 13 deletions(-)
> >
> 
> [...]
> 
> >
> > +#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
> > +
> > +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > +                       const struct bpf_prog *prog)
> > +{
> > +       /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
> > +        */
> > +       if (!capable(CAP_MAC_ADMIN))
> > +               return -EPERM;
> > +
> > +       if (!prog->gpl_compatible) {
> > +               bpf_log(vlog,
> > +                       "LSM programs must have a GPL compatible license\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (strncmp(BPF_LSM_SYM_PREFX, prog->aux->attach_func_name,
> > +                   strlen(BPF_LSM_SYM_PREFX))) {
> 
> sizeof(BPF_LSM_SYM_PREFIX) - 1?

Thanks, done.

> 
> > +               bpf_log(vlog, "attach_btf_id %u points to wrong type name %s\n",
> > +                       prog->aux->attach_btf_id, prog->aux->attach_func_name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> 
> [...]
> 
> > @@ -2367,10 +2369,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
> >         struct file *link_file;
> >         int link_fd, err;
> >
> > -       if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
> > -           prog->expected_attach_type != BPF_TRACE_FEXIT &&
> > -           prog->expected_attach_type != BPF_MODIFY_RETURN &&
> > -           prog->type != BPF_PROG_TYPE_EXT) {
> > +       switch (prog->type) {
> > +       case BPF_PROG_TYPE_TRACING:
> > +               if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
> > +                   prog->expected_attach_type != BPF_TRACE_FEXIT &&
> > +                   prog->expected_attach_type != BPF_MODIFY_RETURN) {
> > +                       err = -EINVAL;
> > +                       goto out_put_prog;
> > +               }
> > +               break;
> > +       case BPF_PROG_TYPE_EXT:
> 
> It looks like an omission that we don't enforce expected_attach_type
> to be 0 here. Should we fix it until it's too late?

Done.

> 
> > +               break;
> > +       case BPF_PROG_TYPE_LSM:
> > +               if (prog->expected_attach_type != BPF_LSM_MAC) {
> > +                       err = -EINVAL;
> > +                       goto out_put_prog;
> > +               }
> > +               break;
> > +       default:
> >                 err = -EINVAL;
> >                 goto out_put_prog;
> >         }
> > @@ -2452,12 +2468,14 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
> >         if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
> >             prog->type != BPF_PROG_TYPE_TRACING &&
> >             prog->type != BPF_PROG_TYPE_EXT &&
> > +           prog->type != BPF_PROG_TYPE_LSM &&
> >             prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
> >                 err = -EINVAL;
> >                 goto out_put_prog;
> >         }
> >
> >         if (prog->type == BPF_PROG_TYPE_TRACING ||
> > +           prog->type == BPF_PROG_TYPE_LSM ||
> >             prog->type == BPF_PROG_TYPE_EXT) {
> 
> 
> can you please refactor this into a nicer explicit switch instead of
> combination of if/elses?

Done.

- KP

> 
> >                 if (attr->raw_tracepoint.name) {
> >                         /* The attach point for this category of programs
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index f30bca2a4d01..9be85aa4ec5f 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/ftrace.h>
> >  #include <linux/rbtree_latch.h>
> >  #include <linux/perf_event.h>
> > +#include <linux/btf.h>
> >
> 
> [...]

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index af81ec7b783c..adf2e5a6de4b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -433,6 +433,10 @@  struct btf_func_model {
  * programs only. Should not be used with normal calls and indirect calls.
  */
 #define BPF_TRAMP_F_SKIP_FRAME		BIT(2)
+/* Override the return value of the original function. This flag only makes
+ * sense for fexit trampolines.
+ */
+#define BPF_TRAMP_F_OVERRIDE_RETURN     BIT(3)
 
 /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
  * bytes on x86.  Pick a number to fit into BPF_IMAGE_SIZE / 2
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index c6423a140220..9bac0a11f303 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -16,6 +16,17 @@ 
 #include <linux/lsm_hook_names.h>
 #undef LSM_HOOK
 
+int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
+			const struct bpf_prog *prog);
+
+#else /* !CONFIG_BPF_LSM */
+
+static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
+				      const struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_BPF_LSM */
 
 #endif /* _LINUX_BPF_LSM_H */
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 530d137f7a84..2a8131b640b8 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -9,6 +9,9 @@ 
 #include <linux/btf.h>
 #include <linux/lsm_hooks.h>
 #include <linux/bpf_lsm.h>
+#include <linux/jump_label.h>
+#include <linux/kallsyms.h>
+#include <linux/bpf_verifier.h>
 
 /* For every LSM hook  that allows attachment of BPF programs, declare a NOP
  * function where a BPF program can be attached as an fexit trampoline.
@@ -27,6 +30,32 @@  noinline __weak void bpf_lsm_##NAME(__VA_ARGS__) {}
 #include <linux/lsm_hook_names.h>
 #undef LSM_HOOK
 
+#define BPF_LSM_SYM_PREFX  "bpf_lsm_"
+
+int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
+			const struct bpf_prog *prog)
+{
+	/* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
+	 */
+	if (!capable(CAP_MAC_ADMIN))
+		return -EPERM;
+
+	if (!prog->gpl_compatible) {
+		bpf_log(vlog,
+			"LSM programs must have a GPL compatible license\n");
+		return -EINVAL;
+	}
+
+	if (strncmp(BPF_LSM_SYM_PREFX, prog->aux->attach_func_name,
+		    strlen(BPF_LSM_SYM_PREFX))) {
+		bpf_log(vlog, "attach_btf_id %u points to wrong type name %s\n",
+			prog->aux->attach_btf_id, prog->aux->attach_func_name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 const struct bpf_prog_ops lsm_prog_ops = {
 };
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6f397c4da05e..67466dd59a35 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3710,7 +3710,14 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	}
 
 	if (arg == nr_args) {
-		if (prog->expected_attach_type == BPF_TRACE_FEXIT) {
+		/* BPF_LSM_MAC programs only have int and void functions they
+		 * can be attached to. When they are attached to a void function
+		 * they result in the creation of an FEXIT trampoline and when
+		 * to a function that returns an int, a MODIFY_RETURN
+		 * trampoline.
+		 */
+		if (prog->expected_attach_type == BPF_TRACE_FEXIT ||
+		    prog->expected_attach_type == BPF_LSM_MAC) {
 			if (!t)
 				return true;
 			t = btf_type_by_id(btf, t->type);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 85567a6ea5f9..845bdfb35852 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -25,6 +25,7 @@ 
 #include <linux/nospec.h>
 #include <linux/audit.h>
 #include <uapi/linux/btf.h>
+#include <linux/bpf_lsm.h>
 
 #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \
 			  (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \
@@ -1935,6 +1936,7 @@  bpf_prog_load_check_attach(enum bpf_prog_type prog_type,
 
 		switch (prog_type) {
 		case BPF_PROG_TYPE_TRACING:
+		case BPF_PROG_TYPE_LSM:
 		case BPF_PROG_TYPE_STRUCT_OPS:
 		case BPF_PROG_TYPE_EXT:
 			break;
@@ -2367,10 +2369,24 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 	struct file *link_file;
 	int link_fd, err;
 
-	if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
-	    prog->expected_attach_type != BPF_TRACE_FEXIT &&
-	    prog->expected_attach_type != BPF_MODIFY_RETURN &&
-	    prog->type != BPF_PROG_TYPE_EXT) {
+	switch (prog->type) {
+	case BPF_PROG_TYPE_TRACING:
+		if (prog->expected_attach_type != BPF_TRACE_FENTRY &&
+		    prog->expected_attach_type != BPF_TRACE_FEXIT &&
+		    prog->expected_attach_type != BPF_MODIFY_RETURN) {
+			err = -EINVAL;
+			goto out_put_prog;
+		}
+		break;
+	case BPF_PROG_TYPE_EXT:
+		break;
+	case BPF_PROG_TYPE_LSM:
+		if (prog->expected_attach_type != BPF_LSM_MAC) {
+			err = -EINVAL;
+			goto out_put_prog;
+		}
+		break;
+	default:
 		err = -EINVAL;
 		goto out_put_prog;
 	}
@@ -2452,12 +2468,14 @@  static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
 	if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT &&
 	    prog->type != BPF_PROG_TYPE_TRACING &&
 	    prog->type != BPF_PROG_TYPE_EXT &&
+	    prog->type != BPF_PROG_TYPE_LSM &&
 	    prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) {
 		err = -EINVAL;
 		goto out_put_prog;
 	}
 
 	if (prog->type == BPF_PROG_TYPE_TRACING ||
+	    prog->type == BPF_PROG_TYPE_LSM ||
 	    prog->type == BPF_PROG_TYPE_EXT) {
 		if (attr->raw_tracepoint.name) {
 			/* The attach point for this category of programs
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f30bca2a4d01..9be85aa4ec5f 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -6,6 +6,7 @@ 
 #include <linux/ftrace.h>
 #include <linux/rbtree_latch.h>
 #include <linux/perf_event.h>
+#include <linux/btf.h>
 
 /* dummy _ops. The verifier will operate on target program's ops. */
 const struct bpf_verifier_ops bpf_extension_verifier_ops = {
@@ -233,15 +234,23 @@  static int bpf_trampoline_update(struct bpf_trampoline *tr)
 	return err;
 }
 
-static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(enum bpf_attach_type t)
+static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
 {
-	switch (t) {
+	switch (prog->expected_attach_type) {
 	case BPF_TRACE_FENTRY:
 		return BPF_TRAMP_FENTRY;
 	case BPF_MODIFY_RETURN:
 		return BPF_TRAMP_MODIFY_RETURN;
 	case BPF_TRACE_FEXIT:
 		return BPF_TRAMP_FEXIT;
+	case BPF_LSM_MAC:
+		if (!prog->aux->attach_func_proto->type)
+			/* The function returns void, we cannot modify its
+			 * return value.
+			 */
+			return BPF_TRAMP_FEXIT;
+		else
+			return BPF_TRAMP_MODIFY_RETURN;
 	default:
 		return BPF_TRAMP_REPLACE;
 	}
@@ -255,7 +264,7 @@  int bpf_trampoline_link_prog(struct bpf_prog *prog)
 	int cnt;
 
 	tr = prog->aux->trampoline;
-	kind = bpf_attach_type_to_tramp(prog->expected_attach_type);
+	kind = bpf_attach_type_to_tramp(prog);
 	mutex_lock(&tr->mutex);
 	if (tr->extension_prog) {
 		/* cannot attach fentry/fexit if extension prog is attached.
@@ -305,7 +314,7 @@  int bpf_trampoline_unlink_prog(struct bpf_prog *prog)
 	int err;
 
 	tr = prog->aux->trampoline;
-	kind = bpf_attach_type_to_tramp(prog->expected_attach_type);
+	kind = bpf_attach_type_to_tramp(prog);
 	mutex_lock(&tr->mutex);
 	if (kind == BPF_TRAMP_REPLACE) {
 		WARN_ON_ONCE(!tr->extension_prog);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 745f3cfdf3b2..c5024499f86b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20,6 +20,7 @@ 
 #include <linux/perf_event.h>
 #include <linux/ctype.h>
 #include <linux/error-injection.h>
+#include <linux/bpf_lsm.h>
 
 #include "disasm.h"
 
@@ -6412,8 +6413,9 @@  static int check_return_code(struct bpf_verifier_env *env)
 	struct tnum range = tnum_range(0, 1);
 	int err;
 
-	/* The struct_ops func-ptr's return type could be "void" */
-	if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS &&
+	/* LSM and struct_ops func-ptr's return type could be "void" */
+	if ((env->prog->type == BPF_PROG_TYPE_STRUCT_OPS ||
+	     env->prog->type == BPF_PROG_TYPE_LSM) &&
 	    !prog->aux->attach_func_proto->type)
 		return 0;
 
@@ -9843,7 +9845,9 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
 		return check_struct_ops_btf_id(env);
 
-	if (prog->type != BPF_PROG_TYPE_TRACING && !prog_extension)
+	if (prog->type != BPF_PROG_TYPE_TRACING &&
+	    prog->type != BPF_PROG_TYPE_LSM &&
+	    !prog_extension)
 		return 0;
 
 	if (!btf_id) {
@@ -9974,8 +9978,16 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 			return -EINVAL;
 		/* fallthrough */
 	case BPF_MODIFY_RETURN:
+	case BPF_LSM_MAC:
 	case BPF_TRACE_FENTRY:
 	case BPF_TRACE_FEXIT:
+		prog->aux->attach_func_name = tname;
+		if (prog->type == BPF_PROG_TYPE_LSM) {
+			ret = bpf_lsm_verify_prog(&env->log, prog);
+			if (ret < 0)
+				return ret;
+		}
+
 		if (!btf_type_is_func(t)) {
 			verbose(env, "attach_btf_id %u is not a function\n",
 				btf_id);
@@ -9990,7 +10002,6 @@  static int check_attach_btf_id(struct bpf_verifier_env *env)
 		tr = bpf_trampoline_lookup(key);
 		if (!tr)
 			return -ENOMEM;
-		prog->aux->attach_func_name = tname;
 		/* t is either vmlinux type or another program's type */
 		prog->aux->attach_func_proto = t;
 		mutex_lock(&tr->mutex);