Message ID | 160051618391.58048.12525358750568883938.stgit@toke.dk |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: Support multi-attach for freplace programs | expand |
On Sat, Sep 19, 2020 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > From the checks and commit messages for modify_return, it seems it was > never the intention that it should be possible to attach a tracing program > with expected_attach_type == BPF_MODIFY_RETURN to another BPF program. > However, check_attach_modify_return() will only look at the function name, > so if the target function starts with "security_", the attach will be > allowed even for bpf2bpf attachment. > > Fix this oversight by also blocking the modification if a target program is > supplied. > > Fixes: 18644cec714a ("bpf: Fix use-after-free in fmod_ret check") > Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN") > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > kernel/bpf/verifier.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 4161b6c406bc..cb1b0f9fd770 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11442,7 +11442,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) > prog->aux->attach_func_name); > } else if (prog->expected_attach_type == BPF_MODIFY_RETURN) { > ret = check_attach_modify_return(prog, addr); > - if (ret) > + if (ret || tgt_prog) can you please do it as a separate check with a more appropriate and meaningful message? > verbose(env, "%s() is not modifiable\n", > prog->aux->attach_func_name); > } >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Sat, Sep 19, 2020 at 4:50 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> From the checks and commit messages for modify_return, it seems it was >> never the intention that it should be possible to attach a tracing program >> with expected_attach_type == BPF_MODIFY_RETURN to another BPF program. >> However, check_attach_modify_return() will only look at the function name, >> so if the target function starts with "security_", the attach will be >> allowed even for bpf2bpf attachment. >> >> Fix this oversight by also blocking the modification if a target program is >> supplied. >> >> Fixes: 18644cec714a ("bpf: Fix use-after-free in fmod_ret check") >> Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN") >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- >> kernel/bpf/verifier.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index 4161b6c406bc..cb1b0f9fd770 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -11442,7 +11442,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) >> prog->aux->attach_func_name); >> } else if (prog->expected_attach_type == BPF_MODIFY_RETURN) { >> ret = check_attach_modify_return(prog, addr); >> - if (ret) >> + if (ret || tgt_prog) > > can you please do it as a separate check with a more appropriate and > meaningful message? Heh, okay, maybe I was being a bit too lazy here ;) -Toke
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 4161b6c406bc..cb1b0f9fd770 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11442,7 +11442,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) prog->aux->attach_func_name); } else if (prog->expected_attach_type == BPF_MODIFY_RETURN) { ret = check_attach_modify_return(prog, addr); - if (ret) + if (ret || tgt_prog) verbose(env, "%s() is not modifiable\n", prog->aux->attach_func_name); }