Re: [PATCH bpf-next 2/2] selftests/bpf: add verifier tests for wide stores

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jun 29, 2019 at 11:02 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 6/28/19 4:10 PM, Stanislav Fomichev wrote:
> > Make sure that wide stores are allowed at proper (aligned) addresses.
> > Note that user_ip6 is naturally aligned on 8-byte boundary, so
> > correct addresses are user_ip6[0] and user_ip6[2]. msg_src_ip6 is,
> > however, aligned on a 4-byte bondary, so only msg_src_ip6[1]
> > can be wide-stored.
> >
> > Cc: Andrii Nakryiko <andriin@xxxxxx>
> > Cc: Yonghong Song <yhs@xxxxxx>
> > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > ---
> >   tools/testing/selftests/bpf/test_verifier.c   | 17 ++++++--
> >   .../selftests/bpf/verifier/wide_store.c       | 40 +++++++++++++++++++
> >   2 files changed, 54 insertions(+), 3 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/verifier/wide_store.c
> >
> > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> > index c5514daf8865..b0773291012a 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -105,6 +105,7 @@ struct bpf_test {
> >                       __u64 data64[TEST_DATA_LEN / 8];
> >               };
> >       } retvals[MAX_TEST_RUNS];
> > +     enum bpf_attach_type expected_attach_type;
> >   };
> >
> >   /* Note we want this to be 64 bit aligned so that the end of our array is
> > @@ -850,6 +851,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >       int fd_prog, expected_ret, alignment_prevented_execution;
> >       int prog_len, prog_type = test->prog_type;
> >       struct bpf_insn *prog = test->insns;
> > +     struct bpf_load_program_attr attr;
> >       int run_errs, run_successes;
> >       int map_fds[MAX_NR_MAPS];
> >       const char *expected_err;
> > @@ -881,8 +883,17 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >               pflags |= BPF_F_STRICT_ALIGNMENT;
> >       if (test->flags & F_NEEDS_EFFICIENT_UNALIGNED_ACCESS)
> >               pflags |= BPF_F_ANY_ALIGNMENT;
> > -     fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
> > -                                  "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
> > +
> > +     memset(&attr, 0, sizeof(attr));
> > +     attr.prog_type = prog_type;
> > +     attr.expected_attach_type = test->expected_attach_type;
> > +     attr.insns = prog;
> > +     attr.insns_cnt = prog_len;
> > +     attr.license = "GPL";
> > +     attr.log_level = 4;
> > +     attr.prog_flags = pflags;
> > +
> > +     fd_prog = bpf_load_program_xattr(&attr, bpf_vlog, sizeof(bpf_vlog));
> >       if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> >               printf("SKIP (unsupported program type %d)\n", prog_type);
> >               skips++;
> > @@ -912,7 +923,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> >                       printf("FAIL\nUnexpected success to load!\n");
> >                       goto fail_log;
> >               }
> > -             if (!strstr(bpf_vlog, expected_err)) {
> > +             if (!expected_err || !strstr(bpf_vlog, expected_err)) {
> >                       printf("FAIL\nUnexpected error message!\n\tEXP: %s\n\tRES: %s\n",
> >                             expected_err, bpf_vlog);
> >                       goto fail_log;
> > diff --git a/tools/testing/selftests/bpf/verifier/wide_store.c b/tools/testing/selftests/bpf/verifier/wide_store.c
> > new file mode 100644
> > index 000000000000..c6385f45b114
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/verifier/wide_store.c
> > @@ -0,0 +1,40 @@
> > +#define BPF_SOCK_ADDR(field, off, res, err) \
> > +{ \
> > +     "wide store to bpf_sock_addr." #field "[" #off "]", \
> > +     .insns = { \
> > +     BPF_MOV64_IMM(BPF_REG_0, 1), \
> > +     BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_0, \
> > +                 offsetof(struct bpf_sock_addr, field[off])), \
> > +     BPF_EXIT_INSN(), \
> > +     }, \
> > +     .result = res, \
> > +     .prog_type = BPF_PROG_TYPE_CGROUP_SOCK_ADDR, \
> > +     .expected_attach_type = BPF_CGROUP_UDP6_SENDMSG, \
> > +     .errstr = err, \
> > +}
> > +
> > +/* user_ip6[0] is u64 aligned */
> > +BPF_SOCK_ADDR(user_ip6, 0, ACCEPT,
> > +           NULL),
> > +BPF_SOCK_ADDR(user_ip6, 1, REJECT,
> > +           "invalid bpf_context access off=12 size=8"),
> > +BPF_SOCK_ADDR(user_ip6, 2, ACCEPT,
> > +           NULL),
> > +BPF_SOCK_ADDR(user_ip6, 3, REJECT,
> > +           "invalid bpf_context access off=20 size=8"),
> > +BPF_SOCK_ADDR(user_ip6, 4, REJECT,
> > +           "invalid bpf_context access off=24 size=8"),
>
> With offset 4, we have
> #968/p wide store to bpf_sock_addr.user_ip6[4] OK
>
> This test case can be removed. user code typically
> won't write bpf_sock_addr.user_ip6[4], and compiler
> typically will give a warning since it is out of
> array bound. Any particular reason you want to
> include this one?

I agree, user_ip6[4] is essentially 8-byte write to user_port field.

>
>
> > +
> > +/* msg_src_ip6[0] is _not_ u64 aligned */
> > +BPF_SOCK_ADDR(msg_src_ip6, 0, REJECT,
> > +           "invalid bpf_context access off=44 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 1, ACCEPT,
> > +           NULL),
> > +BPF_SOCK_ADDR(msg_src_ip6, 2, REJECT,
> > +           "invalid bpf_context access off=52 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 3, REJECT,
> > +           "invalid bpf_context access off=56 size=8"),
> > +BPF_SOCK_ADDR(msg_src_ip6, 4, REJECT,
> > +           "invalid bpf_context access off=60 size=8"),
>
> The same as above, offset=4 case can be removed?

And this one is a write into a struct hole, which should be rejected
even without wide-store check, right?

>
> > +
> > +#undef BPF_SOCK_ADDR
> >



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux