netdev - Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct

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:   Wed, 10 Feb 2021 12:26:20 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf 1/2] libbpf: Ignore non function pointer member in struct_ops

On Tue, Feb 9, 2021 at 12:40 PM Martin KaFai Lau <kafai@...com> wrote:
>
> When libbpf initializes the kernel's struct_ops in
> "bpf_map__init_kern_struct_ops()", it enforces all
> pointer types must be a function pointer and rejects
> others.  It turns out to be too strict.  For example,
> when directly using "struct tcp_congestion_ops" from vmlinux.h,
> it has a "struct module *owner" member and it is set to NULL
> in a bpf_tcp_cc.o.
>
> Instead, it only needs to ensure the member is a function
> pointer if it has been set (relocated) to a bpf-prog.
> This patch moves the "btf_is_func_proto(kern_mtype)" check
> after the existing "if (!prog) { continue; }".
>
> The "btf_is_func_proto(mtype)" has already been guaranteed
> in "bpf_object__collect_st_ops_relos()" which has been run
> before "bpf_map__init_kern_struct_ops()".  Thus, this check
> is removed.
>
> Fixes: 590a00888250 ("bpf: libbpf: Add STRUCT_OPS support")
> Signed-off-by: Martin KaFai Lau <kafai@...com>
> ---

Looks good, see nit below.

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

>  tools/lib/bpf/libbpf.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6ae748f6ea11..b483608ea72a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -887,12 +887,6 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>                         kern_mtype = skip_mods_and_typedefs(kern_btf,
>                                                             kern_mtype->type,
>                                                             &kern_mtype_id);
> -                       if (!btf_is_func_proto(mtype) ||
> -                           !btf_is_func_proto(kern_mtype)) {
> -                               pr_warn("struct_ops init_kern %s: non func ptr %s is not supported\n",
> -                                       map->name, mname);
> -                               return -ENOTSUP;
> -                       }
>
>                         prog = st_ops->progs[i];
>                         if (!prog) {

debug message below this line is a bit misleading, it talks about
"func ptr is not set", but it actually could be any kind of field. So
it would be nice to just talk about "members" or "fields" there, no?

> @@ -901,6 +895,12 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>                                 continue;
>                         }
>
> +                       if (!btf_is_func_proto(kern_mtype)) {
> +                               pr_warn("struct_ops init_kern %s: kernel member %s is not a func ptr\n",
> +                                       map->name, mname);
> +                               return -ENOTSUP;
> +                       }
> +
>                         prog->attach_btf_id = kern_type_id;
>                         prog->expected_attach_type = kern_member_idx;
>
> --
> 2.24.1
>