netdev - Re: [PATCH iproute2] bpf: initialise map symbol before retrieving and comparing its type

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: <CAH3MdRW-8S-w1rR-7WYC95G5==FLiKAZKAT1vHRo6ghtRJN3UQ@mail.gmail.com>
Date:   Mon, 19 Nov 2018 17:38:48 -0800
From:   Y Song <ys114321@...il.com>
To:     Quentin Monnet <quentin.monnet@...ronome.com>
Cc:     David Ahern <dsahern@...il.com>, stephen@...workplumber.org,
        Yonghong Song <yhs@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        netdev <netdev@...r.kernel.org>, oss-drivers@...ronome.com
Subject: Re: [PATCH iproute2] bpf: initialise map symbol before retrieving and
 comparing its type

On Mon, Nov 19, 2018 at 5:28 PM Quentin Monnet
<quentin.monnet@...ronome.com> wrote:
>
> In order to compare BPF map symbol type correctly in regard to the
> latest LLVM, commit 7a04dd84a7f9 ("bpf: check map symbol type properly
> with newer llvm compiler") compares map symbol type to both NOTYPE and
> OBJECT. To do so, it first retrieves the type from "sym.st_info" and
> stores it into a temporary variable.
>
> However, the type is collected from the symbol "sym" before this latter
> symbol is actually updated. gelf_getsym() is called after that and
> updates "sym", and when comparison with OBJECT or NOTYPE happens it is
> done on the type of the symbol collected in the previous passage of the
> loop (or on an uninitialised symbol on the first passage). This may
> eventually break map collection from the ELF file.
>
> Fix this by assigning the type to the temporary variable only after the
> call to gelf_getsym().
>
> Fixes: 7a04dd84a7f9 ("bpf: check map symbol type properly with newer llvm compiler")
> Reported-by: Ron Philip <ron.philip@...ronome.com>
> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
> Reviewed-by: Jiong Wang <jiong.wang@...ronome.com>

Thanks for the fix!
Acked-by: Yonghong Song <yhs@...com>

> ---
>  lib/bpf.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 45f279fa4a41..6aff8f7bad7f 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1758,11 +1758,12 @@ static const char *bpf_map_fetch_name(struct bpf_elf_ctx *ctx, int which)
>         int i;
>
>         for (i = 0; i < ctx->sym_num; i++) {
> -               int type = GELF_ST_TYPE(sym.st_info);
> +               int type;
>
>                 if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
>                         continue;
>
> +               type = GELF_ST_TYPE(sym.st_info);
>                 if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
>                     (type != STT_NOTYPE && type != STT_OBJECT) ||
>                     sym.st_shndx != ctx->sec_maps ||
> @@ -1851,11 +1852,12 @@ static int bpf_map_num_sym(struct bpf_elf_ctx *ctx)
>         GElf_Sym sym;
>
>         for (i = 0; i < ctx->sym_num; i++) {
> -               int type = GELF_ST_TYPE(sym.st_info);
> +               int type;
>
>                 if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
>                         continue;
>
> +               type = GELF_ST_TYPE(sym.st_info);
>                 if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
>                     (type != STT_NOTYPE && type != STT_OBJECT) ||
>                     sym.st_shndx != ctx->sec_maps)
> @@ -1931,10 +1933,12 @@ static int bpf_map_verify_all_offs(struct bpf_elf_ctx *ctx, int end)
>                  * the table again.
>                  */
>                 for (i = 0; i < ctx->sym_num; i++) {
> -                       int type = GELF_ST_TYPE(sym.st_info);
> +                       int type;
>
>                         if (gelf_getsym(ctx->sym_tab, i, &sym) != &sym)
>                                 continue;
> +
> +                       type = GELF_ST_TYPE(sym.st_info);
>                         if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL ||
>                             (type != STT_NOTYPE && type != STT_OBJECT) ||
>                             sym.st_shndx != ctx->sec_maps)
> --
> 2.7.4
>