[PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
Mickaël Salaün
mic at digikod.net
Tue Jan 10 19:04:37 UTC 2023
On 09/01/2023 23:50, Jeff Xu wrote:
> On Mon, Jan 9, 2023 at 7:29 AM Mickaël Salaün <mic at 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 at 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 at google.com> wrote:
>>>>>
>>>>> 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
>>>>>
>>>> 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
Good point.
>>>> 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
This is because a landlocked process can only trace a process in the
same domain or one beneath it. So if a parent process is in its own
domain (whereas the child is not, see the diagrams close to the
FIXTURE_VARIANT definitions), it should not be able to trace the child.
This check is not new.
> 2> yama_ptrace_scope = 0 or 1.
A parent can only trace one of its children up to YAMA_SCOPE_RELATIONAL.
>
> 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 ?
When a parent process *shares* a domain with a child, yes it can trace
it. However when a parent process is in a domain not shared with the
child, it cannot trace it. This is why there is domain_both,
domain_parent and domain_child variants.
>
> 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
>>>>>>
More information about the Linux-security-module-archive
mailing list