netdev - Re: [PATCH bpf] tools: bpftool: fix crash with un-owned prog arrays

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: <CAPhsuW5r4uqhEib81G+ojaeXSdNg1-baP_u1LcCqpZ5MSs08uQ@mail.gmail.com>
Date:   Mon, 28 Jan 2019 12:42:15 -0800
From:   Song Liu <liu.song.a23@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        oss-drivers@...ronome.com, Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] tools: bpftool: fix crash with un-owned prog arrays

On Mon, Jan 28, 2019 at 10:06 AM Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>
> Prog arrays don't have 'owner_prog_type' and 'owner_jited'
> fields in their fdinfo when they are created.  Those fields
> are set and reported when first program is checked for
> compatibility by bpf_prog_array_compatible().
>
> This means that bpftool cannot expect the fields to always
> be there.  Currently trying to show maps on a system with
> an un-owned prog array leads to a crash:
>
> $ bpftool map show
> 389: prog_array  name tail_call_map  flags 0x0
> Error: key 'owner_prog_type' not found in fdinfo
> Error: key 'owner_jited' not found in fdinfo
>        key 4B  value 4B  max_entries 4  memlock 4096B
>        Segmentation fault (core dumped)
>
> We pass a NULL pointer to atoi().
>
> Remove the assumption that fdinfo keys are always present.
> Add missing validations and remove the p_err() calls which
> may lead to broken JSON output as caller will not propagate
> the failure.
>
> Fixes: 99a44bef5870 ("tools: bpftool: add owner_prog_type and owner_jited to bpftool output")
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>

Acked-by: Song Liu <songliubraving@...com>

> ---
>  tools/bpf/bpftool/common.c |  6 +-----
>  tools/bpf/bpftool/map.c    | 17 ++++++++---------
>  2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 897483457bf0..f7261fad45c1 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -297,10 +297,8 @@ char *get_fdinfo(int fd, const char *key)
>         snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", fd);
>
>         fdi = fopen(path, "r");
> -       if (!fdi) {
> -               p_err("can't open fdinfo: %s", strerror(errno));
> +       if (!fdi)
>                 return NULL;
> -       }
>
>         while ((n = getline(&line, &line_n, fdi)) > 0) {
>                 char *value;
> @@ -313,7 +311,6 @@ char *get_fdinfo(int fd, const char *key)
>
>                 value = strchr(line, '\t');
>                 if (!value || !value[1]) {
> -                       p_err("malformed fdinfo!?");
>                         free(line);
>                         return NULL;
>                 }
> @@ -326,7 +323,6 @@ char *get_fdinfo(int fd, const char *key)
>                 return line;
>         }
>
> -       p_err("key '%s' not found in fdinfo", key);
>         free(line);
>         fclose(fdi);
>         return NULL;
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 29a3468c6cf6..1ef1ee2280a2 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -513,10 +513,9 @@ static int show_map_close_json(int fd, struct bpf_map_info *info)
>                                 jsonw_uint_field(json_wtr, "owner_prog_type",
>                                                  prog_type);
>                 }
> -               if (atoi(owner_jited))
> -                       jsonw_bool_field(json_wtr, "owner_jited", true);
> -               else
> -                       jsonw_bool_field(json_wtr, "owner_jited", false);
> +               if (owner_jited)
> +                       jsonw_bool_field(json_wtr, "owner_jited",
> +                                        !!atoi(owner_jited));
>
>                 free(owner_prog_type);
>                 free(owner_jited);
> @@ -569,7 +568,8 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
>                 char *owner_prog_type = get_fdinfo(fd, "owner_prog_type");
>                 char *owner_jited = get_fdinfo(fd, "owner_jited");
>
> -               printf("\n\t");
> +               if (owner_prog_type || owner_jited)
> +                       printf("\n\t");
>                 if (owner_prog_type) {
>                         unsigned int prog_type = atoi(owner_prog_type);
>
> @@ -579,10 +579,9 @@ static int show_map_close_plain(int fd, struct bpf_map_info *info)
>                         else
>                                 printf("owner_prog_type %d  ", prog_type);
>                 }
> -               if (atoi(owner_jited))
> -                       printf("owner jited");
> -               else
> -                       printf("owner not jited");
> +               if (owner_jited)
> +                       printf("owner%s jited",
> +                              atoi(owner_jited) ? "" : " not");
>
>                 free(owner_prog_type);
>                 free(owner_jited);
> --
> 2.19.2
>