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

[Jeff Xu]

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 ?

> 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 ?

> > +     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 ?

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  ?

Thanks
Best Regards,
Jeff



On Thu, Jun 30, 2022 at 8:31 AM Mickaël Salaün <mic at digikod.net> wrote:
>
>
> 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>
>
> I guess we assume that Signed-off-by implies Tested-by, so you can
> remove this Tested-by.
>
> > Signed-off-by: Jeff Xu <jeffxu at google.com>
> > Change-Id: I623742ca9f20ec706a38c92f6c0bab755f73578f
>
> Please remove this Change-Id too.