From: KP Singh <[email protected]>
The helper allows modification of certain bits on the linux_binprm
struct starting with the secureexec bit which can be updated using the
BPF_LSM_F_BPRM_SECUREEXEC flag.
secureexec can be set by the LSM for privilege gaining executions to set
the AT_SECURE auxv for glibc. When set, the dynamic linker disables the
use of certain environment variables (like LD_PRELOAD).
Signed-off-by: KP Singh <[email protected]>
---
include/uapi/linux/bpf.h | 14 ++++++++++++++
kernel/bpf/bpf_lsm.c | 27 +++++++++++++++++++++++++++
scripts/bpf_helpers_doc.py | 2 ++
tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
4 files changed, 57 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 162999b12790..7f1b6ba8246c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3787,6 +3787,14 @@ union bpf_attr {
* *ARG_PTR_TO_BTF_ID* of type *task_struct*.
* Return
* Pointer to the current task.
+ *
+ * long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
+ *
+ * Description
+ * Sets certain options on the *bprm*:
+ *
+ * **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
+ * which sets the **AT_SECURE** auxv for glibc.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -3948,6 +3956,7 @@ union bpf_attr {
FN(task_storage_get), \
FN(task_storage_delete), \
FN(get_current_task_btf), \
+ FN(lsm_set_bprm_opts), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
@@ -4119,6 +4128,11 @@ enum bpf_lwt_encap_mode {
BPF_LWT_ENCAP_IP,
};
+/* Flags for LSM helpers */
+enum {
+ BPF_LSM_F_BPRM_SECUREEXEC = (1ULL << 0),
+};
+
#define __bpf_md_ptr(type, name) \
union { \
type name; \
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 553107f4706a..31f85474a0ef 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -7,6 +7,7 @@
#include <linux/filter.h>
#include <linux/bpf.h>
#include <linux/btf.h>
+#include <linux/binfmts.h>
#include <linux/lsm_hooks.h>
#include <linux/bpf_lsm.h>
#include <linux/kallsyms.h>
@@ -51,6 +52,30 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
return 0;
}
+/* Mask for all the currently supported BPRM option flags */
+#define BPF_LSM_F_BRPM_OPTS_MASK 0x1ULL
+
+BPF_CALL_2(bpf_lsm_set_bprm_opts, struct linux_binprm *, bprm, u64, flags)
+{
+
+ if (flags & ~BPF_LSM_F_BRPM_OPTS_MASK)
+ return -EINVAL;
+
+ bprm->secureexec = (flags & BPF_LSM_F_BPRM_SECUREEXEC);
+ return 0;
+}
+
+BTF_ID_LIST_SINGLE(bpf_lsm_set_bprm_opts_btf_ids, struct, linux_binprm)
+
+const static struct bpf_func_proto bpf_lsm_set_bprm_opts_proto = {
+ .func = bpf_lsm_set_bprm_opts,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_BTF_ID,
+ .arg1_btf_id = &bpf_lsm_set_bprm_opts_btf_ids[0],
+ .arg2_type = ARG_ANYTHING,
+};
+
static const struct bpf_func_proto *
bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -71,6 +96,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_task_storage_get_proto;
case BPF_FUNC_task_storage_delete:
return &bpf_task_storage_delete_proto;
+ case BPF_FUNC_lsm_set_bprm_opts:
+ return &bpf_lsm_set_bprm_opts_proto;
default:
return tracing_prog_func_proto(func_id, prog);
}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 31484377b8b1..c5bc947a70ad 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -418,6 +418,7 @@ class PrinterHelpers(Printer):
'struct bpf_tcp_sock',
'struct bpf_tunnel_key',
'struct bpf_xfrm_state',
+ 'struct linux_binprm',
'struct pt_regs',
'struct sk_reuseport_md',
'struct sockaddr',
@@ -465,6 +466,7 @@ class PrinterHelpers(Printer):
'struct bpf_tcp_sock',
'struct bpf_tunnel_key',
'struct bpf_xfrm_state',
+ 'struct linux_binprm',
'struct pt_regs',
'struct sk_reuseport_md',
'struct sockaddr',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 162999b12790..7f1b6ba8246c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3787,6 +3787,14 @@ union bpf_attr {
* *ARG_PTR_TO_BTF_ID* of type *task_struct*.
* Return
* Pointer to the current task.
+ *
+ * long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
+ *
+ * Description
+ * Sets certain options on the *bprm*:
+ *
+ * **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
+ * which sets the **AT_SECURE** auxv for glibc.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -3948,6 +3956,7 @@ union bpf_attr {
FN(task_storage_get), \
FN(task_storage_delete), \
FN(get_current_task_btf), \
+ FN(lsm_set_bprm_opts), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
@@ -4119,6 +4128,11 @@ enum bpf_lwt_encap_mode {
BPF_LWT_ENCAP_IP,
};
+/* Flags for LSM helpers */
+enum {
+ BPF_LSM_F_BPRM_SECUREEXEC = (1ULL << 0),
+};
+
#define __bpf_md_ptr(type, name) \
union { \
type name; \
--
2.29.2.299.gdc1121823c-goog
From: KP Singh <[email protected]>
The test forks a child process, updates the local storage to set/unset
the securexec bit.
The BPF program in the test attaches to bprm_creds_for_exec which checks
the local storage of the current task to set the secureexec bit on the
binary parameters (bprm).
The child then execs a bash command with the environment variable
TMPDIR set in the envp. The bash command returns a different exit code
based on its observed value of the TMPDIR variable.
Since TMPDIR is one of the variables that is ignored by the dynamic
loader when the secureexec bit is set, one should expect the
child execution to not see this value when the secureexec bit is set.
Signed-off-by: KP Singh <[email protected]>
---
.../selftests/bpf/prog_tests/test_bprm_opts.c | 124 ++++++++++++++++++
tools/testing/selftests/bpf/progs/bprm_opts.c | 34 +++++
2 files changed, 158 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
create mode 100644 tools/testing/selftests/bpf/progs/bprm_opts.c
diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
new file mode 100644
index 000000000000..cba1ef3dc8b4
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#include <asm-generic/errno-base.h>
+#include <sys/stat.h>
+#include <test_progs.h>
+#include <linux/limits.h>
+
+#include "bprm_opts.skel.h"
+#include "network_helpers.h"
+
+#ifndef __NR_pidfd_open
+#define __NR_pidfd_open 434
+#endif
+
+static const char * const bash_envp[] = { "TMPDIR=shouldnotbeset", NULL };
+
+static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
+{
+ return syscall(__NR_pidfd_open, pid, flags);
+}
+
+static int update_storage(int map_fd, int secureexec)
+{
+ int task_fd, ret = 0;
+
+ task_fd = sys_pidfd_open(getpid(), 0);
+ if (task_fd < 0)
+ return errno;
+
+ ret = bpf_map_update_elem(map_fd, &task_fd, &secureexec, BPF_NOEXIST);
+ if (ret)
+ ret = errno;
+
+ close(task_fd);
+ return ret;
+}
+
+static int run_set_secureexec(int map_fd, int secureexec)
+{
+
+ int child_pid, child_status, ret, null_fd;
+
+ child_pid = fork();
+ if (child_pid == 0) {
+ null_fd = open("/dev/null", O_WRONLY);
+ if (null_fd == -1)
+ exit(errno);
+ dup2(null_fd, STDOUT_FILENO);
+ dup2(null_fd, STDERR_FILENO);
+ close(null_fd);
+
+ /* Ensure that all executions from hereon are
+ * secure by setting a local storage which is read by
+ * the bprm_creds_for_exec hook and sets bprm->secureexec.
+ */
+ ret = update_storage(map_fd, secureexec);
+ if (ret)
+ exit(ret);
+
+ /* If the binary is executed with securexec=1, the dynamic
+ * loader ingores and unsets certain variables like LD_PRELOAD,
+ * TMPDIR etc. TMPDIR is used here to simplify the example, as
+ * LD_PRELOAD requires a real .so file.
+ *
+ * If the value of TMPDIR is set, the bash command returns 10
+ * and if the value is unset, it returns 20.
+ */
+ ret = execle("/bin/bash", "bash", "-c",
+ "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20",
+ NULL, bash_envp);
+ if (ret)
+ exit(errno);
+ } else if (child_pid > 0) {
+ waitpid(child_pid, &child_status, 0);
+ ret = WEXITSTATUS(child_status);
+
+ /* If a secureexec occured, the exit status should be 20.
+ */
+ if (secureexec && ret == 20)
+ return 0;
+
+ /* If normal execution happened the exit code should be 10.
+ */
+ if (!secureexec && ret == 10)
+ return 0;
+
+ return ret;
+ }
+
+ return -EINVAL;
+}
+
+void test_test_bprm_opts(void)
+{
+ int err, duration = 0;
+ struct bprm_opts *skel = NULL;
+
+ skel = bprm_opts__open_and_load();
+ if (CHECK(!skel, "skel_load", "skeleton failed\n"))
+ goto close_prog;
+
+ err = bprm_opts__attach(skel);
+ if (CHECK(err, "attach", "attach failed: %d\n", err))
+ goto close_prog;
+
+ /* Run the test with the secureexec bit unset */
+ err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map),
+ 0 /* secureexec */);
+ if (CHECK(err, "run_set_secureexec:0", "err = %d", err))
+ goto close_prog;
+
+ /* Run the test with the secureexec bit set */
+ err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map),
+ 1 /* secureexec */);
+ if (CHECK(err, "run_set_secureexec:1", "err = %d", err))
+ goto close_prog;
+
+close_prog:
+ bprm_opts__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/bprm_opts.c b/tools/testing/selftests/bpf/progs/bprm_opts.c
new file mode 100644
index 000000000000..f353b4fdc0b7
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bprm_opts.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2020 Google LLC.
+ */
+
+#include "vmlinux.h"
+#include <errno.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+ __uint(type, BPF_MAP_TYPE_TASK_STORAGE);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, int);
+ __type(value, int);
+} secure_exec_task_map SEC(".maps");
+
+SEC("lsm/bprm_creds_for_exec")
+int BPF_PROG(secure_exec, struct linux_binprm *bprm)
+{
+ int *secureexec;
+
+ secureexec = bpf_task_storage_get(&secure_exec_task_map,
+ bpf_get_current_task_btf(), 0,
+ BPF_LOCAL_STORAGE_GET_F_CREATE);
+
+ if (secureexec && *secureexec)
+ bpf_lsm_set_bprm_opts(bprm, BPF_LSM_F_BPRM_SECUREEXEC);
+
+ return 0;
+}
--
2.29.2.299.gdc1121823c-goog
On Mon, Nov 16, 2020 at 11:25:35PM +0000, KP Singh wrote:
> From: KP Singh <[email protected]>
>
> The helper allows modification of certain bits on the linux_binprm
> struct starting with the secureexec bit which can be updated using the
> BPF_LSM_F_BPRM_SECUREEXEC flag.
>
> secureexec can be set by the LSM for privilege gaining executions to set
> the AT_SECURE auxv for glibc. When set, the dynamic linker disables the
> use of certain environment variables (like LD_PRELOAD).
>
> Signed-off-by: KP Singh <[email protected]>
> ---
> include/uapi/linux/bpf.h | 14 ++++++++++++++
> kernel/bpf/bpf_lsm.c | 27 +++++++++++++++++++++++++++
> scripts/bpf_helpers_doc.py | 2 ++
> tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
> 4 files changed, 57 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 162999b12790..7f1b6ba8246c 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3787,6 +3787,14 @@ union bpf_attr {
> * *ARG_PTR_TO_BTF_ID* of type *task_struct*.
> * Return
> * Pointer to the current task.
> + *
> + * long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
> + *
> + * Description
> + * Sets certain options on the *bprm*:
> + *
> + * **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
> + * which sets the **AT_SECURE** auxv for glibc.
The return value needs to be documented also.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -3948,6 +3956,7 @@ union bpf_attr {
> FN(task_storage_get), \
> FN(task_storage_delete), \
> FN(get_current_task_btf), \
> + FN(lsm_set_bprm_opts), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> @@ -4119,6 +4128,11 @@ enum bpf_lwt_encap_mode {
> BPF_LWT_ENCAP_IP,
> };
>
> +/* Flags for LSM helpers */
> +enum {
> + BPF_LSM_F_BPRM_SECUREEXEC = (1ULL << 0),
> +};
> +
> #define __bpf_md_ptr(type, name) \
> union { \
> type name; \
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 553107f4706a..31f85474a0ef 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -7,6 +7,7 @@
> #include <linux/filter.h>
> #include <linux/bpf.h>
> #include <linux/btf.h>
> +#include <linux/binfmts.h>
> #include <linux/lsm_hooks.h>
> #include <linux/bpf_lsm.h>
> #include <linux/kallsyms.h>
> @@ -51,6 +52,30 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> return 0;
> }
>
> +/* Mask for all the currently supported BPRM option flags */
> +#define BPF_LSM_F_BRPM_OPTS_MASK 0x1ULL
If there is a need to have v3, it will be better to use
BPF_LSM_F_BPRM_SECUREEXEC instead of 0x1ULL.
> +
> +BPF_CALL_2(bpf_lsm_set_bprm_opts, struct linux_binprm *, bprm, u64, flags)
> +{
> +
> + if (flags & ~BPF_LSM_F_BRPM_OPTS_MASK)
> + return -EINVAL;
> +
> + bprm->secureexec = (flags & BPF_LSM_F_BPRM_SECUREEXEC);
The intention of this helper is to set "or clear" a bit?
It may be useful to clarify the "clear" part in the doc also.
> + return 0;
> +}
> +
On Mon, Nov 16, 2020 at 11:25:36PM +0000, KP Singh wrote:
> From: KP Singh <[email protected]>
>
> The test forks a child process, updates the local storage to set/unset
> the securexec bit.
>
> The BPF program in the test attaches to bprm_creds_for_exec which checks
> the local storage of the current task to set the secureexec bit on the
> binary parameters (bprm).
>
> The child then execs a bash command with the environment variable
> TMPDIR set in the envp. The bash command returns a different exit code
> based on its observed value of the TMPDIR variable.
>
> Since TMPDIR is one of the variables that is ignored by the dynamic
> loader when the secureexec bit is set, one should expect the
> child execution to not see this value when the secureexec bit is set.
>
> Signed-off-by: KP Singh <[email protected]>
> ---
> .../selftests/bpf/prog_tests/test_bprm_opts.c | 124 ++++++++++++++++++
> tools/testing/selftests/bpf/progs/bprm_opts.c | 34 +++++
> 2 files changed, 158 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
> create mode 100644 tools/testing/selftests/bpf/progs/bprm_opts.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
> new file mode 100644
> index 000000000000..cba1ef3dc8b4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2020 Google LLC.
> + */
> +
> +#include <asm-generic/errno-base.h>
> +#include <sys/stat.h>
Is it needed?
> +#include <test_progs.h>
> +#include <linux/limits.h>
> +
> +#include "bprm_opts.skel.h"
> +#include "network_helpers.h"
> +
> +#ifndef __NR_pidfd_open
> +#define __NR_pidfd_open 434
> +#endif
> +
> +static const char * const bash_envp[] = { "TMPDIR=shouldnotbeset", NULL };
> +
> +static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
> +{
> + return syscall(__NR_pidfd_open, pid, flags);
> +}
> +
> +static int update_storage(int map_fd, int secureexec)
> +{
> + int task_fd, ret = 0;
> +
> + task_fd = sys_pidfd_open(getpid(), 0);
> + if (task_fd < 0)
> + return errno;
> +
> + ret = bpf_map_update_elem(map_fd, &task_fd, &secureexec, BPF_NOEXIST);
> + if (ret)
> + ret = errno;
> +
> + close(task_fd);
> + return ret;
> +}
> +
> +static int run_set_secureexec(int map_fd, int secureexec)
> +{
> +
> + int child_pid, child_status, ret, null_fd;
> +
> + child_pid = fork();
> + if (child_pid == 0) {
> + null_fd = open("/dev/null", O_WRONLY);
> + if (null_fd == -1)
> + exit(errno);
> + dup2(null_fd, STDOUT_FILENO);
> + dup2(null_fd, STDERR_FILENO);
> + close(null_fd);
> +
> + /* Ensure that all executions from hereon are
> + * secure by setting a local storage which is read by
> + * the bprm_creds_for_exec hook and sets bprm->secureexec.
> + */
> + ret = update_storage(map_fd, secureexec);
> + if (ret)
> + exit(ret);
> +
> + /* If the binary is executed with securexec=1, the dynamic
> + * loader ingores and unsets certain variables like LD_PRELOAD,
> + * TMPDIR etc. TMPDIR is used here to simplify the example, as
> + * LD_PRELOAD requires a real .so file.
> + *
> + * If the value of TMPDIR is set, the bash command returns 10
> + * and if the value is unset, it returns 20.
> + */
> + ret = execle("/bin/bash", "bash", "-c",
> + "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20",
> + NULL, bash_envp);
> + if (ret)
It should never reach here? May be just exit() unconditionally
instead of having a chance to fall-through and then return -EINVAL.
> + exit(errno);
> + } else if (child_pid > 0) {
> + waitpid(child_pid, &child_status, 0);
> + ret = WEXITSTATUS(child_status);
> +
> + /* If a secureexec occured, the exit status should be 20.
> + */
> + if (secureexec && ret == 20)
> + return 0;
> +
> + /* If normal execution happened the exit code should be 10.
> + */
> + if (!secureexec && ret == 10)
> + return 0;
> +
> + return ret;
Any chance that ret may be 0?
> + }
> +
> + return -EINVAL;
> +}
> +
> +void test_test_bprm_opts(void)
> +{
> + int err, duration = 0;
> + struct bprm_opts *skel = NULL;
> +
> + skel = bprm_opts__open_and_load();
> + if (CHECK(!skel, "skel_load", "skeleton failed\n"))
> + goto close_prog;
> +
> + err = bprm_opts__attach(skel);
> + if (CHECK(err, "attach", "attach failed: %d\n", err))
> + goto close_prog;
> +
> + /* Run the test with the secureexec bit unset */
> + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map),
> + 0 /* secureexec */);
> + if (CHECK(err, "run_set_secureexec:0", "err = %d", err))
nit. err = %d"\n"
> + goto close_prog;
> +
> + /* Run the test with the secureexec bit set */
> + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map),
> + 1 /* secureexec */);
> + if (CHECK(err, "run_set_secureexec:1", "err = %d", err))
Same here.
Others LGTM.
On Tue, Nov 17, 2020 at 1:11 AM Martin KaFai Lau <[email protected]> wrote:
>
> On Mon, Nov 16, 2020 at 11:25:35PM +0000, KP Singh wrote:
> > From: KP Singh <[email protected]>
> >
> > The helper allows modification of certain bits on the linux_binprm
> > struct starting with the secureexec bit which can be updated using the
> > BPF_LSM_F_BPRM_SECUREEXEC flag.
> >
> > secureexec can be set by the LSM for privilege gaining executions to set
> > the AT_SECURE auxv for glibc. When set, the dynamic linker disables the
> > use of certain environment variables (like LD_PRELOAD).
> >
> > Signed-off-by: KP Singh <[email protected]>
> > ---
> > include/uapi/linux/bpf.h | 14 ++++++++++++++
> > kernel/bpf/bpf_lsm.c | 27 +++++++++++++++++++++++++++
> > scripts/bpf_helpers_doc.py | 2 ++
> > tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
> > 4 files changed, 57 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 162999b12790..7f1b6ba8246c 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3787,6 +3787,14 @@ union bpf_attr {
> > * *ARG_PTR_TO_BTF_ID* of type *task_struct*.
> > * Return
> > * Pointer to the current task.
> > + *
> > + * long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
> > + *
> > + * Description
> > + * Sets certain options on the *bprm*:
> > + *
> > + * **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
> > + * which sets the **AT_SECURE** auxv for glibc.
> The return value needs to be documented also.
Done.
>
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -3948,6 +3956,7 @@ union bpf_attr {
> > FN(task_storage_get), \
> > FN(task_storage_delete), \
> > FN(get_current_task_btf), \
> > + FN(lsm_set_bprm_opts), \
> > /* */
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > @@ -4119,6 +4128,11 @@ enum bpf_lwt_encap_mode {
> > BPF_LWT_ENCAP_IP,
> > };
> >
> > +/* Flags for LSM helpers */
> > +enum {
> > + BPF_LSM_F_BPRM_SECUREEXEC = (1ULL << 0),
> > +};
> > +
> > #define __bpf_md_ptr(type, name) \
> > union { \
> > type name; \
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index 553107f4706a..31f85474a0ef 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -7,6 +7,7 @@
> > #include <linux/filter.h>
> > #include <linux/bpf.h>
> > #include <linux/btf.h>
> > +#include <linux/binfmts.h>
> > #include <linux/lsm_hooks.h>
> > #include <linux/bpf_lsm.h>
> > #include <linux/kallsyms.h>
> > @@ -51,6 +52,30 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > return 0;
> > }
> >
> > +/* Mask for all the currently supported BPRM option flags */
> > +#define BPF_LSM_F_BRPM_OPTS_MASK 0x1ULL
> If there is a need to have v3, it will be better to use
> BPF_LSM_F_BPRM_SECUREEXEC instead of 0x1ULL.
Done.
>
> > +
> > +BPF_CALL_2(bpf_lsm_set_bprm_opts, struct linux_binprm *, bprm, u64, flags)
> > +{
> > +
> > + if (flags & ~BPF_LSM_F_BRPM_OPTS_MASK)
> > + return -EINVAL;
> > +
> > + bprm->secureexec = (flags & BPF_LSM_F_BPRM_SECUREEXEC);
> The intention of this helper is to set "or clear" a bit?
> It may be useful to clarify the "clear" part in the doc also.
Updated the docs:
* long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
*
* Description
* Set or clear certain options on *bprm*:
*
* **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
* which sets the **AT_SECURE** auxv for glibc. The bit is
* is cleared if the flag is not specified.
* Return
* **-EINVAL** if invalid *flags* are passed.
>
> > + return 0;
> > +}
> > +
On Tue, Nov 17, 2020 at 1:43 AM Martin KaFai Lau <[email protected]> wrote:
>
> On Mon, Nov 16, 2020 at 11:25:36PM +0000, KP Singh wrote:
> > From: KP Singh <[email protected]>
> >
> > The test forks a child process, updates the local storage to set/unset
> > the securexec bit.
> >
> > The BPF program in the test attaches to bprm_creds_for_exec which checks
> > the local storage of the current task to set the secureexec bit on the
> > binary parameters (bprm).
> >
> > The child then execs a bash command with the environment variable
> > TMPDIR set in the envp. The bash command returns a different exit code
> > based on its observed value of the TMPDIR variable.
> >
> > Since TMPDIR is one of the variables that is ignored by the dynamic
> > loader when the secureexec bit is set, one should expect the
> > child execution to not see this value when the secureexec bit is set.
> >
> > Signed-off-by: KP Singh <[email protected]>
> > ---
> > .../selftests/bpf/prog_tests/test_bprm_opts.c | 124 ++++++++++++++++++
> > tools/testing/selftests/bpf/progs/bprm_opts.c | 34 +++++
> > 2 files changed, 158 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
> > create mode 100644 tools/testing/selftests/bpf/progs/bprm_opts.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
> > new file mode 100644
> > index 000000000000..cba1ef3dc8b4
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_bprm_opts.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright (C) 2020 Google LLC.
> > + */
> > +
> > +#include <asm-generic/errno-base.h>
> > +#include <sys/stat.h>
> Is it needed?
No, Good catch, removed.
>
> > +#include <test_progs.h>
[...]
> > + * If the value of TMPDIR is set, the bash command returns 10
> > + * and if the value is unset, it returns 20.
> > + */
> > + ret = execle("/bin/bash", "bash", "-c",
> > + "[[ -z \"${TMPDIR}\" ]] || exit 10 && exit 20",
> > + NULL, bash_envp);
> > + if (ret)
> It should never reach here? May be just exit() unconditionally
> instead of having a chance to fall-through and then return -EINVAL.
Agreed. changed it to exit(errno); here.
>
> > + exit(errno);
> > + } else if (child_pid > 0) {
> > + waitpid(child_pid, &child_status, 0);
> > + ret = WEXITSTATUS(child_status);
> > +
> > + /* If a secureexec occured, the exit status should be 20.
> > + */
> > + if (secureexec && ret == 20)
> > + return 0;
> > +
> > + /* If normal execution happened the exit code should be 10.
> > + */
> > + if (!secureexec && ret == 10)
> > + return 0;
> > +
> > + return ret;
> Any chance that ret may be 0?
I think it's safer to just let it fall through and return -EINVAL, so
I removed the return ret here.
>
> > + }
[...]
> > + 0 /* secureexec */);
> > + if (CHECK(err, "run_set_secureexec:0", "err = %d", err))
> nit. err = %d"\n"
Fixed.
>
> > + goto close_prog;
> > +
> > + /* Run the test with the secureexec bit set */
> > + err = run_set_secureexec(bpf_map__fd(skel->maps.secure_exec_task_map),
> > + 1 /* secureexec */);
> > + if (CHECK(err, "run_set_secureexec:1", "err = %d", err))
> Same here.
Fixed.
- KP
>
> Others LGTM.
On Tue, Nov 17, 2020 at 3:03 AM KP Singh <[email protected]> wrote:
>
> On Tue, Nov 17, 2020 at 1:11 AM Martin KaFai Lau <[email protected]> wrote:
> >
> > On Mon, Nov 16, 2020 at 11:25:35PM +0000, KP Singh wrote:
> > > From: KP Singh <[email protected]>
> > >
> > > The helper allows modification of certain bits on the linux_binprm
> > > struct starting with the secureexec bit which can be updated using the
> > > BPF_LSM_F_BPRM_SECUREEXEC flag.
> > >
> > > secureexec can be set by the LSM for privilege gaining executions to set
> > > the AT_SECURE auxv for glibc. When set, the dynamic linker disables the
> > > use of certain environment variables (like LD_PRELOAD).
> > >
> > > Signed-off-by: KP Singh <[email protected]>
> > > ---
> > > include/uapi/linux/bpf.h | 14 ++++++++++++++
> > > kernel/bpf/bpf_lsm.c | 27 +++++++++++++++++++++++++++
> > > scripts/bpf_helpers_doc.py | 2 ++
> > > tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
> > > 4 files changed, 57 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 162999b12790..7f1b6ba8246c 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -3787,6 +3787,14 @@ union bpf_attr {
> > > * *ARG_PTR_TO_BTF_ID* of type *task_struct*.
> > > * Return
> > > * Pointer to the current task.
> > > + *
> > > + * long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
> > > + *
> > > + * Description
> > > + * Sets certain options on the *bprm*:
> > > + *
> > > + * **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
> > > + * which sets the **AT_SECURE** auxv for glibc.
> > The return value needs to be documented also.
>
> Done.
>
> >
> > > */
> > > #define __BPF_FUNC_MAPPER(FN) \
> > > FN(unspec), \
> > > @@ -3948,6 +3956,7 @@ union bpf_attr {
> > > FN(task_storage_get), \
> > > FN(task_storage_delete), \
> > > FN(get_current_task_btf), \
> > > + FN(lsm_set_bprm_opts), \
> > > /* */
> > >
> > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > @@ -4119,6 +4128,11 @@ enum bpf_lwt_encap_mode {
> > > BPF_LWT_ENCAP_IP,
> > > };
> > >
> > > +/* Flags for LSM helpers */
> > > +enum {
> > > + BPF_LSM_F_BPRM_SECUREEXEC = (1ULL << 0),
> > > +};
> > > +
> > > #define __bpf_md_ptr(type, name) \
> > > union { \
> > > type name; \
> > > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > > index 553107f4706a..31f85474a0ef 100644
> > > --- a/kernel/bpf/bpf_lsm.c
> > > +++ b/kernel/bpf/bpf_lsm.c
> > > @@ -7,6 +7,7 @@
> > > #include <linux/filter.h>
> > > #include <linux/bpf.h>
> > > #include <linux/btf.h>
> > > +#include <linux/binfmts.h>
> > > #include <linux/lsm_hooks.h>
> > > #include <linux/bpf_lsm.h>
> > > #include <linux/kallsyms.h>
> > > @@ -51,6 +52,30 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > > return 0;
> > > }
> > >
> > > +/* Mask for all the currently supported BPRM option flags */
> > > +#define BPF_LSM_F_BRPM_OPTS_MASK 0x1ULL
> > If there is a need to have v3, it will be better to use
> > BPF_LSM_F_BPRM_SECUREEXEC instead of 0x1ULL.
>
> Done.
>
> >
> > > +
> > > +BPF_CALL_2(bpf_lsm_set_bprm_opts, struct linux_binprm *, bprm, u64, flags)
> > > +{
> > > +
> > > + if (flags & ~BPF_LSM_F_BRPM_OPTS_MASK)
> > > + return -EINVAL;
> > > +
> > > + bprm->secureexec = (flags & BPF_LSM_F_BPRM_SECUREEXEC);
> > The intention of this helper is to set "or clear" a bit?
> > It may be useful to clarify the "clear" part in the doc also.
>
> Updated the docs:
>
> * long bpf_lsm_set_bprm_opts(struct linux_binprm *bprm, u64 flags)
> *
> * Description
> * Set or clear certain options on *bprm*:
> *
> * **BPF_LSM_F_BPRM_SECUREEXEC** Set the secureexec bit
> * which sets the **AT_SECURE** auxv for glibc. The bit is
> * is cleared if the flag is not specified.
(-is) = cleared if the flag is not specified. (Thanks checkpatch!)
> * Return
> * **-EINVAL** if invalid *flags* are passed.
>
> >
> > > + return 0;
> > > +}
> > > +