[PATCH] selftests/landlock: Improve ptrace_test with Yama
Mickaël Salaün
mic at digikod.net
Fri Jan 13 18:12:09 UTC 2023
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 at 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
More information about the Linux-security-module-archive
mailing list