* [PATCH v5 0/1] selftests/landlock: fix ptrace_test
@ 2023-01-13 5:07 jeffxu
2023-01-13 5:07 ` [PATCH v5 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu
0 siblings, 1 reply; 5+ messages in thread
From: jeffxu @ 2023-01-13 5:07 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.
V5:
- address comments from groeck@google.com
V4:
- https://lore.kernel.org/all/20230103190314.3882177-1-jeffxu@google.com
- 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 | 82 +++++++++++++++++--
1 file changed, 76 insertions(+), 6 deletions(-)
base-commit: 963a70bee5880640d0fd83ed29dc1e7ec0d2bd4a
--
2.39.0.314.g84b9a713c41-goog
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v5 1/1] selftests/landlock: skip ptrace_test according to YAMA
2023-01-13 5:07 [PATCH v5 0/1] selftests/landlock: fix ptrace_test jeffxu
@ 2023-01-13 5:07 ` jeffxu
2023-01-13 17:53 ` [PATCH] selftests/landlock: Improve ptrace_test with Yama Mickaël Salaün
0 siblings, 1 reply; 5+ messages in thread
From: jeffxu @ 2023-01-13 5:07 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 | 82 +++++++++++++++++--
1 file changed, 76 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
index c28ef98ff3ac..c9d9f3001d0f 100644
--- a/tools/testing/selftests/landlock/ptrace_test.c
+++ b/tools/testing/selftests/landlock/ptrace_test.c
@@ -19,6 +19,12 @@
#include "common.h"
+/* copy from yama_lsm.c */
+#define YAMA_SCOPE_DISABLED 0
+#define YAMA_SCOPE_RELATIONAL 1
+#define YAMA_SCOPE_CAPABILITY 2
+#define YAMA_SCOPE_NO_ATTACH 3
+
static void create_domain(struct __test_metadata *const _metadata)
{
int ruleset_fd;
@@ -60,6 +66,25 @@ static int test_ptrace_read(const pid_t pid)
return 0;
}
+static int get_yama_ptrace_scope(void)
+{
+ int ret;
+ 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) {
+ close(fd);
+ return -1;
+ }
+
+ ret = atoi(buf);
+ close(fd);
+ return ret;
+}
+
/* clang-format off */
FIXTURE(hierarchy) {};
/* clang-format on */
@@ -232,8 +257,49 @@ 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 >= YAMA_SCOPE_NO_ATTACH)
+ SKIP(return, "Yama forbids any ptrace use (scope %d)",
+ yama_ptrace_scope);
+
+ /*
+ * 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_child = !variant->domain_parent &&
+ yama_ptrace_scope < YAMA_SCOPE_CAPABILITY;
+
+ /*
+ * 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.
+ */
+ can_trace_parent = !variant->domain_child &&
+ yama_ptrace_scope < YAMA_SCOPE_RELATIONAL;
/*
* Removes all effective and permitted capabilities to not interfere
@@ -267,7 +333,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 +349,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,9 +362,8 @@ 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));
@@ -321,7 +386,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 +399,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);
@@ -354,6 +419,11 @@ TEST_F(hierarchy, trace)
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] 5+ messages in thread
* [PATCH] selftests/landlock: Improve ptrace_test with Yama
2023-01-13 5:07 ` [PATCH v5 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu
@ 2023-01-13 17:53 ` Mickaël Salaün
2023-01-13 18:12 ` Mickaël Salaün
0 siblings, 1 reply; 5+ messages in thread
From: Mickaël Salaün @ 2023-01-13 17:53 UTC (permalink / raw)
To: Jeff Xu
Cc: Mickaël Salaün, gnoack, groeck, jorgelo, keescook,
linux-security-module
I found some issues running this new ptrace_test. Indeed, Yama doesn't
restrict PTRACE_MODE_READ whereas Landlock does. I also changed some
comments and socpe conditions. In fact, the final SKIP() wasn't a good
idea because it masks such potential errors; let's only use a log
message instead. I also removed the first SKIP() because we want
everything to be tested, and the MODE_READ does still matter anyway.
Please review this patch and squash it in yours.
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20230113175308.50900-1-mic@digikod.net
---
.../testing/selftests/landlock/ptrace_test.c | 106 ++++++++++--------
1 file changed, 57 insertions(+), 49 deletions(-)
diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
index c9d9f3001d0f..3c3ba37bf8ec 100644
--- a/tools/testing/selftests/landlock/ptrace_test.c
+++ b/tools/testing/selftests/landlock/ptrace_test.c
@@ -19,7 +19,7 @@
#include "common.h"
-/* copy from yama_lsm.c */
+/* Copied from security/yama/yama_lsm.c */
#define YAMA_SCOPE_DISABLED 0
#define YAMA_SCOPE_RELATIONAL 1
#define YAMA_SCOPE_CAPABILITY 2
@@ -70,7 +70,7 @@ static int get_yama_ptrace_scope(void)
{
int ret;
char buf[2] = {};
- int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
+ const int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
if (fd < 0)
return 0;
@@ -260,46 +260,48 @@ TEST_F(hierarchy, trace)
int yama_ptrace_scope;
char buf_parent;
long ret;
- bool can_trace_child, can_trace_parent;
+ bool can_read_child, can_trace_child, can_read_parent, can_trace_parent;
yama_ptrace_scope = get_yama_ptrace_scope();
ASSERT_LE(0, yama_ptrace_scope);
- if (yama_ptrace_scope >= YAMA_SCOPE_NO_ATTACH)
- SKIP(return, "Yama forbids any ptrace use (scope %d)",
- yama_ptrace_scope);
+ if (yama_ptrace_scope > YAMA_SCOPE_DISABLED)
+ TH_LOG("Incomplete tests due to Yama restrictions (scope %d)",
+ yama_ptrace_scope);
/*
- * 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_read_child is true if a parent process can read its child
+ * process, which is only the case when the parent process is not
+ * isolated from the child with a dedicated Landlock domain.
*/
- can_trace_child = !variant->domain_parent &&
- yama_ptrace_scope < YAMA_SCOPE_CAPABILITY;
+ can_read_child = !variant->domain_parent;
/*
- * 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.
+ * can_trace_child is true if a parent process can trace its child
+ * process. This depends on two conditions:
+ * - The parent process is not isolated from the child with a dedicated
+ * Landlock domain.
+ * - Yama allows tracing children (up to YAMA_SCOPE_RELATIONAL).
*/
- can_trace_parent = !variant->domain_child &&
- yama_ptrace_scope < YAMA_SCOPE_RELATIONAL;
+ can_trace_child = can_read_child &&
+ yama_ptrace_scope <= YAMA_SCOPE_RELATIONAL;
+
+ /*
+ * can_read_parent is true if a child process can read its parent
+ * process, which is only the case when the child process is not
+ * isolated from the parent with a dedicated Landlock domain.
+ */
+ can_read_parent = !variant->domain_child;
+
+ /*
+ * can_trace_parent is true if a child process can trace its parent
+ * process. This depends on two conditions:
+ * - The child process is not isolated from the parent with a dedicated
+ * Landlock domain.
+ * - Yama is disabled (YAMA_SCOPE_DISABLED).
+ */
+ can_trace_parent = can_read_parent &&
+ yama_ptrace_scope <= YAMA_SCOPE_DISABLED;
/*
* Removes all effective and permitted capabilities to not interfere
@@ -330,16 +332,21 @@ TEST_F(hierarchy, trace)
/* Waits for the parent to be in a domain, if any. */
ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
- /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the parent. */
+ /* Tests PTRACE_MODE_READ on the parent. */
err_proc_read = test_ptrace_read(parent);
+ if (can_read_parent) {
+ EXPECT_EQ(0, err_proc_read);
+ } else {
+ EXPECT_EQ(EACCES, err_proc_read);
+ }
+
+ /* Tests PTRACE_ATTACH on the parent. */
ret = ptrace(PTRACE_ATTACH, parent, NULL, 0);
- if (!can_trace_parent) {
+ if (can_trace_parent) {
+ EXPECT_EQ(0, ret);
+ } else {
EXPECT_EQ(-1, ret);
EXPECT_EQ(EPERM, errno);
- EXPECT_EQ(EACCES, err_proc_read);
- } else {
- EXPECT_EQ(0, ret);
- EXPECT_EQ(0, err_proc_read);
}
if (ret == 0) {
ASSERT_EQ(parent, waitpid(parent, &status, 0));
@@ -349,11 +356,11 @@ TEST_F(hierarchy, trace)
/* Tests child PTRACE_TRACEME. */
ret = ptrace(PTRACE_TRACEME);
- if (!can_trace_child) {
+ if (can_trace_child) {
+ EXPECT_EQ(0, ret);
+ } else {
EXPECT_EQ(-1, ret);
EXPECT_EQ(EPERM, errno);
- } else {
- EXPECT_EQ(0, ret);
}
/*
@@ -396,17 +403,23 @@ TEST_F(hierarchy, trace)
EXPECT_EQ(ESRCH, errno);
}
- /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the child. */
+ /* Tests PTRACE_MODE_READ on the child. */
err_proc_read = test_ptrace_read(child);
+ if (can_read_child) {
+ EXPECT_EQ(0, err_proc_read);
+ } else {
+ EXPECT_EQ(EACCES, err_proc_read);
+ }
+
+ /* Tests PTRACE_ATTACH on the child. */
ret = ptrace(PTRACE_ATTACH, child, NULL, 0);
if (!can_trace_child) {
EXPECT_EQ(-1, ret);
EXPECT_EQ(EPERM, errno);
- EXPECT_EQ(EACCES, err_proc_read);
} else {
EXPECT_EQ(0, ret);
- EXPECT_EQ(0, err_proc_read);
}
+
if (ret == 0) {
ASSERT_EQ(child, waitpid(child, &status, 0));
ASSERT_EQ(1, WIFSTOPPED(status));
@@ -419,11 +432,6 @@ TEST_F(hierarchy, trace)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests/landlock: Improve ptrace_test with Yama
2023-01-13 17:53 ` [PATCH] selftests/landlock: Improve ptrace_test with Yama Mickaël Salaün
@ 2023-01-13 18:12 ` Mickaël Salaün
2023-01-14 2:02 ` Jeff Xu
0 siblings, 1 reply; 5+ messages in thread
From: Mickaël Salaün @ 2023-01-13 18:12 UTC (permalink / raw)
To: Jeff Xu; +Cc: gnoack, groeck, jorgelo, keescook, linux-security-module
On 13/01/2023 18:53, Mickaël Salaün wrote:
> I found some issues running this new ptrace_test. Indeed, Yama doesn't
> restrict PTRACE_MODE_READ whereas Landlock does. I also changed some
> comments and socpe conditions. In fact, the final SKIP() wasn't a good
> idea because it masks such potential errors; let's only use a log
> message instead. I also removed the first SKIP() because we want
> everything to be tested, and the MODE_READ does still matter anyway.
>
> Please review this patch and squash it in yours.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20230113175308.50900-1-mic@digikod.net
> ---
> .../testing/selftests/landlock/ptrace_test.c | 106 ++++++++++--------
> 1 file changed, 57 insertions(+), 49 deletions(-)
>
> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> index c9d9f3001d0f..3c3ba37bf8ec 100644
> --- a/tools/testing/selftests/landlock/ptrace_test.c
> +++ b/tools/testing/selftests/landlock/ptrace_test.c
> @@ -19,7 +19,7 @@
>
> #include "common.h"
>
> -/* copy from yama_lsm.c */
> +/* Copied from security/yama/yama_lsm.c */
> #define YAMA_SCOPE_DISABLED 0
> #define YAMA_SCOPE_RELATIONAL 1
> #define YAMA_SCOPE_CAPABILITY 2
> @@ -70,7 +70,7 @@ static int get_yama_ptrace_scope(void)
> {
> int ret;
> char buf[2] = {};
> - int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
> + const int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
>
> if (fd < 0)
> return 0;
> @@ -260,46 +260,48 @@ TEST_F(hierarchy, trace)
> int yama_ptrace_scope;
> char buf_parent;
> long ret;
> - bool can_trace_child, can_trace_parent;
> + bool can_read_child, can_trace_child, can_read_parent, can_trace_parent;
>
> yama_ptrace_scope = get_yama_ptrace_scope();
> ASSERT_LE(0, yama_ptrace_scope);
>
> - if (yama_ptrace_scope >= YAMA_SCOPE_NO_ATTACH)
> - SKIP(return, "Yama forbids any ptrace use (scope %d)",
> - yama_ptrace_scope);
> + if (yama_ptrace_scope > YAMA_SCOPE_DISABLED)
> + TH_LOG("Incomplete tests due to Yama restrictions (scope %d)",
> + yama_ptrace_scope);
>
> /*
> - * 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_read_child is true if a parent process can read its child
> + * process, which is only the case when the parent process is not
> + * isolated from the child with a dedicated Landlock domain.
> */
> - can_trace_child = !variant->domain_parent &&
> - yama_ptrace_scope < YAMA_SCOPE_CAPABILITY;
> + can_read_child = !variant->domain_parent;
>
> /*
> - * 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.
> + * can_trace_child is true if a parent process can trace its child
> + * process. This depends on two conditions:
> + * - The parent process is not isolated from the child with a dedicated
> + * Landlock domain.
> + * - Yama allows tracing children (up to YAMA_SCOPE_RELATIONAL).
> */
> - can_trace_parent = !variant->domain_child &&
> - yama_ptrace_scope < YAMA_SCOPE_RELATIONAL;
> + can_trace_child = can_read_child &&
> + yama_ptrace_scope <= YAMA_SCOPE_RELATIONAL;
> +
> + /*
> + * can_read_parent is true if a child process can read its parent
> + * process, which is only the case when the child process is not
> + * isolated from the parent with a dedicated Landlock domain.
> + */
> + can_read_parent = !variant->domain_child;
> +
> + /*
> + * can_trace_parent is true if a child process can trace its parent
> + * process. This depends on two conditions:
> + * - The child process is not isolated from the parent with a dedicated
> + * Landlock domain.
> + * - Yama is disabled (YAMA_SCOPE_DISABLED).
> + */
> + can_trace_parent = can_read_parent &&
> + yama_ptrace_scope <= YAMA_SCOPE_DISABLED;
>
> /*
> * Removes all effective and permitted capabilities to not interfere
> @@ -330,16 +332,21 @@ TEST_F(hierarchy, trace)
> /* Waits for the parent to be in a domain, if any. */
> ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
>
> - /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the parent. */
> + /* Tests PTRACE_MODE_READ on the parent. */
> err_proc_read = test_ptrace_read(parent);
> + if (can_read_parent) {
> + EXPECT_EQ(0, err_proc_read);
> + } else {
> + EXPECT_EQ(EACCES, err_proc_read);
> + }
> +
> + /* Tests PTRACE_ATTACH on the parent. */
> ret = ptrace(PTRACE_ATTACH, parent, NULL, 0);
> - if (!can_trace_parent) {
> + if (can_trace_parent) {
> + EXPECT_EQ(0, ret);
> + } else {
> EXPECT_EQ(-1, ret);
> EXPECT_EQ(EPERM, errno);
> - EXPECT_EQ(EACCES, err_proc_read);
> - } else {
> - EXPECT_EQ(0, ret);
> - EXPECT_EQ(0, err_proc_read);
> }
> if (ret == 0) {
> ASSERT_EQ(parent, waitpid(parent, &status, 0));
> @@ -349,11 +356,11 @@ TEST_F(hierarchy, trace)
>
> /* Tests child PTRACE_TRACEME. */
> ret = ptrace(PTRACE_TRACEME);
> - if (!can_trace_child) {
> + if (can_trace_child) {
> + EXPECT_EQ(0, ret);
> + } else {
> EXPECT_EQ(-1, ret);
> EXPECT_EQ(EPERM, errno);
> - } else {
> - EXPECT_EQ(0, ret);
> }
>
> /*
> @@ -396,17 +403,23 @@ TEST_F(hierarchy, trace)
> EXPECT_EQ(ESRCH, errno);
> }
>
> - /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the child. */
> + /* Tests PTRACE_MODE_READ on the child. */
> err_proc_read = test_ptrace_read(child);
> + if (can_read_child) {
> + EXPECT_EQ(0, err_proc_read);
> + } else {
> + EXPECT_EQ(EACCES, err_proc_read);
> + }
> +
> + /* Tests PTRACE_ATTACH on the child. */
> ret = ptrace(PTRACE_ATTACH, child, NULL, 0);
> if (!can_trace_child) {
I forgot to inverse the !can_trace_child condition to make it more
consistent with the rest. Please update it for your next patch.
> EXPECT_EQ(-1, ret);
> EXPECT_EQ(EPERM, errno);
> - EXPECT_EQ(EACCES, err_proc_read);
> } else {
> EXPECT_EQ(0, ret);
> - EXPECT_EQ(0, err_proc_read);
> }
> +
> if (ret == 0) {
> ASSERT_EQ(child, waitpid(child, &status, 0));
> ASSERT_EQ(1, WIFSTOPPED(status));
> @@ -419,11 +432,6 @@ TEST_F(hierarchy, trace)
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selftests/landlock: Improve ptrace_test with Yama
2023-01-13 18:12 ` Mickaël Salaün
@ 2023-01-14 2:02 ` Jeff Xu
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Xu @ 2023-01-14 2:02 UTC (permalink / raw)
To: Mickaël Salaün
Cc: gnoack, groeck, jorgelo, keescook, linux-security-module
Done.
Thank you for double checking the read operation.
The test case is more complete with that.
-Jeff
On Fri, Jan 13, 2023 at 10:12 AM Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 13/01/2023 18:53, Mickaël Salaün wrote:
> > I found some issues running this new ptrace_test. Indeed, Yama doesn't
> > restrict PTRACE_MODE_READ whereas Landlock does. I also changed some
> > comments and socpe conditions. In fact, the final SKIP() wasn't a good
> > idea because it masks such potential errors; let's only use a log
> > message instead. I also removed the first SKIP() because we want
> > everything to be tested, and the MODE_READ does still matter anyway.
> >
> > Please review this patch and squash it in yours.
> >
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20230113175308.50900-1-mic@digikod.net
> > ---
> > .../testing/selftests/landlock/ptrace_test.c | 106 ++++++++++--------
> > 1 file changed, 57 insertions(+), 49 deletions(-)
> >
> > diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> > index c9d9f3001d0f..3c3ba37bf8ec 100644
> > --- a/tools/testing/selftests/landlock/ptrace_test.c
> > +++ b/tools/testing/selftests/landlock/ptrace_test.c
> > @@ -19,7 +19,7 @@
> >
> > #include "common.h"
> >
> > -/* copy from yama_lsm.c */
> > +/* Copied from security/yama/yama_lsm.c */
> > #define YAMA_SCOPE_DISABLED 0
> > #define YAMA_SCOPE_RELATIONAL 1
> > #define YAMA_SCOPE_CAPABILITY 2
> > @@ -70,7 +70,7 @@ static int get_yama_ptrace_scope(void)
> > {
> > int ret;
> > char buf[2] = {};
> > - int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
> > + const int fd = open("/proc/sys/kernel/yama/ptrace_scope", O_RDONLY);
> >
> > if (fd < 0)
> > return 0;
> > @@ -260,46 +260,48 @@ TEST_F(hierarchy, trace)
> > int yama_ptrace_scope;
> > char buf_parent;
> > long ret;
> > - bool can_trace_child, can_trace_parent;
> > + bool can_read_child, can_trace_child, can_read_parent, can_trace_parent;
> >
> > yama_ptrace_scope = get_yama_ptrace_scope();
> > ASSERT_LE(0, yama_ptrace_scope);
> >
> > - if (yama_ptrace_scope >= YAMA_SCOPE_NO_ATTACH)
> > - SKIP(return, "Yama forbids any ptrace use (scope %d)",
> > - yama_ptrace_scope);
> > + if (yama_ptrace_scope > YAMA_SCOPE_DISABLED)
> > + TH_LOG("Incomplete tests due to Yama restrictions (scope %d)",
> > + yama_ptrace_scope);
> >
> > /*
> > - * 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_read_child is true if a parent process can read its child
> > + * process, which is only the case when the parent process is not
> > + * isolated from the child with a dedicated Landlock domain.
> > */
> > - can_trace_child = !variant->domain_parent &&
> > - yama_ptrace_scope < YAMA_SCOPE_CAPABILITY;
> > + can_read_child = !variant->domain_parent;
> >
> > /*
> > - * 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.
> > + * can_trace_child is true if a parent process can trace its child
> > + * process. This depends on two conditions:
> > + * - The parent process is not isolated from the child with a dedicated
> > + * Landlock domain.
> > + * - Yama allows tracing children (up to YAMA_SCOPE_RELATIONAL).
> > */
> > - can_trace_parent = !variant->domain_child &&
> > - yama_ptrace_scope < YAMA_SCOPE_RELATIONAL;
> > + can_trace_child = can_read_child &&
> > + yama_ptrace_scope <= YAMA_SCOPE_RELATIONAL;
> > +
> > + /*
> > + * can_read_parent is true if a child process can read its parent
> > + * process, which is only the case when the child process is not
> > + * isolated from the parent with a dedicated Landlock domain.
> > + */
> > + can_read_parent = !variant->domain_child;
> > +
> > + /*
> > + * can_trace_parent is true if a child process can trace its parent
> > + * process. This depends on two conditions:
> > + * - The child process is not isolated from the parent with a dedicated
> > + * Landlock domain.
> > + * - Yama is disabled (YAMA_SCOPE_DISABLED).
> > + */
> > + can_trace_parent = can_read_parent &&
> > + yama_ptrace_scope <= YAMA_SCOPE_DISABLED;
> >
> > /*
> > * Removes all effective and permitted capabilities to not interfere
> > @@ -330,16 +332,21 @@ TEST_F(hierarchy, trace)
> > /* Waits for the parent to be in a domain, if any. */
> > ASSERT_EQ(1, read(pipe_parent[0], &buf_child, 1));
> >
> > - /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the parent. */
> > + /* Tests PTRACE_MODE_READ on the parent. */
> > err_proc_read = test_ptrace_read(parent);
> > + if (can_read_parent) {
> > + EXPECT_EQ(0, err_proc_read);
> > + } else {
> > + EXPECT_EQ(EACCES, err_proc_read);
> > + }
> > +
> > + /* Tests PTRACE_ATTACH on the parent. */
> > ret = ptrace(PTRACE_ATTACH, parent, NULL, 0);
> > - if (!can_trace_parent) {
> > + if (can_trace_parent) {
> > + EXPECT_EQ(0, ret);
> > + } else {
> > EXPECT_EQ(-1, ret);
> > EXPECT_EQ(EPERM, errno);
> > - EXPECT_EQ(EACCES, err_proc_read);
> > - } else {
> > - EXPECT_EQ(0, ret);
> > - EXPECT_EQ(0, err_proc_read);
> > }
> > if (ret == 0) {
> > ASSERT_EQ(parent, waitpid(parent, &status, 0));
> > @@ -349,11 +356,11 @@ TEST_F(hierarchy, trace)
> >
> > /* Tests child PTRACE_TRACEME. */
> > ret = ptrace(PTRACE_TRACEME);
> > - if (!can_trace_child) {
> > + if (can_trace_child) {
> > + EXPECT_EQ(0, ret);
> > + } else {
> > EXPECT_EQ(-1, ret);
> > EXPECT_EQ(EPERM, errno);
> > - } else {
> > - EXPECT_EQ(0, ret);
> > }
> >
> > /*
> > @@ -396,17 +403,23 @@ TEST_F(hierarchy, trace)
> > EXPECT_EQ(ESRCH, errno);
> > }
> >
> > - /* Tests PTRACE_ATTACH and PTRACE_MODE_READ on the child. */
> > + /* Tests PTRACE_MODE_READ on the child. */
> > err_proc_read = test_ptrace_read(child);
> > + if (can_read_child) {
> > + EXPECT_EQ(0, err_proc_read);
> > + } else {
> > + EXPECT_EQ(EACCES, err_proc_read);
> > + }
> > +
> > + /* Tests PTRACE_ATTACH on the child. */
> > ret = ptrace(PTRACE_ATTACH, child, NULL, 0);
> > if (!can_trace_child) {
>
> I forgot to inverse the !can_trace_child condition to make it more
> consistent with the rest. Please update it for your next patch.
>
>
> > EXPECT_EQ(-1, ret);
> > EXPECT_EQ(EPERM, errno);
> > - EXPECT_EQ(EACCES, err_proc_read);
> > } else {
> > EXPECT_EQ(0, ret);
> > - EXPECT_EQ(0, err_proc_read);
> > }
> > +
> > if (ret == 0) {
> > ASSERT_EQ(child, waitpid(child, &status, 0));
> > ASSERT_EQ(1, WIFSTOPPED(status));
> > @@ -419,11 +432,6 @@ TEST_F(hierarchy, trace)
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-14 2:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-13 5:07 [PATCH v5 0/1] selftests/landlock: fix ptrace_test jeffxu
2023-01-13 5:07 ` [PATCH v5 1/1] selftests/landlock: skip ptrace_test according to YAMA jeffxu
2023-01-13 17:53 ` [PATCH] selftests/landlock: Improve ptrace_test with Yama Mickaël Salaün
2023-01-13 18:12 ` Mickaël Salaün
2023-01-14 2:02 ` 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.