Re: [PATCH bpf-next v2 3/3] selftests/bpf: Test linux_binprm user memory kfuncs

[bot+bpf-ci]

> selftests/bpf: Test linux_binprm user memory kfuncs
>
> This patch adds a sleepable BPF LSM program attached to
> bprm_check_security to test bpf_copy_from_user_bprm() and
> bpf_copy_from_user_bprm_str().

This isn't a bug, but could the changelogs in this series use the
imperative mood the kernel asks for, e.g. "Add a sleepable BPF LSM
program attached to bprm_check_security ..." instead of "This patch
adds ..."?

> diff --git a/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
> new file mode 100644
> index 000000000000..1007e3aec054
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/copy_from_user_bprm.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <errno.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include <test_progs.h>
> +
> +#include "copy_from_user_bprm.skel.h"
> +
> +void test_copy_from_user_bprm(void)
> +{
> +	char arg0[] = "first";
> +	char arg1[] = "second-argument";
> +	char env0[] = "SOME_ENV=a";
> +	char env1[] = "OTHER_ENV=something";
> +	struct copy_from_user_bprm *skel;
> +	pid_t child;
> +	int status;
> +
> +	skel = copy_from_user_bprm__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +
> +	if (!ASSERT_OK(copy_from_user_bprm__attach(skel), "attach"))
> +		goto out;
> +
> +	child = fork();
> +	if (!ASSERT_GE(child, 0, "fork"))
> +		goto out;
> +
> +	if (!child) {
> +		char *const argv[] = { arg0, arg1, NULL };
> +		char *const envp[] = { env0, env1, NULL };
> +
> +		skel->bss->monitored_pid = getpid();
> +		execve("/bin/true", argv, envp);
> +		_exit(errno);
> +	}
> +
> +	if (!ASSERT_EQ(waitpid(child, &status, 0), child, "waitpid"))
> +		goto out;
> +
> +	if (ASSERT_TRUE(WIFEXITED(status), "child_exited"))
> +		ASSERT_EQ(WEXITSTATUS(status), EPERM, "exec_errno");
> +
> +	ASSERT_EQ(skel->bss->bprm_argc, 2, "bprm_argc");
> +	ASSERT_EQ(skel->bss->bprm_envc, 2, "bprm_envc");
> +	ASSERT_EQ(skel->bss->data_len_match, 1, "data_len_match");
> +	ASSERT_EQ(skel->bss->invalid_flags_ret, -EINVAL, "invalid_flags_ret");
> +	ASSERT_EQ(skel->bss->copy_ret, 0, "copy_ret");
> +	ASSERT_EQ(skel->bss->str_arg0_ret, sizeof(arg0), "str_arg0_ret");
> +	ASSERT_EQ(skel->bss->str_arg1_ret, sizeof(arg1), "str_arg1_ret");
> +	ASSERT_EQ(skel->bss->str_env0_ret, sizeof(env0), "str_env0_ret");
> +	ASSERT_EQ(skel->bss->str_env1_ret, sizeof(env1), "str_env1_ret");

A subsystem pattern flags this as potentially concerning: the test has
no skip path for the configuration in which the kfuncs it exercises are
deliberately unavailable.

The immediately preceding commit in this series (6073b8df0008, "bpf: Add
user memory access kfuncs for linux_binprm") registers both kfuncs on
NOMMU as well, where they are documented and implemented to return
-EOPNOTSUPP:

fs/bpf_fs_kfuncs.c, bpf_copy_from_user_bprm():
  #else
      memset(dst, 0, dst__sz);
      return -EOPNOTSUPP;
  #endif

fs/bpf_fs_kfuncs.c, bpf_copy_from_user_bprm_str():
  #else
      if (flags & BPF_F_PAD_ZEROS)
          memset(dst, 0, dst__sz);
      else
          *(char *)dst = '\0';
      return -EOPNOTSUPP;
  #endif

The changelog of that commit states this explicitly: "On NOMMU systems,
exec arguments are staged in bprm->page[] rather than mapped in bprm->mm,
so the kfuncs return -EOPNOTSUPP."

The BPF program records those return values verbatim into copy_ret,
str_arg0_ret, str_arg1_ret, str_env0_ret, and str_env1_ret
(progs/copy_from_user_bprm.c), and the userspace half then asserts the
success values unconditionally, plus data_len_match, data_match,
str_args_match, and str_envs_match. On a !CONFIG_MMU kernel every one of
those assertions fails and the test is reported as a regression rather
than skipped. Note that invalid_flags_ret (the assertion above) is the
one assertion that would still pass, because the flags check precedes the
#ifdef CONFIG_MMU block.

Would it make sense to use test__skip() here with a reason string when
the kfuncs return -EOPNOTSUPP? test_progs provides test__skip() for
exactly this (123 existing call sites under
tools/testing/selftests/bpf/prog_tests/).

> +	ASSERT_EQ(skel->bss->data_match, 1, "data_match");
> +	ASSERT_EQ(skel->bss->str_args_match, 1, "str_args_match");
> +	ASSERT_EQ(skel->bss->str_envs_match, 1, "str_envs_match");
> +
> +out:
> +	copy_from_user_bprm__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
> new file mode 100644
> index 000000000000..4d7e9002b45b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/copy_from_user_bprm.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include "vmlinux.h"
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <errno.h>
> +#include "bpf_misc.h"

This isn't a bug, but is "bpf_misc.h" needed here? I could not spot
anything in the program that comes from it, so the include may be
droppable.

> +
> +char _license[] SEC("license") = "GPL";
> +
> +static const char expected_data[] = "first\0second-argument\0"
> +				    "SOME_ENV=a\0OTHER_ENV=something";

[ ... ]

> +SEC("lsm.s/bprm_check_security")
> +int BPF_PROG(check_exec_args, struct linux_binprm *bprm)
> +{
> +	u32 pid = bpf_get_current_pid_tgid() >> 32;
> +	char data[sizeof(expected_data)];
> +	char arg0[32];
> +	char arg1[32];
> +	char env0[32];
> +	char env1[32];
> +	u64 offset = 0;
> +	u64 data_len;
> +
> +	if (pid != monitored_pid)
> +		return 0;
> +
> +	bprm_argc = bprm->argc;
> +	bprm_envc = bprm->envc;
> +
> +	/* this is the total size of args and envs starting from bprm->p */
> +	data_len = bprm->exec - bprm->p;
> +	data_len_match = data_len == sizeof(expected_data);
> +
> +	invalid_flags_ret = bpf_copy_from_user_bprm(data,
> +						    sizeof(data), (void *)bprm->p, bprm, ~0ULL);
> +
> +	copy_ret = bpf_copy_from_user_bprm(data, sizeof(data), (void *)bprm->p,
> +					   bprm, 0);
> +	if (copy_ret)
> +		return 0;
> +
> +	data_match =
> +		!__builtin_memcmp(data, expected_data, sizeof(expected_data));
> +
> +	/* arg0 is at bprm->p */
> +	str_arg0_ret = bpf_copy_from_user_bprm_str(arg0, sizeof(arg0),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);
> +	if (str_arg0_ret != sizeof(expected_arg0))
> +		return 0;
> +	offset += str_arg0_ret;
> +
> +	/* arg1 is at bprm->p + sizeof(arg0) */
> +	str_arg1_ret = bpf_copy_from_user_bprm_str(arg1, sizeof(arg1),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);

This isn't a bug, but would these comments read more precisely as
something like "arg1 follows arg0's NUL terminator", or by referring to
the expected_* strings? In this scope arg0, arg1, env0 are the 32-byte
destination buffers, so sizeof(arg0) here is the 32-byte destination
buffer rather than the string length. The code actually advances offset
by the value the kfunc returned (6, 22, 33).

> +	if (str_arg1_ret != sizeof(expected_arg1))
> +		return 0;
> +	offset += str_arg1_ret;
> +
> +	/* env0 is at bprm->p + sizeof(arg0) + sizeof(arg1) */
> +	str_env0_ret = bpf_copy_from_user_bprm_str(env0, sizeof(env0),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);
> +	if (str_env0_ret != sizeof(expected_env0))
> +		return 0;
> +	offset += str_env0_ret;
> +
> +	/* env1 is at bprm->p + sizeof(arg0) + sizeof(arg1) + sizeof(env0) */
> +	str_env1_ret = bpf_copy_from_user_bprm_str(env1, sizeof(env1),
> +						   (void *)(bprm->p + offset),
> +						   bprm, BPF_F_PAD_ZEROS);
> +	if (str_env1_ret != sizeof(expected_env1))
> +		return 0;
> +
> +	str_args_match =
> +		!__builtin_memcmp(arg0, expected_arg0, sizeof(expected_arg0)) &&
> +		!__builtin_memcmp(arg1, expected_arg1, sizeof(expected_arg1));
> +	str_envs_match =
> +		!__builtin_memcmp(env0, expected_env0, sizeof(expected_env0)) &&
> +		!__builtin_memcmp(env1, expected_env1, sizeof(expected_env1));
> +
> +	return data_match && str_args_match && str_envs_match ? -EPERM : 0;
> +}


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32375104040