[PATCH] selftests/landlock: skip ptrace_test when YAMA is enabled
Jeff Xu
jeffxu at google.com
Wed Jul 6 23:41:24 UTC 2022
A correction (resend with plain text)
> =====================================
> 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.
// 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 8/8 pass
Note: 47,48 of fs_test are failing for all YAMA config values (0-3)
On Tue, Jul 5, 2022 at 2:49 PM Jeff Xu <jeffxu at google.com> 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 ?
>
> > 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.
More information about the Linux-security-module-archive
mailing list