linux-kernel - Re: [PATCH ipsec-next v3 5/9] libbpf: selftests: Add verifier tests for CO-RE bitfield writes

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: <CAEf4Bzaz+_y=kxBpPmwYsvzaHypmL=ZBfOK12vLom04DRDWyPg@mail.gmail.com>
Date:   Fri, 1 Dec 2023 15:52:25 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Xu <dxu@...uu.xyz>
Cc:     ast@...nel.org, daniel@...earbox.net, shuah@...nel.org,
        andrii@...nel.org, steffen.klassert@...unet.com,
        antony.antony@...unet.com, alexei.starovoitov@...il.com,
        yonghong.song@...ux.dev, eddyz87@...il.com, mykolal@...com,
        martin.lau@...ux.dev, song@...nel.org, john.fastabend@...il.com,
        kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
        devel@...ux-ipsec.org, netdev@...r.kernel.org
Subject: Re: [PATCH ipsec-next v3 5/9] libbpf: selftests: Add verifier tests
 for CO-RE bitfield writes

On Fri, Dec 1, 2023 at 12:24 PM Daniel Xu <dxu@...uu.xyz> wrote:
>
> Add some tests that exercise BPF_CORE_WRITE_BITFIELD() macro. Since some
> non-trivial bit fiddling is going on, make sure various edge cases (such
> as adjacent bitfields and bitfields at the edge of structs) are
> exercised.
>
> Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> ---
>  .../selftests/bpf/prog_tests/verifier.c       |   2 +
>  .../bpf/progs/verifier_bitfield_write.c       | 100 ++++++++++++++++++
>  2 files changed, 102 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/verifier_bitfield_write.c
>

LGTM, but I'm not sure why we need all those __failure_unpriv, see
below. Regardless:

Acked-by: Andrii Nakryiko <andrii@...nel.org>

> diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
> index 5cfa7a6316b6..67b4948865a3 100644
> --- a/tools/testing/selftests/bpf/prog_tests/verifier.c
> +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
> @@ -6,6 +6,7 @@
>  #include "verifier_and.skel.h"
>  #include "verifier_array_access.skel.h"
>  #include "verifier_basic_stack.skel.h"
> +#include "verifier_bitfield_write.skel.h"
>  #include "verifier_bounds.skel.h"
>  #include "verifier_bounds_deduction.skel.h"
>  #include "verifier_bounds_deduction_non_const.skel.h"
> @@ -115,6 +116,7 @@ static void run_tests_aux(const char *skel_name,
>
>  void test_verifier_and(void)                  { RUN(verifier_and); }
>  void test_verifier_basic_stack(void)          { RUN(verifier_basic_stack); }
> +void test_verifier_bitfield_write(void)       { RUN(verifier_bitfield_write); }
>  void test_verifier_bounds(void)               { RUN(verifier_bounds); }
>  void test_verifier_bounds_deduction(void)     { RUN(verifier_bounds_deduction); }
>  void test_verifier_bounds_deduction_non_const(void)     { RUN(verifier_bounds_deduction_non_const); }
> diff --git a/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c
> new file mode 100644
> index 000000000000..8fe355a19ba6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_bitfield_write.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <stdint.h>
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_core_read.h>
> +
> +#include "bpf_misc.h"
> +
> +struct core_reloc_bitfields {
> +       /* unsigned bitfields */
> +       uint8_t         ub1: 1;
> +       uint8_t         ub2: 2;
> +       uint32_t        ub7: 7;
> +       /* signed bitfields */
> +       int8_t          sb4: 4;
> +       int32_t         sb20: 20;
> +       /* non-bitfields */
> +       uint32_t        u32;
> +       int32_t         s32;
> +} __attribute__((preserve_access_index));
> +
> +SEC("tc")
> +__description("single CO-RE bitfield roundtrip")
> +__btf_path("btf__core_reloc_bitfields.bpf.o")
> +__success __failure_unpriv

do we want __failure_unpriv at all? Is this failure related to
*bitfield* logic at all?

> +__retval(3)
> +int single_field_roundtrip(struct __sk_buff *ctx)
> +{
> +       struct core_reloc_bitfields bitfields;
> +
> +       __builtin_memset(&bitfields, 0, sizeof(bitfields));
> +       BPF_CORE_WRITE_BITFIELD(&bitfields, ub2, 3);
> +       return BPF_CORE_READ_BITFIELD(&bitfields, ub2);
> +}
> +

[...]