netdev - Re: [PATCH bpf-next v4 07/11] libbpf: Resolve invalid weak kfunc calls with imm = 0, off = 0

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]

Message-ID: <CAEf4BzbQ47A9KNbKG6CEXuiubnHL+hsjqk=pD6J1j7_yL=hPbQ@mail.gmail.com>
Date:   Tue, 21 Sep 2021 15:47:09 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v4 07/11] libbpf: Resolve invalid weak kfunc
 calls with imm = 0, off = 0

On Mon, Sep 20, 2021 at 7:15 AM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> Preserve these calls as it allows verifier to succeed in loading the
> program if they are determined to be unreachable after dead code
> elimination during program load. If not, the verifier will fail at
> runtime. This is done for ext->is_weak symbols similar to the case for
> variable ksyms.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> ---

Looks good with few nits below

Acked-by: Andrii Nakryiko <andrii@...nel.org>

>  tools/lib/bpf/libbpf.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3049dfc6088e..3c195eaadf56 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3413,11 +3413,6 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>                                 return -ENOTSUP;
>                         }
>                 } else if (strcmp(sec_name, KSYMS_SEC) == 0) {
> -                       if (btf_is_func(t) && ext->is_weak) {
> -                               pr_warn("extern weak function %s is unsupported\n",
> -                                       ext->name);
> -                               return -ENOTSUP;
> -                       }
>                         ksym_sec = sec;
>                         ext->type = EXT_KSYM;
>                         skip_mods_and_typedefs(obj->btf, t->type,
> @@ -5366,8 +5361,12 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
>                 case RELO_EXTERN_FUNC:
>                         ext = &obj->externs[relo->sym_off];
>                         insn[0].src_reg = BPF_PSEUDO_KFUNC_CALL;
> -                       insn[0].imm = ext->ksym.kernel_btf_id;
> -                       insn[0].off = ext->ksym.offset;
> +                       if (ext->is_set) {
> +                               insn[0].imm = ext->ksym.kernel_btf_id;
> +                               insn[0].off = ext->ksym.offset;
> +                       } else { /* unresolved weak kfunc */
> +                               insn[0].imm = insn[0].off = 0;

it's a bit too easy to miss this, please write as two separate statements

> +                       }
>                         break;
>                 case RELO_SUBPROG_ADDR:
>                         if (insn[0].src_reg != BPF_PSEUDO_FUNC) {
> @@ -6768,8 +6767,10 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>
>         kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC,
>                                     &kern_btf, &kern_btf_fd);
> -       if (kfunc_id < 0) {
> -               pr_warn("extern (func ksym) '%s': not found in kernel BTF\n",
> +       if (kfunc_id == -ESRCH && ext->is_weak) {
> +               return 0;
> +       } else if (kfunc_id < 0) {

nit: there is return above, no need for "else if", please drop that
part. It would be nice if you could clean this up in
bpf_object__resolve_ksym_var_btf_id() as well. Thanks!

> +               pr_warn("extern (func ksym) '%s': not found in kernel or module BTFs\n",
>                         ext->name);
>                 return kfunc_id;
>         }
> --
> 2.33.0
>