[PATCH] selftests/landlock: skip ptrace_test when YAMA is enabled

[Mickaël Salaün]

Hi Jeff,

Thanks for this patch. Here are some comments:

On 29/06/2022 00:29, Jeff Xu wrote:
> ptrace_test assumes YAMA is disabled, skip it if YAMA is enabled.
> 
> Cc: Jorge Lucangeli Obes <jorgelo at chromium.org>
> Cc: Guenter Roeck <groeck at chromium.org>
> Cc: Kees Cook <keescook at chromium.org>
> Cc: Mickaël Salaün <mic at digikod.net>
> Tested-by: Jeff Xu <jeffxu at google.com>
> Signed-off-by: Jeff Xu <jeffxu at google.com>
> Change-Id: I623742ca9f20ec706a38c92f6c0bab755f73578f
> ---
>   .../testing/selftests/landlock/ptrace_test.c  | 49 +++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/ptrace_test.c b/tools/testing/selftests/landlock/ptrace_test.c
> index c28ef98ff3ac..ef2d36f56764 100644
> --- a/tools/testing/selftests/landlock/ptrace_test.c
> +++ b/tools/testing/selftests/landlock/ptrace_test.c
> @@ -226,6 +226,44 @@ FIXTURE_TEARDOWN(hierarchy)
>   {
>   }
>   

Please move these new helpers after test_ptrace_read() and make them static.

> +int open_sysfs(const char *path, int flags, int *fd)
> +{
> +	*fd = open(path, flags);
> +
> +	if (fd < 0)
> +		return -1;
> +
> +	return 0;
> +}

open_sysfs() can be replaced with a call to open(). This makes the code 
simpler.

> +
> +int read_sysfs_int_fd(int fd, int *val)
> +{
> +	char buf[2];
> +
> +	if (read(fd, buf, sizeof(buf)) < 0)

I guess `read(fd, buf, 1)` should be enough and it enables keeping the 
final '\0'. A comment should state that this helper only read the first 
digit (which is enough for Yama).

> +		return -1;
> +
> +	buf[sizeof(buf) - 1] = '\0';

Use `char buf[2] = {};` instead.

> +	*val = atoi(buf);
> +	return 0;
> +}

Same for read_sysfs_int_fd(), you can inline the code in read_sysfs_int().

> +
> +int read_sysfs_int(const char *path, int *val)
> +{
> +	int fd;
> +
> +	if (open_sysfs(path, O_RDONLY, &fd) != 0)
> +		return -1;
> +
> +	if (read_sysfs_int_fd(fd, val) != 0) {
> +		close(fd);
> +		return -1;
> +	}
> +
> +	close(fd);
> +	return 0;
> +}
> +
>   /* Test PTRACE_TRACEME and PTRACE_ATTACH for parent and child. */
>   TEST_F(hierarchy, trace)
>   {
> @@ -235,6 +273,17 @@ TEST_F(hierarchy, trace)
>   	char buf_parent;
>   	long ret;
>   
> +	int ptrace_val;
> +
> +	ASSERT_EQ(0, read_sysfs_int("/proc/sys/kernel/yama/ptrace_scope",
> +				    &ptrace_val));

This is a good test but it fail if Yama is not built in the kernel.

For now, I think you can create two helpers named something like 
is_yama_restricting() and is_yama_denying() (for admin-only attach).

> +	if (ptrace_val != 0) {

Some tests should work even if ptrace_val == 1. SKIP() should only be 
called when the test would fail. Can you please check all tests with all 
Yama values?

> +		/*
> +		 * Yama's scoped ptrace is presumed disabled.  If enabled, skip.
> +		 */
> +		SKIP(return, "yama is enabled, skip current test");
> +	}
> +
>   	/*
>   	 * Removes all effective and permitted capabilities to not interfere
>   	 * with cap_ptrace_access_check() in case of PTRACE_MODE_FSCREDS.