Re: [PATCH v2 1/1] selftests/landlock: skip ptrace_test according to YAMA

["Mickaël Salaün" <mic@digikod.net>]

This is much better! We can tailor a bit more the tests though.

On 13/12/2022 19:58, 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  | 34 +++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> index c28ef98ff3ac..8565a25a9587 100644
> --- a/tools/testing/selftests/landlock/ptrace_test.c
> +++ b/tools/testing/selftests/landlock/ptrace_test.c
> @@ -60,6 +60,24 @@ static int test_ptrace_read(const pid_t pid)
>   	return 0;
>   }
>   
> +static int get_ptrace_scope(void)

Please rename to get_yama_ptrace_scope().

> +{
> +	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;
> +
> +	buf[1] = '\0';

You can replace that with `char buf[2] = {};`


> +	ret = atoi(buf);
> +	close(fd);
> +	return ret;
> +}
> +
>   /* clang-format off */
>   FIXTURE(hierarchy) {};
>   /* clang-format on */
> @@ -69,6 +87,7 @@ FIXTURE_VARIANT(hierarchy)
>   	const bool domain_both;
>   	const bool domain_parent;
>   	const bool domain_child;
> +	const int  yama_value;

Please rename to yama_ptrace_scope_max and remove the extra space.

>   };
>   
>   /*
> @@ -93,6 +112,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_without_domain) {
>   	.domain_both = false,
>   	.domain_parent = false,
>   	.domain_child = false,
> +	.yama_value = 0,
>   };
>   
>   /*
> @@ -110,6 +130,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_with_one_domain) {
>   	.domain_both = false,
>   	.domain_parent = false,
>   	.domain_child = true,
> +	.yama_value = 1,
>   };
>   
>   /*
> @@ -126,6 +147,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_parent_domain) {
>   	.domain_both = false,
>   	.domain_parent = true,
>   	.domain_child = false,
> +	.yama_value = 0,
>   };
>   
>   /*
> @@ -143,6 +165,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_sibling_domain) {
>   	.domain_both = false,
>   	.domain_parent = true,
>   	.domain_child = true,
> +	.yama_value = 2,
>   };
>   
>   /*
> @@ -160,6 +183,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_sibling_domain) {
>   	.domain_both = true,
>   	.domain_parent = false,
>   	.domain_child = false,
> +	.yama_value = 0,
>   };
>   
>   /*
> @@ -178,6 +202,7 @@ FIXTURE_VARIANT_ADD(hierarchy, allow_with_nested_domain) {
>   	.domain_both = true,
>   	.domain_parent = false,
>   	.domain_child = true,
> +	.yama_value = 1,
>   };
>   
>   /*
> @@ -196,6 +221,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_nested_and_parent_domain) {
>   	.domain_both = true,
>   	.domain_parent = true,
>   	.domain_child = false,
> +	.yama_value = 0,
>   };
>   
>   /*
> @@ -216,6 +242,7 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_forked_domain) {
>   	.domain_both = true,
>   	.domain_parent = true,
>   	.domain_child = true,
> +	.yama_value = 2,
>   };
>   
>   FIXTURE_SETUP(hierarchy)
> @@ -232,9 +259,16 @@ TEST_F(hierarchy, trace)
>   	pid_t child, parent;
>   	int status, err_proc_read;
>   	int pipe_child[2], pipe_parent[2];
> +	int yama;

Please rename to yama_ptrace_scope.


>   	char buf_parent;
>   	long ret;
>   
> +	yama = get_ptrace_scope();
> +	ASSERT_LE(0, yama);
> +
> +	if (variant->yama_value < yama)

if (yama_ptrace_scope >= 3)

> +		SKIP(return, "unsupported yama value %d", yama);

"Yama forbids any ptrace use (scope 3)"


This check skips the whole test, whereas the issues with Yama are about:
- a child process tracing its parent;
- the PTRACE_TRACEME case.

I think the main remaining parts to change is the `if 
(variant->domain_*` checks to extend with the yama_ptrace_scope_max check.

However, it is useful to highlight that a test didn't fully covered 
Landlock checks. I think the best approach is to call SKIP() at the end 
of TEST_F(hierarchy, trace) if yama_ptrace_scope >= 1 . This way, we 
test as much as possible (Landlock and Yama) and we mark the "tampered" 
tests as skipped.


> +
>   	/*
>   	 * Removes all effective and permitted capabilities to not interfere
>   	 * with cap_ptrace_access_check() in case of PTRACE_MODE_FSCREDS.