* [PATCH v4 0/1] selftests/landlock: Fix selftest ptrace_test
@ 2023-01-03 19:03 jeffxu
2023-01-03 19:03 ` [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu
0 siblings, 1 reply; 9+ messages in thread
From: jeffxu @ 2023-01-03 19:03 UTC (permalink / raw)
To: mic; +Cc: jorgelo, keescook, linux-security-module, groeck, gnoack, Jeff Xu
From: Jeff Xu <jeffxu@google.com>
Landlock ptrace failed because YAMA is enabled.
This patch check YAMA value and skip related tests.
V4:
- apply can_trace_parent and can_trace_child to smaller scope.
V3:
https://lore.kernel.org/all/20221227140244.1041292-1-jeffxu@google.com
- add can_trace_parent and can_trace_child to handle yama value.
V2:
https://lore.kernel.org/all/20221213185816.3942853-1-jeffxu@chromium.org
- update from code review comments.
V1:
https://lore.kernel.org/all/20221213185816.3942853-1-jeffxu@chromium.org
- skip specific testcase according to YAMA value
V0:
https://lore.kernel.org/r/20220628222941.2642917-1-jeffxu@google.com
- skip entire ptrace when YAMA is not 0.
Jeff Xu (1):
selftests/landlock: skip ptrace_test according to YAMA
.../testing/selftests/landlock/ptrace_test.c | 48 ++++++++++++++++---
1 file changed, 42 insertions(+), 6 deletions(-)
base-commit: 963a70bee5880640d0fd83ed29dc1e7ec0d2bd4a
--
2.39.0.314.g84b9a713c41-goog
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-03 19:03 [PATCH v4 0/1] selftests/landlock: Fix selftest ptrace_test jeffxu
@ 2023-01-03 19:03 ` jeffxu
2023-01-03 20:12 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: jeffxu @ 2023-01-03 19:03 UTC (permalink / raw)
To: mic; +Cc: jorgelo, keescook, linux-security-module, groeck, gnoack, Jeff Xu
From: Jeff Xu <jeffxu@google.com>
Add check for yama setting for ptrace_test.
Signed-off-by: Jeff Xu <jeffxu@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;
+ 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;
+
+ 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);
/*
* 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]));
+
if (variant->domain_child)
create_domain(_metadata);
@@ -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));
+
_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));
+
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-03 19:03 ` [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu
@ 2023-01-03 20:12 ` Guenter Roeck
2023-01-03 23:49 ` Jeff Xu
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2023-01-03 20:12 UTC (permalink / raw)
To: jeffxu
Cc: mic, jorgelo, keescook, linux-security-module, groeck, gnoack, Jeff Xu
On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
>
> From: Jeff Xu <jeffxu@google.com>
>
> Add check for yama setting for ptrace_test.
>
> Signed-off-by: Jeff Xu <jeffxu@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
> + 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.
> /*
> * 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);
>
> @@ -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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-03 20:12 ` Guenter Roeck
@ 2023-01-03 23:49 ` Jeff Xu
2023-01-04 3:40 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Xu @ 2023-01-03 23:49 UTC (permalink / raw)
To: Guenter Roeck
Cc: jeffxu, mic, jorgelo, keescook, linux-security-module, groeck, gnoack
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@google.com> wrote:
>
> On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
> >
> > From: Jeff Xu <jeffxu@google.com>
> >
> > Add check for yama setting for ptrace_test.
> >
> > Signed-off-by: Jeff Xu <jeffxu@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)
> > + 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
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-03 23:49 ` Jeff Xu
@ 2023-01-04 3:40 ` Guenter Roeck
2023-01-09 15:29 ` Mickaël Salaün
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2023-01-04 3:40 UTC (permalink / raw)
To: Jeff Xu
Cc: jeffxu, mic, jorgelo, keescook, linux-security-module, groeck, gnoack
On Tue, Jan 3, 2023 at 3:50 PM Jeff Xu <jeffxu@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@google.com> wrote:
> >
> > On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
> > >
> > > From: Jeff Xu <jeffxu@google.com>
> > >
> > > Add check for yama setting for ptrace_test.
> > >
> > > Signed-off-by: Jeff Xu <jeffxu@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
> > > + 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
> > >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-04 3:40 ` Guenter Roeck
@ 2023-01-09 15:29 ` Mickaël Salaün
2023-01-09 22:50 ` Jeff Xu
0 siblings, 1 reply; 9+ messages in thread
From: Mickaël Salaün @ 2023-01-09 15:29 UTC (permalink / raw)
To: Guenter Roeck, Jeff Xu
Cc: jeffxu, jorgelo, keescook, linux-security-module, groeck, gnoack
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@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@google.com> wrote:
>>>
>>> On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
>>>>
>>>> From: Jeff Xu <jeffxu@google.com>
>>>>
>>>> Add check for yama setting for ptrace_test.
>>>>
>>>> Signed-off-by: Jeff Xu <jeffxu@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
>>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-09 15:29 ` Mickaël Salaün
@ 2023-01-09 22:50 ` Jeff Xu
2023-01-10 19:04 ` Mickaël Salaün
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Xu @ 2023-01-09 22:50 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Guenter Roeck, jeffxu, jorgelo, keescook, linux-security-module,
groeck, gnoack
On Mon, Jan 9, 2023 at 7:29 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> 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@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@google.com> wrote:
> >>>
> >>> On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
> >>>>
> >>>> From: Jeff Xu <jeffxu@google.com>
> >>>>
> >>>> Add check for yama setting for ptrace_test.
> >>>>
> >>>> Signed-off-by: Jeff Xu <jeffxu@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.
> >>
Hi Mickaël
Can you check the above comment please ?
I also find it difficult to understand how can_trace_child is set.
On this line:
can_trace_child = !variant->domain_parent &&
yama_ptrace_scope < 2;
it translates to
can_trace_child is true when 1> && 2>
1> when parent process don't have landlock policy
2> yama_ptrace_scope = 0 or 1.
My question is:
When the parent process has a landlock policy, and 2 is true,
the parent can also trace the child process, right ?
So 1> is not necessary in theory ?
As reference: the latest code (after updating the rest of comments in V7)
can be found at patchset 8 of
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4084253
Thanks
Jeff
> >>>> /*
> >>>> * 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
> >>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-09 22:50 ` Jeff Xu
@ 2023-01-10 19:04 ` Mickaël Salaün
2023-01-10 20:41 ` Jeff Xu
0 siblings, 1 reply; 9+ messages in thread
From: Mickaël Salaün @ 2023-01-10 19:04 UTC (permalink / raw)
To: Jeff Xu
Cc: Guenter Roeck, jeffxu, jorgelo, keescook, linux-security-module,
groeck, gnoack
On 09/01/2023 23:50, Jeff Xu wrote:
> On Mon, Jan 9, 2023 at 7:29 AM Mickaël Salaün <mic@digikod.net> wrote:
>>
>> 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@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@google.com> wrote:
>>>>>
>>>>> On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
>>>>>>
>>>>>> From: Jeff Xu <jeffxu@google.com>
>>>>>>
>>>>>> Add check for yama setting for ptrace_test.
>>>>>>
>>>>>> Signed-off-by: Jeff Xu <jeffxu@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
Good point.
>>>> But I agree this code is difficult to understand, I'm now lost on why
>>>> we need the negation too.
>>>>
> Hi Mickaël
>
> Can you check the above comment please ?
> I also find it difficult to understand how can_trace_child is set.
>
> On this line:
> can_trace_child = !variant->domain_parent &&
> yama_ptrace_scope < 2;
>
> it translates to
> can_trace_child is true when 1> && 2>
> 1> when parent process don't have landlock policy
This is because a landlocked process can only trace a process in the
same domain or one beneath it. So if a parent process is in its own
domain (whereas the child is not, see the diagrams close to the
FIXTURE_VARIANT definitions), it should not be able to trace the child.
This check is not new.
> 2> yama_ptrace_scope = 0 or 1.
A parent can only trace one of its children up to YAMA_SCOPE_RELATIONAL.
>
> My question is:
> When the parent process has a landlock policy, and 2 is true,
> the parent can also trace the child process, right ?
> So 1> is not necessary in theory ?
When a parent process *shares* a domain with a child, yes it can trace
it. However when a parent process is in a domain not shared with the
child, it cannot trace it. This is why there is domain_both,
domain_parent and domain_child variants.
>
> As reference: the latest code (after updating the rest of comments in V7)
> can be found at patchset 8 of
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4084253
>
> Thanks
> Jeff
>
>>>>>> /*
>>>>>> * 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
>>>>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-10 19:04 ` Mickaël Salaün
@ 2023-01-10 20:41 ` Jeff Xu
0 siblings, 0 replies; 9+ messages in thread
From: Jeff Xu @ 2023-01-10 20:41 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Guenter Roeck, jeffxu, jorgelo, keescook, linux-security-module,
groeck, gnoack
On Tue, Jan 10, 2023 at 11:04 AM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 09/01/2023 23:50, Jeff Xu wrote:
> > On Mon, Jan 9, 2023 at 7:29 AM Mickaël Salaün <mic@digikod.net> wrote:
> >>
> >> 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@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@google.com> wrote:
> >>>>>
> >>>>> On Tue, Jan 3, 2023 at 11:03 AM <jeffxu@chromium.org> wrote:
> >>>>>>
> >>>>>> From: Jeff Xu <jeffxu@google.com>
> >>>>>>
> >>>>>> Add check for yama setting for ptrace_test.
> >>>>>>
> >>>>>> Signed-off-by: Jeff Xu <jeffxu@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
>
> Good point.
>
> >>>> But I agree this code is difficult to understand, I'm now lost on why
> >>>> we need the negation too.
> >>>>
> > Hi Mickaël
> >
> > Can you check the above comment please ?
> > I also find it difficult to understand how can_trace_child is set.
> >
> > On this line:
> > can_trace_child = !variant->domain_parent &&
> > yama_ptrace_scope < 2;
> >
> > it translates to
> > can_trace_child is true when 1> && 2>
> > 1> when parent process don't have landlock policy
>
> This is because a landlocked process can only trace a process in the
> same domain or one beneath it. So if a parent process is in its own
> domain (whereas the child is not, see the diagrams close to the
> FIXTURE_VARIANT definitions), it should not be able to trace the child.
>
> This check is not new.
>
>
> > 2> yama_ptrace_scope = 0 or 1.
>
> A parent can only trace one of its children up to YAMA_SCOPE_RELATIONAL.
>
> >
> > My question is:
> > When the parent process has a landlock policy, and 2 is true,
> > the parent can also trace the child process, right ?
> > So 1> is not necessary in theory ?
>
> When a parent process *shares* a domain with a child, yes it can trace
> it. However when a parent process is in a domain not shared with the
> child, it cannot trace it. This is why there is domain_both,
> domain_parent and domain_child variants.
>
Thanks for clarification.
I'm adding below comments to help readers:
can_trace_child: if a parent process can trace its child process.
There are two conditions concerning landlock:
1> the parent and child processes are in the same landlock domain or
one beneath it (case: domain_both = true).
2> yama allows tracing children (up to YAMA_SCOPE_RELATIONAL).
Both 1 and 2 need to be met for can_trace_child to be true.
If a parent process has its own domain not shared with the child
process (case:domain_parent = true), then the parent can't trace the
child.
can_trace_parent: if a child process can trace its parent process.
There are two conditions concerning landlock:
1> the parent and child process are in the same landlock domain or
one beneath it.(case: domain_both = true).
2> yama is disabled (YAMA_SCOPE_DISABLED).
Both 1 and 2 need to be met for can_trace_parent to be true.
If a child process has its own domain not shared with the parent
process (case:domain_child = true, then the child can't trace the
parent.
>
> >
> > As reference: the latest code (after updating the rest of comments in V7)
> > can be found at patchset 8 of
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/4084253
> >
> > Thanks
> > Jeff
> >
> >>>>>> /*
> >>>>>> * 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
> >>>>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-10 20:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-03 19:03 [PATCH v4 0/1] selftests/landlock: Fix selftest ptrace_test jeffxu
2023-01-03 19:03 ` [PATCH v4 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu
2023-01-03 20:12 ` Guenter Roeck
2023-01-03 23:49 ` Jeff Xu
2023-01-04 3:40 ` Guenter Roeck
2023-01-09 15:29 ` Mickaël Salaün
2023-01-09 22:50 ` Jeff Xu
2023-01-10 19:04 ` Mickaël Salaün
2023-01-10 20:41 ` Jeff Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.