linux-kernel - Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace program with write access

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] [day] [month] [year] [list]

Message-ID: <F03D5838-0E4D-4B55-B8CA-42FABEA09A05@fb.com>
Date:   Thu, 27 Aug 2020 19:41:59 +0000
From:   Udip Pant <udippant@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     Alexei Starovoitov <ast@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        "David S . Miller" <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next v3 2/4] selftests/bpf: add test for freplace
 program with write access



On 8/26/20, 11:05 PM, "Andrii Nakryiko" <andrii.nakryiko@...il.com> wrote:

On Tue, Aug 25, 2020 at 4:21 PM Udip Pant <udippant@...com> wrote:
>>
>> This adds a selftest that tests the behavior when a freplace target program
>> attempts to make a write access on a packet. The expectation is that the read or write
>> access is granted based on the program type of the linked program and
>> not itself (which is of type, for e.g., BPF_PROG_TYPE_EXT).
>>
>> This test fails without the associated patch on the verifier.
>>
>> Signed-off-by: Udip Pant <udippant@...com>
>> ---
>>  .../selftests/bpf/prog_tests/fexit_bpf2bpf.c  |  1 +
>>  .../selftests/bpf/progs/fexit_bpf2bpf.c       | 27 +++++++++++++++++++
>>  .../selftests/bpf/progs/test_pkt_access.c     | 20 ++++++++++++++
>>  3 files changed, 48 insertions(+)
>>
>
>[...]
>
>> +__attribute__ ((noinline))
>> +int test_pkt_write_access_subprog(struct __sk_buff *skb, __u32 off)
>> +{
>> +       void *data = (void *)(long)skb->data;
>> +       void *data_end = (void *)(long)skb->data_end;
>> +       struct tcphdr *tcp = NULL;
>> +
>> +       if (off > sizeof(struct ethhdr) + sizeof(struct ipv6hdr))
>> +               return -1;
>> +
>> +       tcp = data + off;
>> +       if (tcp + 1 > data_end)
>> +               return -1;
>> +       /* make modification to the packet data */
>> +       tcp->check++;
>
> Just FYI for all BPF contributors. This change makes test_pkt_access
> BPF program to fail on kernel 5.5, which (the kernel) we use as part
> libbpf CI testing. test_pkt_access.o in turn makes few different
> selftests (see [0] for details) to fail on 5.5 (because
> test_pkt_access is used as one of BPF objects loaded as part of those
> selftests). This is ok, I'm blacklisting (at least temporarily) those
> tests, but I wanted to bring up this issue, as it did happen before
> and will keep happening in the future and will constantly decrease
> test coverage for older kernels that libbpf CI performs.
>
> I propose that when we introduce new features (like new fields in a
> BPF program's context or something along those lines) and want to test
> them, we should lean towards creating new tests, not modify existing
> ones. This will allow all already working selftests to keep working
> for older kernels. Does this sound reasonable as an approach?
>
> As for this particular breakage, I'd appreciate someone taking a look
> at the problem and checking if it's some new feature that's not
> present in 5.5 or just Clang/verifier interactions (32-bit pointer
> arithmetic, is this a new issue?). If it's something fixable, it would
> be nice to fix and restore 5.5 tests. Thanks!
>
>   [0] https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.com_github_libbpf_libbpf_jobs_378226438&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=EICQWIMWWJPbefL5Ad4oTQ&m=ezWtkpxlrj19nFAuBX5LswTLCVR3zCtVY7MBlIsm7bA&s=zzYFn7QWYsp3BDOxYP95S4yKr2kqndGw1w6zHoNaRdU&e= 
>
> Verifier complains about:
>
> ; if (test_pkt_write_access_subprog(skb, (void *)tcp - data))

Not sure about this specific issue with 32-bit pointer arithmetic. But I can try a workaround to fix this test by passing only the skb (and deriving the tcp-header inside the method).