netdev - Re: [PATCH bpf-next 1/5] selftests/bpf: fix btf

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:   Thu, 28 Feb 2019 11:19:12 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, Kernel Team <kernel-team@...com>,
        Alexei Starovoitov <ast@...com>,
        Networking <netdev@...r.kernel.org>, bpf@...r.kernel.org,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next 1/5] selftests/bpf: fix btf_dedup testing code

On Thu, Feb 28, 2019 at 10:52 AM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.com> wrote:
>
> Em Wed, Feb 27, 2019 at 02:46:37PM -0800, Andrii Nakryiko escreveu:
> > btf_dedup testing code doesn't account for length of struct btf_header
> > when calculating the start of a string section. This patch fixes this
> > problem.
> >
> > Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API")
>
> I think this clarifies things, but a Fixes seems excessive, right? I.e.
> if you missed it in both sides of the (a != b) expression, the test will
> be just as valid.
>
> I say this because Fixes tags are now tracked and generates backporting
> efforts that sometimes end up causing unnecessary human exchanges when
> the patches don't apply because of some other patch.o

I think this is a legitimate fix. The bug wasn't exposed until I added
a new test from this patchset, but still. See below.

>
> Or is there some further use of 'test_strs' and 'expect_strs' further
> down that do_test_dedup() function?

Yes, few lines further down we use those pointers to iterate over all
strings in BTF string section and compare them:

        test_str_cur = test_strs;
        test_str_end = test_strs + test_hdr->str_len;
        expect_str_cur = expect_strs;
        expect_str_end = expect_strs + expect_hdr->str_len;
        while (test_str_cur < test_str_end && expect_str_cur < expect_str_end) {
                ... compare strings ...
        }

With this bug, what ended up happening was that we never tested last
sizeof(struct btf_header) bytes of string section. It worked fine for
existing tests by luck, but in general it would fail, because after
that loop we check that we finished iteration exactly where we should:

        if (CHECK(test_str_cur != test_str_end,
                  "test_str_cur:%p != test_str_end:%p",
                  test_str_cur, test_str_end)) {
                err = -1;
                goto done;
        }

New test I added didn't have lucky byte arrangement that made
test_str_cur == test_str_end true for previous tests.


>
> /me scratches head, probably missing something...
>
> - Arnaldo
>
> > Signed-off-by: Andrii Nakryiko <andriin@...com>
> > ---
> >  tools/testing/selftests/bpf/.gitignore | 1 +
> >  tools/testing/selftests/bpf/test_btf.c | 4 ++--
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> > index e47168d1257d..3b74d23fffab 100644
> > --- a/tools/testing/selftests/bpf/.gitignore
> > +++ b/tools/testing/selftests/bpf/.gitignore
> > @@ -14,6 +14,7 @@ feature
> >  test_libbpf_open
> >  test_sock
> >  test_sock_addr
> > +test_sock_fields
> >  urandom_read
> >  test_btf
> >  test_sockmap
> > diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
> > index 02d314383a9c..1426c0a905c8 100644
> > --- a/tools/testing/selftests/bpf/test_btf.c
> > +++ b/tools/testing/selftests/bpf/test_btf.c
> > @@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num)
> >       }
> >
> >       test_hdr = test_btf_data;
> > -     test_strs = test_btf_data + test_hdr->str_off;
> > +     test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off;
> >       expect_hdr = expect_btf_data;
> > -     expect_strs = expect_btf_data + expect_hdr->str_off;
> > +     expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off;
> >       if (CHECK(test_hdr->str_len != expect_hdr->str_len,
> >                 "test_hdr->str_len:%u != expect_hdr->str_len:%u",
> >                 test_hdr->str_len, expect_hdr->str_len)) {
> > --
> > 2.17.1
>
> --
>
> - Arnaldo