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

[Guenter Roeck]

On Tue, Jan 3, 2023 at 11:03 AM <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  | 48 ++++++++++++++++---
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> index c28ef98ff3ac..379f5ddf6c3f 100644
> --- a/tools/testing/selftests/landlock/ptrace_test.c
> +++ b/tools/testing/selftests/landlock/ptrace_test.c
> @@ -60,6 +60,23 @@ static int test_ptrace_read(const pid_t pid)
>         return 0;
>  }
>
> +static int get_yama_ptrace_scope(void)
> +{
> +       int ret = -1;

Unnecessary initialization

> +       char buf[2] = {};

Unnecessary initialization

> +       int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
> +
> +       if (fd < 0)
> +               return 0;
> +
> +       if (read(fd, &buf, 1) < 0)

buf is an array, & is thus unnecessary. Also, if the file is empty,
the return value would be 0.

> +               return -1;

leaking file descriptor

> +
> +       ret = atoi(buf);
> +       close(fd);
> +       return ret;
> +}
> +
>  /* clang-format off */
>  FIXTURE(hierarchy) {};
>  /* clang-format on */
> @@ -232,8 +249,20 @@ TEST_F(hierarchy, trace)
>         pid_t child, parent;
>         int status, err_proc_read;
>         int pipe_child[2], pipe_parent[2];
> +       int yama_ptrace_scope;
>         char buf_parent;
>         long ret;
> +       bool can_trace_child, can_trace_parent;
> +
> +       yama_ptrace_scope = get_yama_ptrace_scope();
> +       ASSERT_LE(0, yama_ptrace_scope);
> +
> +       if (yama_ptrace_scope >= 3)
> +               SKIP(return, "Yama forbids any ptrace use (scope %d)",
> +                          yama_ptrace_scope);
> +
> +       can_trace_child = !variant->domain_parent && (yama_ptrace_scope < 2);
> +       can_trace_parent = !variant->domain_child && (yama_ptrace_scope < 1);
>

Unnecessary ( ).

It is difficult to understand the context. yama_ptrace_scope == 2 is
YAMA_SCOPE_CAPABILITY, and yama_ptrace_scope == 1 is
YAMA_SCOPE_RELATIONAL. I for my part have no idea how that relates to
child/parent permissions. Also, I have no idea why the negation
(can_trace_child = !variant->domain_parent) is necessary, and what its
functional impact might be. Someone else will have to chime in here.

>         /*
>          * Removes all effective and permitted capabilities to not interfere
> @@ -258,6 +287,7 @@ TEST_F(hierarchy, trace)
>
>                 ASSERT_EQ(0, close(pipe_parent[1]));
>                 ASSERT_EQ(0, close(pipe_child[0]));
> +

Unnecessary whitespace change

>                 if (variant->domain_child)

Why not change this code ?

>                         create_domain(_metadata);
>
> @@ -267,7 +297,7 @@ TEST_F(hierarchy, trace)
>                 /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the parent. */
>                 err_proc_read = test_ptrace_read(parent);
>                 ret = ptrace(PTRACE_ATTACH, parent, NULL, 0);
> -               if (variant->domain_child) {
> +               if (!can_trace_parent) {
>                         EXPECT_EQ(-1, ret);
>                         EXPECT_EQ(EPERM, errno);
>                         EXPECT_EQ(EACCES, err_proc_read);
> @@ -283,7 +313,7 @@ TEST_F(hierarchy, trace)
>
>                 /* Tests child PTRACE_TRACEME. */
>                 ret = ptrace(PTRACE_TRACEME);
> -               if (variant->domain_parent) {
> +               if (!can_trace_child) {
>                         EXPECT_EQ(-1, ret);
>                         EXPECT_EQ(EPERM, errno);
>                 } else {
> @@ -296,12 +326,12 @@ TEST_F(hierarchy, trace)
>                  */
>                 ASSERT_EQ(1, write(pipe_child[1], ".", 1));
>
> -               if (!variant->domain_parent) {
> +               if (can_trace_child)
>                         ASSERT_EQ(0, raise(SIGSTOP));
> -               }
>
>                 /* Waits for the parent PTRACE_ATTACH test. */
>                 ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> +

Unnecessary whitespace change

>                 _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
>                 return;
>         }
> @@ -321,7 +351,7 @@ TEST_F(hierarchy, trace)
>         ASSERT_EQ(1, read(pipe_child[0], &buf_parent, 1));
>
>         /* Tests child PTRACE_TRACEME. */
> -       if (!variant->domain_parent) {
> +       if (can_trace_child) {
>                 ASSERT_EQ(child, waitpid(child, &status, 0));
>                 ASSERT_EQ(1, WIFSTOPPED(status));
>                 ASSERT_EQ(0, ptrace(PTRACE_DETACH, child, NULL, 0));
> @@ -334,7 +364,7 @@ TEST_F(hierarchy, trace)
>         /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the child. */
>         err_proc_read = test_ptrace_read(child);
>         ret = ptrace(PTRACE_ATTACH, child, NULL, 0);
> -       if (variant->domain_parent) {
> +       if (!can_trace_child) {
>                 EXPECT_EQ(-1, ret);
>                 EXPECT_EQ(EPERM, errno);
>                 EXPECT_EQ(EACCES, err_proc_read);
> @@ -350,10 +380,16 @@ TEST_F(hierarchy, trace)
>
>         /* Signals that the parent PTRACE_ATTACH test is done. */
>         ASSERT_EQ(1, write(pipe_parent[1], ".", 1));
> +

Unnecessary whitespace change

>         ASSERT_EQ(child, waitpid(child, &status, 0));
>         if (WIFSIGNALED(status) || !WIFEXITED(status) ||
>             WEXITSTATUS(status) != EXIT_SUCCESS)
>                 _metadata->passed = 0;
> +
> +       if (yama_ptrace_scope > 0)
> +               SKIP(return,
> +                          "Incomplete tests due to Yama restrictions (scope %d)",
> +                          yama_ptrace_scope);
>  }
>
>  TEST_HARNESS_MAIN
> --
> 2.39.0.314.g84b9a713c41-goog
>