On Mon, Dec 20, 2021 at 8:28 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 12/20/21 12:12 PM, Hao Luo wrote:
> > The second parameter of bpf_d_path() can only accept writable
> > memories. rdonly_mem obtained from bpf_per_cpu_ptr() can not
> > be passed into bpf_d_path for modification. This patch adds
> > a selftest to verify this behavior.
> >
> > Signed-off-by: Hao Luo <haoluo@xxxxxxxxxx>
> > ---
> > .../testing/selftests/bpf/prog_tests/d_path.c | 22 +++++++++++++-
> > .../bpf/progs/test_d_path_check_rdonly_mem.c | 30 +++++++++++++++++++
> > 2 files changed, 51 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > index 0a577a248d34..f8d8c5a5dfba 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/d_path.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
> > @@ -9,6 +9,7 @@
> > #define MAX_FILES 7
> >
> > #include "test_d_path.skel.h"
> > +#include "test_d_path_check_rdonly_mem.skel.h"
> >
> > static int duration;
> >
> > @@ -99,7 +100,7 @@ static int trigger_fstat_events(pid_t pid)
> > return ret;
> > }
> >
> > -void test_d_path(void)
> > +static void test_d_path_basic(void)
> > {
> > struct test_d_path__bss *bss;
> > struct test_d_path *skel;
> > @@ -155,3 +156,22 @@ void test_d_path(void)
> > cleanup:
> > test_d_path__destroy(skel);
> > }
> > +
> > +static void test_d_path_check_rdonly_mem(void)
> > +{
> > + struct test_d_path_check_rdonly_mem *skel;
> > +
> > + skel = test_d_path_check_rdonly_mem__open_and_load();
> > + ASSERT_ERR_PTR(skel, "unexpected load of a prog using d_path to write rdonly_mem\n");
> > +
> > + test_d_path_check_rdonly_mem__destroy(skel);
>
> You shouldn't call test_d_path_check_rdonly_mem__destroy(skel) if skel
> is an ERR_PTR. Maybe
> if (!ASSERT_ERR_PTR(...))
> test_d_path_check_rdonly_mem__destroy(skel);
>
Ack. Will change that.
I don't know if it's only me: I find it confusing when figuring out
what ASSERT_ERR_PTR(ptr) returns. Is the returned value 'ptr'? or 'ptr
!= NULL'? or 'err != 0'? I used to think ASSERT-like function/macro
returns nothing.
I noticed that xxx__destroy has a check for NULL, so I put the destroy
function unconditionally.
> > +}
> > +
> > +void test_d_path(void)
> > +{
> > + if (test__start_subtest("basic"))
> > + test_d_path_basic();
> > +
> > + if (test__start_subtest("check_rdonly_mem"))
> > + test_d_path_check_rdonly_mem();
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > new file mode 100644
> > index 000000000000..c7a9655d5850
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_d_path_check_rdonly_mem.c
> > @@ -0,0 +1,30 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2021 Google */
> > +
> > +#include "vmlinux.h"
> > +
> > +#include "vmlinux.h"
>
> duplicated vmlinux.h.
>
Thanks. Will fix that.
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +extern const int bpf_prog_active __ksym;
> > +
> > +SEC("fentry/security_inode_getattr")
> > +int BPF_PROG(d_path_check_rdonly_mem, struct path *path, struct kstat *stat,
> > + __u32 request_mask, unsigned int query_flags)
> > +{
> > + char *active;
>
> int *active?
> It may not matter since the program is rejected by the kernel but
> with making it conforms to kernel definition we have one less thing
> to worry about the verification.
>
Because bpf_d_path() accepts 'char *' instead of 'int *', I need to
cast 'active' to 'char *' somewhere, otherwise the compiler will issue
a warning. To combine with your comment, maybe the following:
int *active;
active = (int *)bpf_per_cpu_ptr(...);
...
bpf_d_path(path, (char *)active, sizeof(int));
> > + __u32 cpu;
> > +
> > + cpu = bpf_get_smp_processor_id();
> > + active = (char *)bpf_per_cpu_ptr(&bpf_prog_active, cpu);
>
> int *
>
> > + if (active) {
> > + /* FAIL here! 'active' is a rdonly_mem. bpf helpers that
>
> 'active' points to readonly memory.
>
Ack.
> > + * update its arguments can not write into it.
> > + */
> > + bpf_d_path(path, active, sizeof(int));
> > + }
> > + return 0;
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
