[PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA
Jeff Xu
jeffxu at google.com
Thu Dec 15 20:42:40 UTC 2022
On Thu, Dec 15, 2022 at 12:34 PM Jeff Xu <jeffxu at google.com> wrote:
>
> Hi Mickaël
> Thanks for reviewing.
>
> On Thu, Dec 15, 2022 at 10:34 AM Mickaël Salaün <mic at digikod.net> wrote:
> >
> > This is much better! We can tailor a bit more the tests though.
> >
> > On 13/12/2022 19:58, jeffxu at chromium.org wrote:
> > > From: Jeff Xu <jeffxu at google.com>
> > >
> > > Add check for yama setting for ptrace_test.
> > >
> > > Signed-off-by: Jeff Xu <jeffxu at 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.
More information about the Linux-security-module-archive
mailing list