[PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
Mickaël Salaün
mic at digikod.net
Mon Jan 9 15:29:03 UTC 2023
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
>> But I agree this code is difficult to understand, I'm now lost on why
>> we need the negation too.
>>
>>>> /*
>>>> * 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