[PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA
Jeff Xu
jeffxu at google.com
Fri Dec 16 15:18:43 UTC 2022
On Fri, Dec 16, 2022 at 2:15 AM Mickaël Salaün <mic at digikod.net> wrote:
>
>
> On 15/12/2022 21:34, Jeff Xu 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.
>
> In response to your other email, yama_ptrace_scope_max_supported looks
> good too.
>
> >
> >>> };
> >>>
> >>> /*
> >>> @@ -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.
>
> The idea is to run the tests as much as possible, but in the case of a
> scope of 3, any ptrace is denied. However, it would indeed be better to
> integrate this third value in the following `if (variant->domain_*`
> checks, like for the other scopes.
>
>
> >
> > 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.
>
> I agree in a general case, but here all the branches already exist to
> test all possible Landlock combinations. Adding Yama's ptrace scope will
> only update the existing branch conditions.
>
>
> >
> > 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().
>
> All the logic for resource cleanup is already in place.
>
> >
> > 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.
>
> The current test code already covers all possible combinations, so it is
> just a matter of extending the branch conditions. You can update these
> checks using dedicated booleans:
> - can_trace_child = !variant->domain_parent && (yama_ptrace_scope < 2);
> - can_trace_parent = !variant->domain_child && (yama_ptrace_scope < 1);
>
> …and at the end of all the hierarchy.trace tests add:
> if (yama_ptrace_scope > 0)
> SKIP(return, "Incomplete tests due to Yama restrictions (ptrace_scope:
> %d)", yama_ptrace_scope);
OKey, I will update and send a new one.
Thanks
Jeff
More information about the Linux-security-module-archive
mailing list