Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA

[Jeff Xu <jeffxu@google.com>]

On Thu, Dec 15, 2022 at 12:34 PM Jeff Xu <jeffxu@google.com> wrote:
>
> Hi Mickaël
> Thanks for reviewing.
>
> On Thu, Dec 15, 2022 at 10:34 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > This is much better! We can tailor a bit more the tests though.
> >
> > On 13/12/2022 19:58, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@google.com>
> > >
> > > Add check for yama setting for ptrace_test.
> > >
> > > Signed-off-by: Jeff Xu <jeffxu@google.com>
> > > ---
> > >   .../testing/selftests/landlock/ptrace_test.c  | 34 +++++++++++++++++++
> > >   1 file changed, 34 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> > > index c28ef98ff3ac..8565a25a9587 100644
> > > --- a/tools/testing/selftests/landlock/ptrace_test.c
> > > +++ b/tools/testing/selftests/landlock/ptrace_test.c
> > > @@ -60,6 +60,24 @@ static int test_ptrace_read(const pid_t pid)
> > >       return 0;
> > >   }
> > >
> > > +static int get_ptrace_scope(void)
> >
> > Please rename to get_yama_ptrace_scope().
> >
> Done.
>
> > > +{
> > > +     int ret = -1;
> > > +     char buf[2];
> > > +     int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
> > > +
> > > +     if (fd < 0)
> > > +             return 0;
> > > +
> > > +     if (read(fd, &buf, 1) < 0)
> > > +             return -1;
> > > +
> > > +     buf[1] = '\0';
> >
> > You can replace that with `char buf[2] = {};`
> >
> Done.
> The Compiler seems to be getting a lot smarter :) Thanks.
>
> >
> > > +     ret = atoi(buf);
> > > +     close(fd);
> > > +     return ret;
> > > +}
> > > +
> > >   /* clang-format off */
> > >   FIXTURE(hierarchy) {};
> > >   /* clang-format on */
> > > @@ -69,6 +87,7 @@ FIXTURE_VARIANT(hierarchy)
> > >       const bool domain_both;
> > >       const bool domain_parent;
> > >       const bool domain_child;
> > > +     const int  yama_value;
> >
> > Please rename to yama_ptrace_scope_max and remove the extra space.
> >
> why _max ?  yama_ptrace_scope_current is more proporate ?
> FYI: This is the current sysctl setting.
>
Sorry, I will correct myself. This is max supported by the variant,
not the current sysctl setting.
I will change it to :
yama_ptrace_scope_max_supported

