[bpf-next,v5,5/8] bpf: Fix context type resolving for extension programs

From: Toke Høiland-Jørgensen <toke@redhat.com>

Eelco reported we can't properly access arguments if the tracing
program is attached to extension program.

Having following program:

  SEC("classifier/test_pkt_md_access")
  int test_pkt_md_access(struct __sk_buff *skb)

with its extension:

  SEC("freplace/test_pkt_md_access")
  int test_pkt_md_access_new(struct __sk_buff *skb)

and tracing that extension with:

  SEC("fentry/test_pkt_md_access_new")
  int BPF_PROG(fentry, struct sk_buff *skb)

It's not possible to access skb argument in the fentry program,
with following error from verifier:

  ; int BPF_PROG(fentry, struct sk_buff *skb)
  0: (79) r1 = *(u64 *)(r1 +0)
  invalid bpf_context access off=0 size=8

The problem is that btf_ctx_access gets the context type for the
traced program, which is in this case the extension.

But when we trace extension program, we want to get the context
type of the program that the extension is attached to, so we can
access the argument properly in the trace program.

This version of the patch is tweaked slightly from Jiri's original one,
since the refactoring in the previous patches means we have to get the
target prog type from the new variable in prog->aux instead of directly
from the target prog.

Reported-by: Eelco Chaudron <echaudro@redhat.com>
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 kernel/bpf/btf.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Eelco reported we can't properly access arguments if the tracing
> program is attached to extension program.
>
> Having following program:
>
>   SEC("classifier/test_pkt_md_access")
>   int test_pkt_md_access(struct __sk_buff *skb)
>
> with its extension:
>
>   SEC("freplace/test_pkt_md_access")
>   int test_pkt_md_access_new(struct __sk_buff *skb)
>
> and tracing that extension with:
>
>   SEC("fentry/test_pkt_md_access_new")
>   int BPF_PROG(fentry, struct sk_buff *skb)
>
> It's not possible to access skb argument in the fentry program,
> with following error from verifier:
>
>   ; int BPF_PROG(fentry, struct sk_buff *skb)
>   0: (79) r1 = *(u64 *)(r1 +0)
>   invalid bpf_context access off=0 size=8
>
> The problem is that btf_ctx_access gets the context type for the
> traced program, which is in this case the extension.
>
> But when we trace extension program, we want to get the context
> type of the program that the extension is attached to, so we can
> access the argument properly in the trace program.
>
> This version of the patch is tweaked slightly from Jiri's original one,
> since the refactoring in the previous patches means we have to get the
> target prog type from the new variable in prog->aux instead of directly
> from the target prog.
>
> Reported-by: Eelco Chaudron <echaudro@redhat.com>
> Suggested-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  kernel/bpf/btf.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 9228af9917a8..55f7b2ba1cbd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>
>         info->reg_type = PTR_TO_BTF_ID;
>         if (tgt_prog) {
> -               ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
> +               enum bpf_prog_type tgt_type;
> +
> +               if (tgt_prog->type == BPF_PROG_TYPE_EXT)
> +                       tgt_type = tgt_prog->aux->tgt_prog_type;

what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
this be a loop?

Which also brings up a few follow up questions. Now that we allow same
PROG_EXT program to be attached to multiple other programs:

1. what prevents us from attaching PROG_EXT to itself?
2. How do we prevent long chain of EXT programs or even loops?

Can you please add a few selftests testing such cases? I have a
feeling that with your changes in this patch set now it's possible to
break the kernel very easily. I don't know what the proper solution
is, but let's at least have a test that does show breakage, then try
to figure out the solution. See also comment in check_attach_btf_id()
about fentry/fexit and freplace interactions. That might not be
enough.


> +               else
> +                       tgt_type = tgt_prog->type;
> +
> +               ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
>                 if (ret > 0) {
>                         info->btf_id = ret;
>                         return true;
>

On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > From: Toke Høiland-Jørgensen <toke@redhat.com>
> >
> > Eelco reported we can't properly access arguments if the tracing
> > program is attached to extension program.
> >
> > Having following program:
> >
> >   SEC("classifier/test_pkt_md_access")
> >   int test_pkt_md_access(struct __sk_buff *skb)
> >
> > with its extension:
> >
> >   SEC("freplace/test_pkt_md_access")
> >   int test_pkt_md_access_new(struct __sk_buff *skb)
> >
> > and tracing that extension with:
> >
> >   SEC("fentry/test_pkt_md_access_new")
> >   int BPF_PROG(fentry, struct sk_buff *skb)
> >
> > It's not possible to access skb argument in the fentry program,
> > with following error from verifier:
> >
> >   ; int BPF_PROG(fentry, struct sk_buff *skb)
> >   0: (79) r1 = *(u64 *)(r1 +0)
> >   invalid bpf_context access off=0 size=8
> >
> > The problem is that btf_ctx_access gets the context type for the
> > traced program, which is in this case the extension.
> >
> > But when we trace extension program, we want to get the context
> > type of the program that the extension is attached to, so we can
> > access the argument properly in the trace program.
> >
> > This version of the patch is tweaked slightly from Jiri's original one,
> > since the refactoring in the previous patches means we have to get the
> > target prog type from the new variable in prog->aux instead of directly
> > from the target prog.
> >
> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> >  kernel/bpf/btf.c |    9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 9228af9917a8..55f7b2ba1cbd 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >
> >         info->reg_type = PTR_TO_BTF_ID;
> >         if (tgt_prog) {
> > -               ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
> > +               enum bpf_prog_type tgt_type;
> > +
> > +               if (tgt_prog->type == BPF_PROG_TYPE_EXT)
> > +                       tgt_type = tgt_prog->aux->tgt_prog_type;
>
> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
> this be a loop?

ok, never mind this specifically. there is an explicit check

if (tgt_prog->type == prog->type) {
    verbose(env, "Cannot recursively attach\n");
    return -EINVAL;
}

that will prevent this.

But, I think we still will be able to construct a long chain of
fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
infinitum. Can you please construct such a selftest? And then we
should probably fix those checks to also disallow FMOD_RET, in
addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
prog type should check if that can cause any problems).

>
> Which also brings up a few follow up questions. Now that we allow same
> PROG_EXT program to be attached to multiple other programs:
>
> 1. what prevents us from attaching PROG_EXT to itself?
> 2. How do we prevent long chain of EXT programs or even loops?
>
> Can you please add a few selftests testing such cases? I have a
> feeling that with your changes in this patch set now it's possible to
> break the kernel very easily. I don't know what the proper solution
> is, but let's at least have a test that does show breakage, then try
> to figure out the solution. See also comment in check_attach_btf_id()
> about fentry/fexit and freplace interactions. That might not be
> enough.
>
>
> > +               else
> > +                       tgt_type = tgt_prog->type;
> > +
> > +               ret = btf_translate_to_vmlinux(log, btf, t, tgt_type, arg);
> >                 if (ret > 0) {
> >                         info->btf_id = ret;
> >                         return true;
> >

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> > From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> > Eelco reported we can't properly access arguments if the tracing
>> > program is attached to extension program.
>> >
>> > Having following program:
>> >
>> >   SEC("classifier/test_pkt_md_access")
>> >   int test_pkt_md_access(struct __sk_buff *skb)
>> >
>> > with its extension:
>> >
>> >   SEC("freplace/test_pkt_md_access")
>> >   int test_pkt_md_access_new(struct __sk_buff *skb)
>> >
>> > and tracing that extension with:
>> >
>> >   SEC("fentry/test_pkt_md_access_new")
>> >   int BPF_PROG(fentry, struct sk_buff *skb)
>> >
>> > It's not possible to access skb argument in the fentry program,
>> > with following error from verifier:
>> >
>> >   ; int BPF_PROG(fentry, struct sk_buff *skb)
>> >   0: (79) r1 = *(u64 *)(r1 +0)
>> >   invalid bpf_context access off=0 size=8
>> >
>> > The problem is that btf_ctx_access gets the context type for the
>> > traced program, which is in this case the extension.
>> >
>> > But when we trace extension program, we want to get the context
>> > type of the program that the extension is attached to, so we can
>> > access the argument properly in the trace program.
>> >
>> > This version of the patch is tweaked slightly from Jiri's original one,
>> > since the refactoring in the previous patches means we have to get the
>> > target prog type from the new variable in prog->aux instead of directly
>> > from the target prog.
>> >
>> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> >  kernel/bpf/btf.c |    9 ++++++++-
>> >  1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> > index 9228af9917a8..55f7b2ba1cbd 100644
>> > --- a/kernel/bpf/btf.c
>> > +++ b/kernel/bpf/btf.c
>> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> >
>> >         info->reg_type = PTR_TO_BTF_ID;
>> >         if (tgt_prog) {
>> > -               ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>> > +               enum bpf_prog_type tgt_type;
>> > +
>> > +               if (tgt_prog->type == BPF_PROG_TYPE_EXT)
>> > +                       tgt_type = tgt_prog->aux->tgt_prog_type;
>>
>> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
>> this be a loop?
>
> ok, never mind this specifically. there is an explicit check
>
> if (tgt_prog->type == prog->type) {
>     verbose(env, "Cannot recursively attach\n");
>     return -EINVAL;
> }
>
> that will prevent this.
>
> But, I think we still will be able to construct a long chain of
> fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
> infinitum. Can you please construct such a selftest? And then we
> should probably fix those checks to also disallow FMOD_RET, in
> addition to BPF_TRACE_FENTRY/FEXIT

Sure, can do!

-Toke

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> > From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> > Eelco reported we can't properly access arguments if the tracing
>> > program is attached to extension program.
>> >
>> > Having following program:
>> >
>> >   SEC("classifier/test_pkt_md_access")
>> >   int test_pkt_md_access(struct __sk_buff *skb)
>> >
>> > with its extension:
>> >
>> >   SEC("freplace/test_pkt_md_access")
>> >   int test_pkt_md_access_new(struct __sk_buff *skb)
>> >
>> > and tracing that extension with:
>> >
>> >   SEC("fentry/test_pkt_md_access_new")
>> >   int BPF_PROG(fentry, struct sk_buff *skb)
>> >
>> > It's not possible to access skb argument in the fentry program,
>> > with following error from verifier:
>> >
>> >   ; int BPF_PROG(fentry, struct sk_buff *skb)
>> >   0: (79) r1 = *(u64 *)(r1 +0)
>> >   invalid bpf_context access off=0 size=8
>> >
>> > The problem is that btf_ctx_access gets the context type for the
>> > traced program, which is in this case the extension.
>> >
>> > But when we trace extension program, we want to get the context
>> > type of the program that the extension is attached to, so we can
>> > access the argument properly in the trace program.
>> >
>> > This version of the patch is tweaked slightly from Jiri's original one,
>> > since the refactoring in the previous patches means we have to get the
>> > target prog type from the new variable in prog->aux instead of directly
>> > from the target prog.
>> >
>> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> >  kernel/bpf/btf.c |    9 ++++++++-
>> >  1 file changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> > index 9228af9917a8..55f7b2ba1cbd 100644
>> > --- a/kernel/bpf/btf.c
>> > +++ b/kernel/bpf/btf.c
>> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> >
>> >         info->reg_type = PTR_TO_BTF_ID;
>> >         if (tgt_prog) {
>> > -               ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>> > +               enum bpf_prog_type tgt_type;
>> > +
>> > +               if (tgt_prog->type == BPF_PROG_TYPE_EXT)
>> > +                       tgt_type = tgt_prog->aux->tgt_prog_type;
>>
>> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
>> this be a loop?
>
> ok, never mind this specifically. there is an explicit check
>
> if (tgt_prog->type == prog->type) {
>     verbose(env, "Cannot recursively attach\n");
>     return -EINVAL;
> }
>
> that will prevent this.
>
> But, I think we still will be able to construct a long chain of
> fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
> infinitum. Can you please construct such a selftest? And then we
> should probably fix those checks to also disallow FMOD_RET, in
> addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
> prog type should check if that can cause any problems).

Huh, I thought fmod_ret was supposed to be for kernel functions only?
However, I can't really point to anywhere in the code that ensures this,
other than check_attach_modify_return(), but I think that will allow a
bpf function as long as its name starts with "security_" ?

Is there actually any use case for modify_return being attached to a BPF
function (you could just use freplace instead, couldn't you?). Or should
we just disallow that entirely (if I'm not missing somewhere it's
already blocked)?

-Toke

On Thu, Sep 17, 2020 at 10:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >
> >> > From: Toke Høiland-Jørgensen <toke@redhat.com>
> >> >
> >> > Eelco reported we can't properly access arguments if the tracing
> >> > program is attached to extension program.
> >> >
> >> > Having following program:
> >> >
> >> >   SEC("classifier/test_pkt_md_access")
> >> >   int test_pkt_md_access(struct __sk_buff *skb)
> >> >
> >> > with its extension:
> >> >
> >> >   SEC("freplace/test_pkt_md_access")
> >> >   int test_pkt_md_access_new(struct __sk_buff *skb)
> >> >
> >> > and tracing that extension with:
> >> >
> >> >   SEC("fentry/test_pkt_md_access_new")
> >> >   int BPF_PROG(fentry, struct sk_buff *skb)
> >> >
> >> > It's not possible to access skb argument in the fentry program,
> >> > with following error from verifier:
> >> >
> >> >   ; int BPF_PROG(fentry, struct sk_buff *skb)
> >> >   0: (79) r1 = *(u64 *)(r1 +0)
> >> >   invalid bpf_context access off=0 size=8
> >> >
> >> > The problem is that btf_ctx_access gets the context type for the
> >> > traced program, which is in this case the extension.
> >> >
> >> > But when we trace extension program, we want to get the context
> >> > type of the program that the extension is attached to, so we can
> >> > access the argument properly in the trace program.
> >> >
> >> > This version of the patch is tweaked slightly from Jiri's original one,
> >> > since the refactoring in the previous patches means we have to get the
> >> > target prog type from the new variable in prog->aux instead of directly
> >> > from the target prog.
> >> >
> >> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
> >> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> > ---
> >> >  kernel/bpf/btf.c |    9 ++++++++-
> >> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> >> > index 9228af9917a8..55f7b2ba1cbd 100644
> >> > --- a/kernel/bpf/btf.c
> >> > +++ b/kernel/bpf/btf.c
> >> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> >> >
> >> >         info->reg_type = PTR_TO_BTF_ID;
> >> >         if (tgt_prog) {
> >> > -               ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
> >> > +               enum bpf_prog_type tgt_type;
> >> > +
> >> > +               if (tgt_prog->type == BPF_PROG_TYPE_EXT)
> >> > +                       tgt_type = tgt_prog->aux->tgt_prog_type;
> >>
> >> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
> >> this be a loop?
> >
> > ok, never mind this specifically. there is an explicit check
> >
> > if (tgt_prog->type == prog->type) {
> >     verbose(env, "Cannot recursively attach\n");
> >     return -EINVAL;
> > }
> >
> > that will prevent this.
> >
> > But, I think we still will be able to construct a long chain of
> > fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
> > infinitum. Can you please construct such a selftest? And then we
> > should probably fix those checks to also disallow FMOD_RET, in
> > addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
> > prog type should check if that can cause any problems).
>
> Huh, I thought fmod_ret was supposed to be for kernel functions only?

Yeah, I realized that afterwards, but didn't want to ramble on forever :)

> However, I can't really point to anywhere in the code that ensures this,
> other than check_attach_modify_return(), but I think that will allow a
> bpf function as long as its name starts with "security_" ?

I think error_injection_list check will disallow anything that's not a
specially marked kernel function. So we are probably safe as is, even
though a bit implicitly.

>
> Is there actually any use case for modify_return being attached to a BPF
> function (you could just use freplace instead, couldn't you?). Or should
> we just disallow that entirely (if I'm not missing somewhere it's
> already blocked)?

No idea, but I think it works as is right now, so I wouldn't touch it.

>
> -Toke
>

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Sep 17, 2020 at 10:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Wed, Sep 16, 2020 at 12:59 PM Andrii Nakryiko
>> > <andrii.nakryiko@gmail.com> wrote:
>> >>
>> >> On Tue, Sep 15, 2020 at 5:50 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >> >
>> >> > From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> >
>> >> > Eelco reported we can't properly access arguments if the tracing
>> >> > program is attached to extension program.
>> >> >
>> >> > Having following program:
>> >> >
>> >> >   SEC("classifier/test_pkt_md_access")
>> >> >   int test_pkt_md_access(struct __sk_buff *skb)
>> >> >
>> >> > with its extension:
>> >> >
>> >> >   SEC("freplace/test_pkt_md_access")
>> >> >   int test_pkt_md_access_new(struct __sk_buff *skb)
>> >> >
>> >> > and tracing that extension with:
>> >> >
>> >> >   SEC("fentry/test_pkt_md_access_new")
>> >> >   int BPF_PROG(fentry, struct sk_buff *skb)
>> >> >
>> >> > It's not possible to access skb argument in the fentry program,
>> >> > with following error from verifier:
>> >> >
>> >> >   ; int BPF_PROG(fentry, struct sk_buff *skb)
>> >> >   0: (79) r1 = *(u64 *)(r1 +0)
>> >> >   invalid bpf_context access off=0 size=8
>> >> >
>> >> > The problem is that btf_ctx_access gets the context type for the
>> >> > traced program, which is in this case the extension.
>> >> >
>> >> > But when we trace extension program, we want to get the context
>> >> > type of the program that the extension is attached to, so we can
>> >> > access the argument properly in the trace program.
>> >> >
>> >> > This version of the patch is tweaked slightly from Jiri's original one,
>> >> > since the refactoring in the previous patches means we have to get the
>> >> > target prog type from the new variable in prog->aux instead of directly
>> >> > from the target prog.
>> >> >
>> >> > Reported-by: Eelco Chaudron <echaudro@redhat.com>
>> >> > Suggested-by: Jiri Olsa <jolsa@kernel.org>
>> >> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> >> > ---
>> >> >  kernel/bpf/btf.c |    9 ++++++++-
>> >> >  1 file changed, 8 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> >> > index 9228af9917a8..55f7b2ba1cbd 100644
>> >> > --- a/kernel/bpf/btf.c
>> >> > +++ b/kernel/bpf/btf.c
>> >> > @@ -3860,7 +3860,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
>> >> >
>> >> >         info->reg_type = PTR_TO_BTF_ID;
>> >> >         if (tgt_prog) {
>> >> > -               ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>> >> > +               enum bpf_prog_type tgt_type;
>> >> > +
>> >> > +               if (tgt_prog->type == BPF_PROG_TYPE_EXT)
>> >> > +                       tgt_type = tgt_prog->aux->tgt_prog_type;
>> >>
>> >> what if tgt_prog->aux->tgt_prog_type is also BPF_PROG_TYPE_EXT? Should
>> >> this be a loop?
>> >
>> > ok, never mind this specifically. there is an explicit check
>> >
>> > if (tgt_prog->type == prog->type) {
>> >     verbose(env, "Cannot recursively attach\n");
>> >     return -EINVAL;
>> > }
>> >
>> > that will prevent this.
>> >
>> > But, I think we still will be able to construct a long chain of
>> > fmod_ret -> freplace -> fmod_ret -> freplace -> and so on ad
>> > infinitum. Can you please construct such a selftest? And then we
>> > should probably fix those checks to also disallow FMOD_RET, in
>> > addition to BPF_TRACE_FENTRY/FEXIT (and someone more familiar with LSM
>> > prog type should check if that can cause any problems).
>>
>> Huh, I thought fmod_ret was supposed to be for kernel functions only?
>
> Yeah, I realized that afterwards, but didn't want to ramble on forever :)
>
>> However, I can't really point to anywhere in the code that ensures this,
>> other than check_attach_modify_return(), but I think that will allow a
>> bpf function as long as its name starts with "security_" ?
>
> I think error_injection_list check will disallow anything that's not a
> specially marked kernel function. So we are probably safe as is, even
> though a bit implicitly.

Got a selftest working now, and no, it seems not. At least attachment
will succeed if the freplace program has a security_ prefix in its
function name. So will add a new patch to fix that, and the selftest :)

-Toke