* [PATCH v2 0/1] selftests/landlock: Fix selftest ptrace_test run fail @ 2022-12-13 18:58 jeffxu 2022-12-13 18:58 ` [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu 0 siblings, 1 reply; 7+ messages in thread From: jeffxu @ 2022-12-13 18:58 UTC (permalink / raw) To: mic; +Cc: jorgelo, keescook, linux-security-module, groeck, Jeff Xu From: Jeff Xu <jeffxu@google.com> Landlock ptrace failed because YAMA is enabled. This patch check YAMA value and skip related tests. V1: - skip specific testcase according to YAMA value V0: https://lore.kernel.org/r/20220628222941.2642917-1-jeffxu@google.com - skip entire ptrace when YAMA is not 0. Jeff Xu (1): selftests/landlock: skip ptrace_test according to YAMA .../testing/selftests/landlock/ptrace_test.c | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) base-commit: 963a70bee5880640d0fd83ed29dc1e7ec0d2bd4a -- 2.39.0.rc1.256.g54fd8350bd-goog ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA 2022-12-13 18:58 [PATCH v2 0/1] selftests/landlock: Fix selftest ptrace_test run fail jeffxu @ 2022-12-13 18:58 ` jeffxu 2022-12-15 18:34 ` Mickaël Salaün 0 siblings, 1 reply; 7+ messages in thread From: jeffxu @ 2022-12-13 18:58 UTC (permalink / raw) To: mic; +Cc: jorgelo, keescook, linux-security-module, groeck, Jeff Xu 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) +{ + 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'; + 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; }; /* @@ -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; char buf_parent; long ret; + yama = get_ptrace_scope(); + ASSERT_LE(0, yama); + + if (variant->yama_value < yama) + SKIP(return, "unsupported yama value %d", yama); + /* * Removes all effective and permitted capabilities to not interfere * with cap_ptrace_access_check() in case of PTRACE_MODE_FSCREDS. -- 2.39.0.rc1.256.g54fd8350bd-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA 2022-12-13 18:58 ` [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu @ 2022-12-15 18:34 ` Mickaël Salaün 2022-12-15 20:34 ` Jeff Xu 0 siblings, 1 reply; 7+ messages in thread From: Mickaël Salaün @ 2022-12-15 18:34 UTC (permalink / raw) To: jeffxu; +Cc: jorgelo, keescook, linux-security-module, groeck, Jeff Xu 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(). > +{ > + 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] = {};` > + 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. > }; > > /* > @@ -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. > 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)" 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 covered 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. > + > /* > * Removes all effective and permitted capabilities to not interfere > * with cap_ptrace_access_check() in case of PTRACE_MODE_FSCREDS. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA 2022-12-15 18:34 ` Mickaël Salaün @ 2022-12-15 20:34 ` Jeff Xu 2022-12-15 20:42 ` Jeff Xu 2022-12-16 10:15 ` Mickaël Salaün 0 siblings, 2 replies; 7+ messages in thread From: Jeff Xu @ 2022-12-15 20:34 UTC (permalink / raw) To: Mickaël Salaün Cc: jeffxu, jorgelo, keescook, linux-security-module, groeck 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. > > }; > > > > /* > > @@ -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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA 2022-12-15 20:34 ` Jeff Xu @ 2022-12-15 20:42 ` Jeff Xu 2022-12-16 10:15 ` Mickaël Salaün 1 sibling, 0 replies; 7+ messages in thread From: Jeff Xu @ 2022-12-15 20:42 UTC (permalink / raw) To: Mickaël Salaün Cc: jeffxu, jorgelo, keescook, linux-security-module, groeck 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA 2022-12-15 20:34 ` Jeff Xu 2022-12-15 20:42 ` Jeff Xu @ 2022-12-16 10:15 ` Mickaël Salaün 2022-12-16 15:18 ` Jeff Xu 1 sibling, 1 reply; 7+ messages in thread From: Mickaël Salaün @ 2022-12-16 10:15 UTC (permalink / raw) To: Jeff Xu; +Cc: jeffxu, jorgelo, keescook, linux-security-module, groeck 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@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. 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA 2022-12-16 10:15 ` Mickaël Salaün @ 2022-12-16 15:18 ` Jeff Xu 0 siblings, 0 replies; 7+ messages in thread From: Jeff Xu @ 2022-12-16 15:18 UTC (permalink / raw) To: Mickaël Salaün Cc: jeffxu, jorgelo, keescook, linux-security-module, groeck On Fri, Dec 16, 2022 at 2:15 AM Mickaël Salaün <mic@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@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. > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-12-16 15:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-13 18:58 [PATCH v2 0/1] selftests/landlock: Fix selftest ptrace_test run fail jeffxu 2022-12-13 18:58 ` [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu 2022-12-15 18:34 ` Mickaël Salaün 2022-12-15 20:34 ` Jeff Xu 2022-12-15 20:42 ` Jeff Xu 2022-12-16 10:15 ` Mickaël Salaün 2022-12-16 15:18 ` Jeff Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.