netdev - Re: [PATCH bpf-next 1/2] bpf: Allow ld_imm64 instruction to point to kfunc.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAADnVQL62Qr0ChNgOcs0o-pJ+x9HKx8R-TY321XaGqY=TSL2jQ@mail.gmail.com>
Date:   Thu, 16 Mar 2023 09:45:10 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Eduard Zingerman <eddyz87@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        David Vernet <void@...ifault.com>,
        Dave Marchevsky <davemarchevsky@...a.com>,
        Tejun Heo <tj@...nel.org>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/2] bpf: Allow ld_imm64 instruction to point to kfunc.

On Thu, Mar 16, 2023 at 7:14 AM Eduard Zingerman <eddyz87@...il.com> wrote:
>
> On Wed, 2023-03-15 at 15:36 -0700, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@...nel.org>
> >
> > Allow ld_imm64 insn with BPF_PSEUDO_BTF_ID to hold the address of kfunc.
> > PTR_MEM is already recognized for NULL-ness by is_branch_taken(),
> > so unresolved kfuncs will be seen as zero.
> > This allows BPF programs to detect at load time whether kfunc is present
> > in the kernel with bpf_kfunc_exists() macro.
> >
> > Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> > ---
> >  kernel/bpf/verifier.c       | 7 +++++--
> >  tools/lib/bpf/bpf_helpers.h | 3 +++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 60793f793ca6..4e49d34d8cd6 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -15955,8 +15955,8 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> >               goto err_put;
> >       }
> >
> > -     if (!btf_type_is_var(t)) {
> > -             verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR.\n", id);
> > +     if (!btf_type_is_var(t) && !btf_type_is_func(t)) {
> > +             verbose(env, "pseudo btf_id %d in ldimm64 isn't KIND_VAR or KIND_FUNC\n", id);
> >               err = -EINVAL;
> >               goto err_put;
> >       }
> > @@ -15990,6 +15990,9 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
> >               aux->btf_var.reg_type = PTR_TO_BTF_ID | MEM_PERCPU;
> >               aux->btf_var.btf = btf;
> >               aux->btf_var.btf_id = type;
> > +     } else if (!btf_type_is_func(t)) {
> > +             aux->btf_var.reg_type = PTR_TO_MEM | MEM_RDONLY;
> > +             aux->btf_var.mem_size = 0;
>
> This if statement has the following conditions in master:
>
>         if (percpu) {
>         // ...
>         } else if (!btf_type_is_struct(t)) {
>         // ...
>         } else {
>         // ...
>         }
>
> Conditions `!btf_type_is_func()` and `!btf_type_is_struct()` are
> not mutually exclusive, thus adding `if (!btf_type_is_func())`
> would match certain conditions that were previously matched by struct
> case, wouldn't it? E.g. if type is `BTF_KIND_INT`?

ohh. good catch!

> Although, I was not able to trigger it, as it seems that pahole only
> encodes per-cpu vars in BTF.

right. that's why we don't have selftests for this code
that could have caught my braino.

There were patches to add vars to vmlinux btf, but it didn't materialize yet.