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

[Jeff Xu <jeffxu@google.com>]

On Mon, Jan 9, 2023 at 7:29 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> Looks good and agree with Guenter's suggestions
>
> On 04/01/2023 04:40, Guenter Roeck wrote:
> > On Tue, Jan 3, 2023 at 3:50 PM Jeff Xu <jeffxu@google.com> wrote:
> >>
> >> Thanks for the comments.
> >> I agree with most comments, but need Michael to chime in/confirm on below:
> >>
> >> On Tue, Jan 3, 2023 at 12:12 PM Guenter Roeck <groeck@google.com> wrote:
> >>>
> >>> On Tue, Jan 3, 2023 at 11:03 AM <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  | 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
> >>>
> >> buf was used later by atoi(), and atoi needs a string, because the
> >> function only reads one byte in read(),
> >> so it needs to add buf[1] = '\0'. In V2, there was a comment  to
> >> change the buf[1] = '\0' to char buf[2] = {},
> >> my understanding is that the compiler is smart enough and will
> >> optimize the initialization to write 0 on the
> >> memory  (since this is char and length is 2, and less then the size of int)
> >>
> >
> > Good point.
> >
> > Guenter
>
> Looks good to me with the other suggestions applied.
>
>
> >
> >>>> +       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.
> >>>
> >> I will copy the definition of the constant definition from yama_lsm.c
> >> But I agree this code is difficult to understand, I'm now lost on why
> >> we need the negation too.
> >>
Hi Mickaël

Can you check the above comment please ?
I also find it difficult to understand how can_trace_child is set.

On this line:
can_trace_child = !variant->domain_parent &&
  yama_ptrace_scope < 2;

it translates to
can_trace_child is true when 1> && 2>
1> when parent process don't have landlock policy
2> yama_ptrace_scope = 0 or 1.

My question is:
When the parent process has a landlock policy, and 2 is true,
the parent can also trace the child process, right ?
So 1> is not necessary in theory ?

As reference:  the latest code (after updating the rest of comments in V7)
can be found at patchset 8 of
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4084253

Thanks
Jeff

> >>>>          /*
> >>>>           * 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);
> >>>>
> >> create_domain actually applies the landlocked policy to the
> >> (child/parent) process.
> >> This is part of the setup of the testcase, so it is needed.
> >>
> >>
> >>>> @@ -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
> >>>>