> > >   };
> > >
> > >   /*
> > > @@ -93,6 +112,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_without_domain) {
> > >       .domain_both = false,
> > >       .domain_parent = false,
> > >       .domain_child = false,
> > > +     .yama_value = 0,
> > >   };
> > >
> > >   /*
> > > @@ -110,6 +130,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_with_one_domain) {
> > >       .domain_both = false,
> > >       .domain_parent = false,
> > >       .domain_child = true,
> > > +     .yama_value = 1,
> > >   };
> > >
> > >   /*
> > > @@ -126,6 +147,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_parent_domain) {
> > >       .domain_both = false,
> > >       .domain_parent = true,
> > >       .domain_child = false,
> > > +     .yama_value = 0,
> > >   };
> > >
> > >   /*
> > > @@ -143,6 +165,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_sibling_domain) {
> > >       .domain_both = false,
> > >       .domain_parent = true,
> > >       .domain_child = true,
> > > +     .yama_value = 2,
> > >   };
> > >
> > >   /*
> > > @@ -160,6 +183,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_sibling_domain) {
> > >       .domain_both = true,
> > >       .domain_parent = false,
> > >       .domain_child = false,
> > > +     .yama_value = 0,
> > >   };
> > >
> > >   /*
> > > @@ -178,6 +202,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_with_nested_domain) {
> > >       .domain_both = true,
> > >       .domain_parent = false,
> > >       .domain_child = true,
> > > +     .yama_value = 1,
> > >   };
> > >
> > >   /*
> > > @@ -196,6 +221,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_nested_and_parent_domain) {
> > >       .domain_both = true,
> > >       .domain_parent = true,
> > >       .domain_child = false,
> > > +     .yama_value = 0,
> > >   };
> > >
> > >   /*
> > > @@ -216,6 +242,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_forked_domain) {
> > >       .domain_both = true,
> > >       .domain_parent = true,
> > >       .domain_child = true,
> > > +     .yama_value = 2,
> > >   };
> > >
> > >   FIXTURE_SETUP(hierarchy)
> > > @@ -232,9 +259,16 @@ TEST_F(hierarchy, trace)
> > >       pid_t child, parent;
> > >       int status, err_proc_read;
> > >       int pipe_child[2], pipe_parent[2];
> > > +     int yama;
> >
> > Please rename to yama_ptrace_scope.
> >
> Done.
>
>
> >
> > >       char buf_parent;
> > >       long ret;
> > >
> > > +     yama = get_ptrace_scope();
> > > +     ASSERT_LE(0, yama);
> > > +
> > > +     if (variant->yama_value < yama)
> >
> > if (yama_ptrace_scope >= 3)
> >
> > > +             SKIP(return, "unsupported yama value %d", yama);
> >
> > "Yama forbids any ptrace use (scope 3)"
> >
> >
> why comparing with  3?  the test will  skip particular hierarchy,
> according to current
> sysctl yama_ptrace setting.
>
> For example: when kernel.yama.ptrace_scope = 1 the result will be like:
> localhost /usr/local/bin # ./ptrace_test
> TAP version 13
> 1..8
> # Starting 8 tests from 9 test cases.
> #  RUN           hierarchy.allow_without_domain.trace ...
> #      SKIP      unsupported yama value 1
> #            OK  hierarchy.allow_without_domain.trace
> ok 1 # SKIP unsupported yama value 1
> #  RUN           hierarchy.allow_with_one_domain.trace ...
> #            OK  hierarchy.allow_with_one_domain.trace
> ok 2 hierarchy.allow_with_one_domain.trace
> #  RUN           hierarchy.deny_with_parent_domain.trace ...
> #      SKIP      unsupported yama value 1
> #            OK  hierarchy.deny_with_parent_domain.trace
> ok 3 # SKIP unsupported yama value 1
> #  RUN           hierarchy.deny_with_sibling_domain.trace ...
> #            OK  hierarchy.deny_with_sibling_domain.trace
> ok 4 hierarchy.deny_with_sibling_domain.trace
> #  RUN           hierarchy.allow_sibling_domain.trace ...
> #      SKIP      unsupported yama value 1
> #            OK  hierarchy.allow_sibling_domain.trace
> ok 5 # SKIP unsupported yama value 1
> #  RUN           hierarchy.allow_with_nested_domain.trace ...
> #            OK  hierarchy.allow_with_nested_domain.trace
> ok 6 hierarchy.allow_with_nested_domain.trace
> #  RUN           hierarchy.deny_with_nested_and_parent_domain.trace ...
> #      SKIP      unsupported yama value 1
> #            OK  hierarchy.deny_with_nested_and_parent_domain.trace
> ok 7 # SKIP unsupported yama value 1
> #  RUN           hierarchy.deny_with_forked_domain.trace ...
> #            OK  hierarchy.deny_with_forked_domain.trace
> ok 8 hierarchy.deny_with_forked_domain.trace
> # PASSED: 8 / 8 tests passed.
> # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:4 error:0
>
> when yama.ptrace_scope is 2:
> localhost /usr/local/bin # sysctl kernel.yama.ptrace_scope=2
> kernel.yama.ptrace_scope = 2
> localhost /usr/local/bin # ./ptrace_test
> TAP version 13
> 1..8
> # Starting 8 tests from 9 test cases.
> #  RUN           hierarchy.allow_without_domain.trace ...
> #      SKIP      unsupported yama value 2
> #            OK  hierarchy.allow_without_domain.trace
> ok 1 # SKIP unsupported yama value 2
> #  RUN           hierarchy.allow_with_one_domain.trace ...
> #      SKIP      unsupported yama value 2
> #            OK  hierarchy.allow_with_one_domain.trace
> ok 2 # SKIP unsupported yama value 2
> #  RUN           hierarchy.deny_with_parent_domain.trace ...
> #      SKIP      unsupported yama value 2
> #            OK  hierarchy.deny_with_parent_domain.trace
> ok 3 # SKIP unsupported yama value 2
> #  RUN           hierarchy.deny_with_sibling_domain.trace ...
> #            OK  hierarchy.deny_with_sibling_domain.trace
> ok 4 hierarchy.deny_with_sibling_domain.trace
> #  RUN           hierarchy.allow_sibling_domain.trace ...
> #      SKIP      unsupported yama value 2
> #            OK  hierarchy.allow_sibling_domain.trace
> ok 5 # SKIP unsupported yama value 2
> #  RUN           hierarchy.allow_with_nested_domain.trace ...
> #      SKIP      unsupported yama value 2
> #            OK  hierarchy.allow_with_nested_domain.trace
> ok 6 # SKIP unsupported yama value 2
> #  RUN           hierarchy.deny_with_nested_and_parent_domain.trace ...
> #      SKIP      unsupported yama value 2
> #            OK  hierarchy.deny_with_nested_and_parent_domain.trace
> ok 7 # SKIP unsupported yama value 2
> #  RUN           hierarchy.deny_with_forked_domain.trace ...
> #            OK  hierarchy.deny_with_forked_domain.trace
> ok 8 hierarchy.deny_with_forked_domain.trace
> # PASSED: 8 / 8 tests passed.
> # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:6 error:0
>
> > This check skips the whole test, whereas the issues with Yama are about:
> > - a child process tracing its parent;
> > - the PTRACE_TRACEME case.
> >
> > I think the main remaining parts to change is the `if
> > (variant->domain_*` checks to extend with the yama_ptrace_scope_max check.
> >
> > However, it is useful to highlight that a test didn't fully cover
> > Landlock checks. I think the best approach is to call SKIP() at the end
> > of TEST_F(hierarchy, trace) if yama_ptrace_scope >= 1 . This way, we
> > test as much as possible (Landlock and Yama) and we mark the "tampered"
> > tests as skipped.
> >
> I believe the test case should not have a lot of branches and logic
> (if/else), which makes
> the test case more complex and harder to read.  By that reason, SKIP()
> is better at beginning
> of the testcase.
>
> Another reason is resource cleanup.  When SKIP() is not at the
> beginning of tests,
> the cleanup logic can get much more complicated because there are more
> combinations of resource cleanup to
> to be dealt with, after SKIP().
>
> Therefore I do not believe in "tests as much as possible" in a single
> test, I would rather have a
> dedicated test for the situation.
>
> Thoughts ?
>
> >
> > > +
> > >       /*
> > >        * Removes all effective and permitted capabilities to not interfere
> > >        * with cap_ptrace_access_check() in case of PTRACE_MODE_FSCREDS.