netdev - Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor check_attach_btf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Sep 2020 18:54:12 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        KP Singh <kpsingh@...omium.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v5 2/8] bpf: verifier: refactor
 check_attach_btf_id()

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

> On Thu, Sep 17, 2020 at 3:06 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@...il.com> writes:
>>
>> >>
>> >> +int bpf_check_attach_target(struct bpf_verifier_log *log,
>> >> +                           const struct bpf_prog *prog,
>> >> +                           const struct bpf_prog *tgt_prog,
>> >> +                           u32 btf_id,
>> >> +                           struct btf_func_model *fmodel,
>> >> +                           long *tgt_addr,
>> >> +                           const char **tgt_name,
>> >> +                           const struct btf_type **tgt_type);
>> >
>> > So this is obviously an abomination of a function signature,
>> > especially for a one exported to other files.
>> >
>> > One candidate to remove would be tgt_type, which is supposed to be a
>> > derivative of target BTF (vmlinux or tgt_prog->btf) + btf_id,
>> > **except** (and that's how I found the bug below), in case of
>> > fentry/fexit programs attaching to "conservative" BPF functions, in
>> > which case what's stored in aux->attach_func_proto is different from
>> > what is passed into btf_distill_func_proto. So that's a bug already
>> > (you'll return NULL in some cases for tgt_type, while it has to always
>> > be non-NULL).
>>
>> Okay, looked at this in more detail, and I don't think the refactored
>> code is doing anything different from the pre-refactor version?
>>
>> Before we had this:
>>
>>                 if (tgt_prog && conservative) {
>>                         prog->aux->attach_func_proto = NULL;
>>                         t = NULL;
>>                 }
>>
>> and now we just have
>>
>>                 if (tgt_prog && conservative)
>>                         t = NULL;
>>
>> in bpf_check_attach_target(), which gets returned as tgt_type and
>> subsequently assigned to prog->aux->attach_func_proto.
>
> Yeah, you are totally right, I don't know how I missed that
> `prog->aux->attach_func_proto = NULL;`, sorry about that.

No worries - this was certainly not the easiest to review; thanks for
sticking with it! :)

[..]

> Please add my ack when you post a new version:
>
> Acked-by: Andrii Nakryiko <andriin@...com>

Will do, thanks!

-Toke