netdev - Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object

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, 15 Jan 2020 05:46:00 +0000
From:   Martin Lau <kafai@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        "Daniel Borkmann" <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        "Kernel Team" <Kernel-team@...com>,
        Networking <netdev@...r.kernel.org>,
        Paul Chaignon <paul.chaignon@...nge.com>
Subject: Re: [PATCH bpf-next 1/5] bpftool: Fix a leak of btf object

On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@...com> wrote:
> >
> > When testing a map has btf or not, maps_have_btf() tests it by actually
> > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> > forgot to btf__free() it.
> >
> > In maps_have_btf() stage, there is no need to test it by really
> > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> > info.btf_id is good enough.
> >
> > Also, the err_close case is unnecessary, and also causes double
> > close() because the calling func do_dump() will close() all fds again.
> >
> > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > Cc: Paul Chaignon <paul.chaignon@...nge.com>
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > ---
> 
> this is clearly a simplification, but isn't do_dump still buggy? see below
> 
> >  tools/bpf/bpftool/map.c | 16 ++--------------
> >  1 file changed, 2 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index c01f76fa6876..e00e9e19d6b7 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
> >  {
> >         struct bpf_map_info info = {};
> >         __u32 len = sizeof(info);
> > -       struct btf *btf = NULL;
> >         int err, i;
> >
> >         for (i = 0; i < nb_fds; i++) {
> >                 err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> >                 if (err) {
> >                         p_err("can't get map info: %s", strerror(errno));
> > -                       goto err_close;
> > -               }
> > -
> > -               err = btf__get_from_id(info.btf_id, &btf);
> > -               if (err) {
> > -                       p_err("failed to get btf");
> > -                       goto err_close;
> > +                       return -1;
> >                 }
> >
> > -               if (!btf)
> > +               if (!info.btf_id)
> >                         return 0;
> 
> if info.btf_id is non-zero, shouldn't we immediately return 1 and be
> done with it?
No.  maps_have_btf() returns 1 only if all the maps have btf.

> 
> I'm also worried about do_dump logic. What's the behavior when some
> maps do have BTF and some don't? Should we use btf_writer for all,
> some or none maps for that case?
For plain_text, btf output is either for all or for none.
It is the intention of the "Fixes" patch if I read it correctly,
and it is kept as is in this bug fix.
It will become clear by doing a plain text dump on maps with and
without btf.  They are very different.

Can the output format for with and without BTF somehow merged for
plain text?  May be if it is still common to have no-BTF map
going forward but how this may look like will need another
discussion.

> I'd expect we'd use BTF info for
> those maps that have BTF and fall back to raw output for those that
> don't, but I'm not sure that how code behaves right now.
The json_output is doing what you described, print BTF info
whenever available.

> 
> Maybe Paul can clarify...
> 
> 
> >         }
> >
> >         return 1;
> > -
> > -err_close:
> > -       for (; i < nb_fds; i++)
> > -               close(fds[i]);
> > -       return -1;
> >  }
> >
> >  static int
> > --
> > 2.17.1
> >