[PATCH] selftests/landlock: skip ptrace_test when YAMA is enabled

[Mickaël Salaün]

On 05/07/2022 23:49, Jeff Xu wrote:
> Hi Mickaël
> 
> Thank you for your review, please see my response below.
> 
>> Hi Jeff,
>>
>> Thanks for this patch. Here are some comments:
>>
>> On 29/06/2022 00:29, Jeff Xu wrote:
>>> ptrace_test assumes YAMA is disabled, skip it if YAMA is enabled.
>>>
>>> Cc: Jorge Lucangeli Obes <jorgelo at chromium.org>
>>> Cc: Guenter Roeck <groeck at chromium.org>
>>> Cc: Kees Cook <keescook at chromium.org>
>>> Cc: Mickaël Salaün <mic at digikod.net>
>>> Tested-by: Jeff Xu <jeffxu at google.com>
>>> Signed-off-by: Jeff Xu <jeffxu at google.com>
>>> Change-Id: I623742ca9f20ec706a38c92f6c0bab755f73578f
>>> ---
>>>    .../testing/selftests/landlock/ptrace_test.c  | 49 +++++++++++++++++++
>>>    1 file changed, 49 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
>>> index c28ef98ff3ac..ef2d36f56764 100644
>>> --- a/tools/testing/selftests/landlock/ptrace_test.c
>>> +++ b/tools/testing/selftests/landlock/ptrace_test.c
>>> @@ -226,6 +226,44 @@ FIXTURE_TEARDOWN(hierarchy)
>>>    {
>>>    }
>>>
>>
>> Please move these new helpers after test_ptrace_read() and make them static.
>>
>>> +int open_sysfs(const char *path, int flags, int *fd)
>>> +{
>>> +     *fd = open(path, flags);
>>> +
>>> +     if (fd < 0)
>>> +             return -1;
>>> +
>>> +     return 0;
>>> +}
>>
>> open_sysfs() can be replaced with a call to open(). This makes the code
>> simpler.
>>
>>> +
>>> +int read_sysfs_int_fd(int fd, int *val)
>>> +{
>>> +     char buf[2];
>>> +
>>> +     if (read(fd, buf, sizeof(buf)) < 0)
>>
>> I guess `read(fd, buf, 1)` should be enough and it enables keeping the
>> final '\0'. A comment should state that this helper only read the first
>> digit (which is enough for Yama).
>>
>>> +             return -1;
>>> +
>>> +     buf[sizeof(buf) - 1] = '\0';
>>
>> Use `char buf[2] = {};` instead.
>>
>>> +     *val = atoi(buf);
>>> +     return 0;
>>> +}
>>
> 
> Thanks, I will revise the code, my original thought is to extend it as
> a common utility function to parse an int, let me finish it in the
> next iteration of patch.
> 
>> Same for read_sysfs_int_fd(), you can inline the code in read_sysfs_int().
>> This is a good test but it fail if Yama is not built in the kernel.
>>
> I don't have a kernel built without yama, so my original thought is to
> fail it and whoever has the need can fix it. What is your thought on this ?

The current status is that all tests should pass with a minimal kernel 
configuration (cf. the test config file). Yama is an exception for these 
tests, not the norm, so you also need to test against a kernel without Yama.


> 
>> For now, I think you can create two helpers named something like
>> is_yama_restricting() and is_yama_denying() (for admin-only attach).
>>
> Can you please clarify on the difference/implementation on those 2 ?

They should check for the different values of ptrace_scope: 
https://docs.kernel.org/admin-guide/LSM/Yama.html
- is_yama_restricting() should return true if ptrace_scope == 1
- is_yama_denying() should return true if ptrace_scope >= 2

Restricted ptrace only forbids a child process to ptrace its parents. 
There is no specific restriction for a parent to ptrace its children.

Ptracing is always denied if ptrace_scope >= 2 (without specific 
capabilties, which is the case for these tests).

> 
>>> +     if (ptrace_val != 0) {
>>
>> Some tests should work even if ptrace_val == 1. SKIP() should only be
>> called when the test would fail. Can you please check all tests with all
>> Yama values?
> Sure, below are yama cases with testing result:
> =====================================
> case 0 - classic ptrace permissions: a process can PTRACE_ATTACH to any other
>      process running under the same uid, as long as it is dumpable (i.e.
>      did not transition uids, start privileged, or have called
>      prctl(PR_SET_DUMPABLE...) already). Similarly, PTRACE_TRACEME is
>      unchanged.
> 
> Test: All passing
> ======================================
> Case 1 - restricted ptrace: a process must have a predefined relationship
>      with the inferior it wants to call PTRACE_ATTACH on. By default,
>      this relationship is that of only its descendants when the above
>      classic criteria is also met. To change the relationship, an
>      inferior can call prctl(PR_SET_PTRACER, debugger, ...) to declare
>      an allowed debugger PID to call PTRACE_ATTACH on the inferior.
>      Using PTRACE_TRACEME is unchanged.
> 
> Test:
> // Base_test: 7/7 pass.
> // Fs_test 46/48 pass
> //.   not ok 47 layout2_overlay.no_restriction
> //.   not ok 48 layout2_overlay.same_content_different_file
> //  Ptrace_test 4/8 pass
> // #          FAIL  hierarchy.allow_without_domain.trace
> // #          FAIL  hierarchy.deny_with_parent_domain.trace
> // #          FAIL  hierarchy.allow_sibling_domain.trace
> // #          FAIL  hierarchy.deny_with_nested_and_parent_domain.trace
> ====================================================
> Case 2 - admin-only attach: only processes with CAP_SYS_PTRACE may use ptrace
>      with PTRACE_ATTACH, or through children calling PTRACE_TRACEME.
> Case 3 - no attach: no processes may use ptrace with PTRACE_ATTACH nor via
>      PTRACE_TRACEME. Once set, this sysctl value cannot be changed.
> Test: *case2 and case3 have the same results:
> // Base_test: 7/7 pass.
> // Fs_test 46/48 pass
> //.   not ok 47 layout2_overlay.no_restriction
> //.   not ok 48 layout2_overlay.same_content_different_file
> //  Ptrace 2/8 pass
> //.  ok 4 hierarchy.deny_with_sibling_domain.trace
> //.  ok 8 hierarchy.deny_with_forked_domain.trace
> // the other 6 tests failed with timeout.
> ===============================================
> 
> Do you know why fs_test (47,48) is failing when yama value = 1,2,3 ?

These are all the overlayfs tests but I don't know why they are failing 
with Yama. Could you please pinpoint the exact(s) failing ASSERT? 
(Whatever my following comments, this would be valuable to know the issue.)


> 
> FOR SKIP,  it might be messy to add SKIP after checking variant names
> in TEST_F(), (too many if/else , which make it less readable),
> ideally this should be when or before FIXTURE_VARIANT_ADD() is called.
> or somehow refactor the code to remove the variant check in TEST_F()
> 
> Is there a better way  ?

OK, let's follow another approach.

The first alternative would be to disable Yama for all the Landlock 
ptrace tests. You'll first need to save the current Yama settings and 
restore it after each test, even if they failed. I think this 
alternative makes sense because Landlock tests should not be about Yama.

The second alternative would be to test with and without Yama, with 
different Yama settings. That would also require to disable Yama for 1/3 
or 1/4 of tests. The downside of this alternative is that it requires 3 
or 4 times variants, and it actually test Yama, which is not the goal of 
the Landlock tests.

Anyway, you also need to update the comment about Yama in the current tests.