[PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA
Mickaël Salaün
mic at digikod.net
Fri Dec 16 10:15:07 UTC 2022
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 at digikod.net> wrote:
>>
>> This is much better! We can tailor a bit more the tests though.
>>
>> On 13/12/2022 19:58, 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 | 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);
More information about the Linux-security-module-archive
mailing list