[RFC v1 00/14] Kernel Runtime Security Instrumentation

[RFC v1 00/14] Kernel Runtime Security Instrumentation

[KP Singh]

From: KP Singh <kpsingh@google.com>

# Motivation

Signaling and mitigation are two key aspects of security which go
hand-in-hand.  Signals provide the necessary context to narrow down a
malicious actor and are key to creating effective MAC policies to
mitigate/prevent the malicious actor.

One can obtain signals from the kernel by using the audit infrastructure.
System-wide MAC is done separately as a part of the LSMs (eg. SELinux,
AppArmor).

Augmenting the signal information provided by audit requires kernel
changes to audit, its policy language and user-space components.
Furthermore, building a MAC policy based on the newly added signal
requires changes to various LSMs and their own respective policy
languages.

KRSI attempts to solve this problem by providing a common policy API in
the form of security focussed eBPF helpers and a common surface for
creating dynamic (not requiring re-compilation of the kernel) MAC and
Audit policies by attaching eBPF programs to the various LSM hooks.


# Why an LSM?

Linux Security Modules target security behaviours rather than the API. For
example, it's easy to miss out a newly added system call for executing
processes (eg. execve, execveat etc.) while the LSM framework ensures that
all process executions trigger the relevant hooks irrespective of how the
process was executed.

Allowing users to attach eBPF programs to LSM hooks also benefits the LSM
eco-system by enabling feedback from the security community about the kind
of behaviours that the LSM should be targeting.


# How does it work?

NOTE: The cover letter will be assimilated into the documentation
(Documentation/security/) as a part of the patch series.

## SecurityFS Interface

KRSI hooks create a file in securityfs to which eBPF programs can be
attached.  This file maps to a single LSM hook but adds a layer of
indirection thus shielding the user-space from any changes to the
underlying hook. This includes changes like renaming of the underlying
hook or replacing the hook with another that maps better to the security
behaviour represented.

For Example:

	/sys/kernel/security/krsi/process_execution -> bprm_check_security

## eBPF Program / Helpers

In order to keep things simple for the user, KRSI intends to provide one
eBPF program type. Since, there is only one type of context for a given
eBPF program type, the members of the KRSI context are selectively
populated depending on the hook.

KRSI is conservative about the access into the context and expects users
to use the helpers as an API for getting the required information. This
helps limit the internals of the LSM exposed to the user and maintain
backward compatibility. It also allows the maintainers to update the
structure of the context without worrying about breaking user-space.

The intention is to keep the helpers precise and not depend on kernel
headers or internals strengthening the backward compatibility and
usability across a large and diverse fleet.

Precise helpers also help in tackling seemingly intractable problems. For
example, a helper to dump all environment variables is hard because the
environment variables can be 32 pages long. While a helper to only extract
certain relevant environment variables (e.g. LD_PRELOAD, HISTFILESIZE)
helps in building a more reliable and precise signal with lower overhead.

## Attachment

A privileged (CAP_SYS_ADMIN for loading / attaching eBPF programs)
user-space program opens the securityfs file and the eBPF program
(BPF_PROG_LOAD) and attaches (BPF_PROG_ATTACH) the eBPF program to the
hook.

	hook_fd = open("/sys/kernel/security/krsi/process_execution", O_RDWR) 
	prog_fd = bpf(BPF_PROG_LOAD, ...) 
	bpf(BPF_PROG_ATTACH, hook_fd, prog_fd)

There can be more than one program attached to a hook and attaching a
program with the same name replaces the existing program. The user can see
the programs attached to the hook as follows:

	cat /sys/kernel/security/krsi/process_execution 
	env_dumper
	my_auditor

## Audit / MAC Policy

The userspace controls the policy by installing eBPF programs to the LSM
hook.  If any of the attached eBPF programs return an error (-ENOPERM),
the action represented by the hook is denied.

The audit logs are written using a format chosen by the eBPF program to
the perf events buffer (using bpf_perf_event_output) and can be further
processed in user-space.


# Current Status

The RFC provides a proto-type implementation with a few helpers and a
sample program samples/bpf/krsi_kern.c that hooks into process execution
and logs specific environment variables to the perf events buffer.

The user-space program samples/bpf/krsi_user.c loads the eBPF program,
configures the environment variable that needs to be audited and listens
for perf events.


KP Singh (14):
  krsi: Add a skeleton and config options for the KRSI LSM
  krsi: Introduce types for KRSI eBPF
  bpf: krsi: sync BPF UAPI header with tools
  krsi: Add support in libbpf for BPF_PROG_TYPE_KRSI
  krsi: Initialize KRSI hooks and create files in securityfs
  krsi: Implement eBPF operations, attachment and execution
  krsi: Check for premissions on eBPF attachment
  krsi: Show attached program names in hook read handler.
  krsi: Add a helper function for bpf_perf_event_output
  krsi: Handle attachment of the same program
  krsi: Pin argument pages in bprm_check_security hook
  krsi: Add an eBPF helper function to get the value of an env variable
  krsi: Provide an example to read and log environment variables
  krsi: Pin arg pages only when needed

 MAINTAINERS                               |   8 +
 include/linux/bpf_types.h                 |   3 +
 include/linux/krsi.h                      |  19 ++
 include/uapi/linux/bpf.h                  |  44 ++-
 kernel/bpf/syscall.c                      |   7 +
 samples/bpf/.gitignore                    |   1 +
 samples/bpf/Makefile                      |   3 +
 samples/bpf/krsi_helpers.h                |  31 ++
 samples/bpf/krsi_kern.c                   |  52 ++++
 samples/bpf/krsi_user.c                   | 202 +++++++++++++
 security/Kconfig                          |   1 +
 security/Makefile                         |   2 +
 security/krsi/Kconfig                     |  22 ++
 security/krsi/Makefile                    |   3 +
 security/krsi/include/hooks.h             |  22 ++
 security/krsi/include/krsi_fs.h           |  19 ++
 security/krsi/include/krsi_init.h         | 106 +++++++
 security/krsi/krsi.c                      | 157 ++++++++++
 security/krsi/krsi_fs.c                   | 190 ++++++++++++
 security/krsi/ops.c                       | 342 ++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h            |  44 ++-
 tools/lib/bpf/libbpf.c                    |   4 +
 tools/lib/bpf/libbpf.h                    |   2 +
 tools/lib/bpf/libbpf.map                  |   2 +
 tools/lib/bpf/libbpf_probes.c             |   1 +
 tools/testing/selftests/bpf/bpf_helpers.h |   3 +
 26 files changed, 1288 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/krsi.h
 create mode 100644 samples/bpf/krsi_helpers.h
 create mode 100644 samples/bpf/krsi_kern.c
 create mode 100644 samples/bpf/krsi_user.c
 create mode 100644 security/krsi/Kconfig
 create mode 100644 security/krsi/Makefile
 create mode 100644 security/krsi/include/hooks.h
 create mode 100644 security/krsi/include/krsi_fs.h
 create mode 100644 security/krsi/include/krsi_init.h
 create mode 100644 security/krsi/krsi.c
 create mode 100644 security/krsi/krsi_fs.c
 create mode 100644 security/krsi/ops.c

-- 
2.20.1

[RFC v1 01/14] krsi: Add a skeleton and config options for the KRSI LSM

[KP Singh]

From: KP Singh <kpsingh@google.com>

The LSM can be enabled by:

- Enabling CONFIG_SECURITY_KRSI.
- Adding "krsi" to the CONFIG_LSM string.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 MAINTAINERS            |  5 +++++
 security/Kconfig       |  1 +
 security/Makefile      |  2 ++
 security/krsi/Kconfig  | 22 ++++++++++++++++++++++
 security/krsi/Makefile |  1 +
 security/krsi/krsi.c   | 24 ++++++++++++++++++++++++
 6 files changed, 55 insertions(+)
 create mode 100644 security/krsi/Kconfig
 create mode 100644 security/krsi/Makefile
 create mode 100644 security/krsi/krsi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9cbcf167bdd0..8e0364391d8b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9002,6 +9002,11 @@ F:	include/linux/kprobes.h
 F:	include/asm-generic/kprobes.h
 F:	kernel/kprobes.c
 
+KRSI SECURITY MODULE
+M:	KP Singh <kpsingh@chromium.org>
+S:	Supported
+F:	security/krsi/
+
 KS0108 LCD CONTROLLER DRIVER
 M:	Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
 S:	Maintained
diff --git a/security/Kconfig b/security/Kconfig
index 0d65594b5196..febf7953803f 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -236,6 +236,7 @@ source "security/tomoyo/Kconfig"
 source "security/apparmor/Kconfig"
 source "security/loadpin/Kconfig"
 source "security/yama/Kconfig"
+source "security/krsi/Kconfig"
 source "security/safesetid/Kconfig"
 
 source "security/integrity/Kconfig"
diff --git a/security/Makefile b/security/Makefile
index c598b904938f..25779ce89bf2 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_SMACK)		+= smack
 subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
 subdir-$(CONFIG_SECURITY_YAMA)		+= yama
+subdir-$(CONFIG_SECURITY_KRSI)		+= krsi
 subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
 subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
 
@@ -25,6 +26,7 @@ obj-$(CONFIG_AUDIT)			+= lsm_audit.o
 obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/
 obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/
 obj-$(CONFIG_SECURITY_YAMA)		+= yama/
+obj-$(CONFIG_SECURITY_KRSI)		+= krsi/
 obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
 obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
diff --git a/security/krsi/Kconfig b/security/krsi/Kconfig
new file mode 100644
index 000000000000..bf5eab4355af
--- /dev/null
+++ b/security/krsi/Kconfig
@@ -0,0 +1,22 @@
+config SECURITY_KRSI
+	bool "Runtime Security Instrumentation (BPF-based MAC and audit policy)"
+	depends on SECURITY
+	depends on SECURITYFS
+	depends on BPF
+	depends on BPF_SYSCALL
+	help
+	  This selects the Kernel Runtime Security Instrumentation
+	  LSM which allows dynamic instrumentation of the security hooks with
+	  eBPF programs. The LSM creates per-hook files in securityfs to which
+	  eBPF programs can be attached.
+
+	  If you are unsure how to answer this question, answer N.
+
+config SECURITY_KRSI_ENFORCE
+	bool "Deny operations based on the evaluation of the attached programs"
+	depends on SECURITY_KRSI
+	help
+	  eBPF programs attached to hooks can be used for both auditing and
+	  enforcement. Enabling enforcement implies that the evaluation result
+	  from the attached eBPF programs will allow and deny the operation
+	  guarded by the security hook.
diff --git a/security/krsi/Makefile b/security/krsi/Makefile
new file mode 100644
index 000000000000..73320e8d16f8
--- /dev/null
+++ b/security/krsi/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SECURITY_KRSI) := krsi.o
diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
new file mode 100644
index 000000000000..9ce4f56fb78d
--- /dev/null
+++ b/security/krsi/krsi.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/lsm_hooks.h>
+
+static int krsi_process_execution(struct linux_binprm *bprm)
+{
+	return 0;
+}
+
+static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(bprm_check_security, krsi_process_execution),
+};
+
+static int __init krsi_init(void)
+{
+	security_add_hooks(krsi_hooks, ARRAY_SIZE(krsi_hooks), "krsi");
+	pr_info("eBPF and LSM are friends now.\n");
+	return 0;
+}
+
+DEFINE_LSM(krsi) = {
+	.name = "krsi",
+	.init = krsi_init,
+};
-- 
2.20.1

[RFC v1 02/14] krsi: Introduce types for KRSI eBPF

[KP Singh]

From: KP Singh <kpsingh@google.com>

KRSI intrdocues a new eBPF program type BPF_PROG_TYPE_KRSI with an
expected attach type of BPF_KRSI. An -EINVAL error is returned if an
attachment is requested.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/bpf_types.h |  3 +++
 include/uapi/linux/bpf.h  |  2 ++
 kernel/bpf/syscall.c      |  6 ++++++
 security/krsi/Makefile    |  2 +-
 security/krsi/ops.c       | 10 ++++++++++
 5 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 security/krsi/ops.c

diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index eec5aeeeaf92..129594c09b5c 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -38,6 +38,9 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LIRC_MODE2, lirc_mode2)
 #ifdef CONFIG_INET
 BPF_PROG_TYPE(BPF_PROG_TYPE_SK_REUSEPORT, sk_reuseport)
 #endif
+#ifdef CONFIG_SECURITY_KRSI
+BPF_PROG_TYPE(BPF_PROG_TYPE_KRSI, krsi)
+#endif
 
 BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
 BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a5aa7d3ac6a1..32ab38f1a2fe 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -171,6 +171,7 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_CGROUP_SYSCTL,
 	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
 	BPF_PROG_TYPE_CGROUP_SOCKOPT,
+	BPF_PROG_TYPE_KRSI,
 };
 
 enum bpf_attach_type {
@@ -197,6 +198,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_UDP6_RECVMSG,
 	BPF_CGROUP_GETSOCKOPT,
 	BPF_CGROUP_SETSOCKOPT,
+	BPF_KRSI,
 	__MAX_BPF_ATTACH_TYPE
 };
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 5d141f16f6fa..f38a539f7e67 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1915,6 +1915,9 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 	case BPF_LIRC_MODE2:
 		ptype = BPF_PROG_TYPE_LIRC_MODE2;
 		break;
+	case BPF_KRSI:
+		ptype = BPF_PROG_TYPE_KRSI;
+		break;
 	case BPF_FLOW_DISSECTOR:
 		ptype = BPF_PROG_TYPE_FLOW_DISSECTOR;
 		break;
@@ -1946,6 +1949,9 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 	case BPF_PROG_TYPE_LIRC_MODE2:
 		ret = lirc_prog_attach(attr, prog);
 		break;
+	case BPF_PROG_TYPE_KRSI:
+		ret = -EINVAL;
+		break;
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 		ret = skb_flow_dissector_bpf_prog_attach(attr, prog);
 		break;
diff --git a/security/krsi/Makefile b/security/krsi/Makefile
index 73320e8d16f8..660cc1f422fd 100644
--- a/security/krsi/Makefile
+++ b/security/krsi/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_SECURITY_KRSI) := krsi.o
+obj-$(CONFIG_SECURITY_KRSI) := krsi.o ops.o
diff --git a/security/krsi/ops.c b/security/krsi/ops.c
new file mode 100644
index 000000000000..f2de3bd9621e
--- /dev/null
+++ b/security/krsi/ops.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/filter.h>
+#include <linux/bpf.h>
+
+const struct bpf_prog_ops krsi_prog_ops = {
+};
+
+const struct bpf_verifier_ops krsi_verifier_ops = {
+};
-- 
2.20.1

[RFC v1 03/14] bpf: krsi: sync BPF UAPI header with tools

[KP Singh]

From: KP Singh <kpsingh@google.com>

Signed-off-by: KP Singh <kpsingh@google.com>
---
 tools/include/uapi/linux/bpf.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a5aa7d3ac6a1..32ab38f1a2fe 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -171,6 +171,7 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_CGROUP_SYSCTL,
 	BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE,
 	BPF_PROG_TYPE_CGROUP_SOCKOPT,
+	BPF_PROG_TYPE_KRSI,
 };
 
 enum bpf_attach_type {
@@ -197,6 +198,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_UDP6_RECVMSG,
 	BPF_CGROUP_GETSOCKOPT,
 	BPF_CGROUP_SETSOCKOPT,
+	BPF_KRSI,
 	__MAX_BPF_ATTACH_TYPE
 };
 
-- 
2.20.1

[RFC v1 04/14] krsi: Add support in libbpf for BPF_PROG_TYPE_KRSI

[KP Singh]

From: KP Singh <kpsingh@google.com>

Update the libbpf library with functionality to load and
attach a program type BPF_PROG_TYPE_KRSI.

Since the bpf_prog_load does not allow the specification of an
expected attach type, it's recommended to use bpf_prog_load_xattr and
set the expected attach type as KRSI.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 tools/lib/bpf/libbpf.c        | 4 ++++
 tools/lib/bpf/libbpf.h        | 2 ++
 tools/lib/bpf/libbpf.map      | 2 ++
 tools/lib/bpf/libbpf_probes.c | 1 +
 4 files changed, 9 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 2b57d7ea7836..3cc86bbc68cd 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2676,6 +2676,7 @@ static bool bpf_prog_type__needs_kver(enum bpf_prog_type type)
 	case BPF_PROG_TYPE_PERF_EVENT:
 	case BPF_PROG_TYPE_CGROUP_SYSCTL:
 	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
+	case BPF_PROG_TYPE_KRSI:
 		return false;
 	case BPF_PROG_TYPE_KPROBE:
 	default:
@@ -3536,6 +3537,7 @@ bool bpf_program__is_##NAME(const struct bpf_program *prog)	\
 }								\
 
 BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
+BPF_PROG_TYPE_FNS(krsi, BPF_PROG_TYPE_KRSI);
 BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
 BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
 BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
@@ -3590,6 +3592,8 @@ static const struct {
 	BPF_PROG_SEC("lwt_out",			BPF_PROG_TYPE_LWT_OUT),
 	BPF_PROG_SEC("lwt_xmit",		BPF_PROG_TYPE_LWT_XMIT),
 	BPF_PROG_SEC("lwt_seg6local",		BPF_PROG_TYPE_LWT_SEG6LOCAL),
+	BPF_APROG_SEC("krsi",			BPF_PROG_TYPE_KRSI,
+						BPF_KRSI),
 	BPF_APROG_SEC("cgroup_skb/ingress",	BPF_PROG_TYPE_CGROUP_SKB,
 						BPF_CGROUP_INET_INGRESS),
 	BPF_APROG_SEC("cgroup_skb/egress",	BPF_PROG_TYPE_CGROUP_SKB,
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 5cbf459ece0b..8781d29b4035 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -261,6 +261,7 @@ LIBBPF_API int bpf_program__set_sched_cls(struct bpf_program *prog);
 LIBBPF_API int bpf_program__set_sched_act(struct bpf_program *prog);
 LIBBPF_API int bpf_program__set_xdp(struct bpf_program *prog);
 LIBBPF_API int bpf_program__set_perf_event(struct bpf_program *prog);
+LIBBPF_API int bpf_program__set_krsi(struct bpf_program *prog);
 LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
 				      enum bpf_prog_type type);
 LIBBPF_API void
@@ -275,6 +276,7 @@ LIBBPF_API bool bpf_program__is_sched_cls(const struct bpf_program *prog);
 LIBBPF_API bool bpf_program__is_sched_act(const struct bpf_program *prog);
 LIBBPF_API bool bpf_program__is_xdp(const struct bpf_program *prog);
 LIBBPF_API bool bpf_program__is_perf_event(const struct bpf_program *prog);
+LIBBPF_API bool bpf_program__is_krsi(const struct bpf_program *prog);
 
 /*
  * No need for __attribute__((packed)), all members of 'bpf_map_def'
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index f9d316e873d8..75b8fe419c11 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -68,6 +68,7 @@ LIBBPF_0.0.1 {
 		bpf_prog_test_run_xattr;
 		bpf_program__fd;
 		bpf_program__is_kprobe;
+		bpf_program__is_krsi;
 		bpf_program__is_perf_event;
 		bpf_program__is_raw_tracepoint;
 		bpf_program__is_sched_act;
@@ -85,6 +86,7 @@ LIBBPF_0.0.1 {
 		bpf_program__set_expected_attach_type;
 		bpf_program__set_ifindex;
 		bpf_program__set_kprobe;
+		bpf_program__set_krsi;
 		bpf_program__set_perf_event;
 		bpf_program__set_prep;
 		bpf_program__set_priv;
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index ace1a0708d99..cc515a36794d 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -102,6 +102,7 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 	case BPF_PROG_TYPE_CGROUP_SYSCTL:
 	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
+	case BPF_PROG_TYPE_KRSI:
 	default:
 		break;
 	}
-- 
2.20.1

Re: [RFC v1 04/14] krsi: Add support in libbpf for BPF_PROG_TYPE_KRSI

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> Update the libbpf library with functionality to load and
> attach a program type BPF_PROG_TYPE_KRSI.
> 
> Since the bpf_prog_load does not allow the specification of an
> expected attach type, it's recommended to use bpf_prog_load_xattr and
> set the expected attach type as KRSI.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   tools/lib/bpf/libbpf.c        | 4 ++++
>   tools/lib/bpf/libbpf.h        | 2 ++
>   tools/lib/bpf/libbpf.map      | 2 ++
>   tools/lib/bpf/libbpf_probes.c | 1 +
>   4 files changed, 9 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2b57d7ea7836..3cc86bbc68cd 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2676,6 +2676,7 @@ static bool bpf_prog_type__needs_kver(enum bpf_prog_type type)
>   	case BPF_PROG_TYPE_PERF_EVENT:
>   	case BPF_PROG_TYPE_CGROUP_SYSCTL:
>   	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> +	case BPF_PROG_TYPE_KRSI:
>   		return false;
>   	case BPF_PROG_TYPE_KPROBE:
>   	default:
> @@ -3536,6 +3537,7 @@ bool bpf_program__is_##NAME(const struct bpf_program *prog)	\
>   }								\
>   
>   BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
> +BPF_PROG_TYPE_FNS(krsi, BPF_PROG_TYPE_KRSI);
>   BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
>   BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
>   BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
> @@ -3590,6 +3592,8 @@ static const struct {
>   	BPF_PROG_SEC("lwt_out",			BPF_PROG_TYPE_LWT_OUT),
>   	BPF_PROG_SEC("lwt_xmit",		BPF_PROG_TYPE_LWT_XMIT),
>   	BPF_PROG_SEC("lwt_seg6local",		BPF_PROG_TYPE_LWT_SEG6LOCAL),
> +	BPF_APROG_SEC("krsi",			BPF_PROG_TYPE_KRSI,
> +						BPF_KRSI),
>   	BPF_APROG_SEC("cgroup_skb/ingress",	BPF_PROG_TYPE_CGROUP_SKB,
>   						BPF_CGROUP_INET_INGRESS),
>   	BPF_APROG_SEC("cgroup_skb/egress",	BPF_PROG_TYPE_CGROUP_SKB,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5cbf459ece0b..8781d29b4035 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -261,6 +261,7 @@ LIBBPF_API int bpf_program__set_sched_cls(struct bpf_program *prog);
>   LIBBPF_API int bpf_program__set_sched_act(struct bpf_program *prog);
>   LIBBPF_API int bpf_program__set_xdp(struct bpf_program *prog);
>   LIBBPF_API int bpf_program__set_perf_event(struct bpf_program *prog);
> +LIBBPF_API int bpf_program__set_krsi(struct bpf_program *prog);
>   LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
>   				      enum bpf_prog_type type);
>   LIBBPF_API void
> @@ -275,6 +276,7 @@ LIBBPF_API bool bpf_program__is_sched_cls(const struct bpf_program *prog);
>   LIBBPF_API bool bpf_program__is_sched_act(const struct bpf_program *prog);
>   LIBBPF_API bool bpf_program__is_xdp(const struct bpf_program *prog);
>   LIBBPF_API bool bpf_program__is_perf_event(const struct bpf_program *prog);
> +LIBBPF_API bool bpf_program__is_krsi(const struct bpf_program *prog);
>   
>   /*
>    * No need for __attribute__((packed)), all members of 'bpf_map_def'
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index f9d316e873d8..75b8fe419c11 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -68,6 +68,7 @@ LIBBPF_0.0.1 {
>   		bpf_prog_test_run_xattr;
>   		bpf_program__fd;
>   		bpf_program__is_kprobe;
> +		bpf_program__is_krsi;
>   		bpf_program__is_perf_event;
>   		bpf_program__is_raw_tracepoint;
>   		bpf_program__is_sched_act;
> @@ -85,6 +86,7 @@ LIBBPF_0.0.1 {
>   		bpf_program__set_expected_attach_type;
>   		bpf_program__set_ifindex;
>   		bpf_program__set_kprobe;
> +		bpf_program__set_krsi;
>   		bpf_program__set_perf_event;
>   		bpf_program__set_prep;
>   		bpf_program__set_priv;

Please put the above two new API functions in version LIBBPF_0.0.5.

> diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
> index ace1a0708d99..cc515a36794d 100644
> --- a/tools/lib/bpf/libbpf_probes.c
> +++ b/tools/lib/bpf/libbpf_probes.c
> @@ -102,6 +102,7 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
>   	case BPF_PROG_TYPE_FLOW_DISSECTOR:
>   	case BPF_PROG_TYPE_CGROUP_SYSCTL:
>   	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> +	case BPF_PROG_TYPE_KRSI:
>   	default:
>   		break;
>   	}
> 

[RFC v1 05/14] krsi: Initialize KRSI hooks and create files in securityfs

[KP Singh]

From: KP Singh <kpsingh@google.com>

The LSM creates files in securityfs for each hook registered with the
LSM.

    /sys/kernel/security/bpf/<h_name>

The initialization of the hooks is done collectively in an internal
header "hooks.h" which results in:

* Creation of a file for the hook in the securityfs.
* Allocation of a krsi_hook data structure which stores a pointer to the
  dentry of the newly created file in securityfs.
* A pointer to the krsi_hook data structure is stored in the private
  d_fsdata of dentry of the file created in securityFS.

These files will later be used to specify an attachment target during
BPF_PROG_LOAD.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/krsi/Makefile            |  4 +-
 security/krsi/include/hooks.h     | 21 ++++++++
 security/krsi/include/krsi_fs.h   | 19 +++++++
 security/krsi/include/krsi_init.h | 45 ++++++++++++++++
 security/krsi/krsi.c              | 16 +++++-
 security/krsi/krsi_fs.c           | 88 +++++++++++++++++++++++++++++++
 6 files changed, 191 insertions(+), 2 deletions(-)
 create mode 100644 security/krsi/include/hooks.h
 create mode 100644 security/krsi/include/krsi_fs.h
 create mode 100644 security/krsi/include/krsi_init.h
 create mode 100644 security/krsi/krsi_fs.c

diff --git a/security/krsi/Makefile b/security/krsi/Makefile
index 660cc1f422fd..4586241f16e1 100644
--- a/security/krsi/Makefile
+++ b/security/krsi/Makefile
@@ -1 +1,3 @@
-obj-$(CONFIG_SECURITY_KRSI) := krsi.o ops.o
+obj-$(CONFIG_SECURITY_KRSI) := krsi.o krsi_fs.o ops.o
+
+ccflags-y := -I$(srctree)/security/krsi -I$(srctree)/security/krsi/include
diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
new file mode 100644
index 000000000000..e070c452b5de
--- /dev/null
+++ b/security/krsi/include/hooks.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * The hooks for the KRSI LSM are declared in this file.
+ *
+ * This header MUST NOT be included directly and should
+ * be only used to initialize the hooks lists.
+ *
+ * Format:
+ *
+ *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
+ *
+ * KRSI adds one layer of indirection between the name of the hook and the name
+ * it exposes to the userspace in Security FS to prevent the userspace from
+ * breaking in case the name of the hook changes in the kernel or if there's
+ * another LSM hook that maps better to the represented security behaviour.
+ */
+KRSI_HOOK_INIT(PROCESS_EXECUTION,
+	       process_execution,
+	       bprm_check_security,
+	       krsi_process_execution)
diff --git a/security/krsi/include/krsi_fs.h b/security/krsi/include/krsi_fs.h
new file mode 100644
index 000000000000..38134661d8d6
--- /dev/null
+++ b/security/krsi/include/krsi_fs.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _KRSI_FS_H
+#define _KRSI_FS_H
+
+#include <linux/bpf.h>
+#include <linux/fs.h>
+#include <linux/types.h>
+
+bool is_krsi_hook_file(struct file *f);
+
+/*
+ * The name of the directory created in securityfs
+ *
+ *	/sys/kernel/security/<dir_name>
+ */
+#define KRSI_SFS_NAME "krsi"
+
+#endif /* _KRSI_FS_H */
diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
new file mode 100644
index 000000000000..68755182a031
--- /dev/null
+++ b/security/krsi/include/krsi_init.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _KRSI_INIT_H
+#define _KRSI_INIT_H
+
+#include "krsi_fs.h"
+
+enum krsi_hook_type {
+	PROCESS_EXECUTION,
+	__MAX_KRSI_HOOK_TYPE, /* delimiter */
+};
+
+extern int krsi_fs_initialized;
+/*
+ * The LSM creates one file per hook.
+ *
+ * A pointer to krsi_hook data structure is stored in the
+ * private fsdata of the dentry of the per-hook file created
+ * in securityfs.
+ */
+struct krsi_hook {
+	/*
+	 * The name of the security hook, a file with this name will be created
+	 * in the securityfs.
+	 */
+	const char *name;
+	/*
+	 * The type of the LSM hook, the LSM uses this to index the list of the
+	 * hooks to run the eBPF programs that may have been attached.
+	 */
+	enum krsi_hook_type h_type;
+	/*
+	 * The dentry of the file created in securityfs.
+	 */
+	struct dentry *h_dentry;
+};
+
+extern struct krsi_hook krsi_hooks_list[];
+
+#define krsi_for_each_hook(hook) \
+	for ((hook) = &krsi_hooks_list[0]; \
+	     (hook) < &krsi_hooks_list[__MAX_KRSI_HOOK_TYPE]; \
+	     (hook)++)
+
+#endif /* _KRSI_INIT_H */
diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
index 9ce4f56fb78d..77d7e2f91172 100644
--- a/security/krsi/krsi.c
+++ b/security/krsi/krsi.c
@@ -2,13 +2,27 @@
 
 #include <linux/lsm_hooks.h>
 
+#include "krsi_init.h"
+
+struct krsi_hook krsi_hooks_list[] = {
+	#define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
+		[TYPE] = { \
+			.h_type = TYPE, \
+			.name = #NAME, \
+		},
+	#include "hooks.h"
+	#undef KRSI_HOOK_INIT
+};
+
 static int krsi_process_execution(struct linux_binprm *bprm)
 {
 	return 0;
 }
 
 static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
-	LSM_HOOK_INIT(bprm_check_security, krsi_process_execution),
+	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
+	#include "hooks.h"
+	#undef KRSI_HOOK_INIT
 };
 
 static int __init krsi_init(void)
diff --git a/security/krsi/krsi_fs.c b/security/krsi/krsi_fs.c
new file mode 100644
index 000000000000..604f826cee5c
--- /dev/null
+++ b/security/krsi/krsi_fs.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/types.h>
+#include <linux/security.h>
+
+#include "krsi_fs.h"
+#include "krsi_init.h"
+
+extern struct krsi_hook krsi_hooks_list[];
+
+static struct dentry *krsi_dir;
+
+static const struct file_operations krsi_hook_ops = {
+	.llseek = generic_file_llseek,
+};
+
+int krsi_fs_initialized;
+
+bool is_krsi_hook_file(struct file *f)
+{
+	return f->f_op == &krsi_hook_ops;
+}
+
+static void __init krsi_free_hook(struct krsi_hook *h)
+{
+	securityfs_remove(h->h_dentry);
+	h->h_dentry = NULL;
+}
+
+static int __init krsi_init_hook(struct krsi_hook *h, struct dentry *parent)
+{
+	struct dentry *h_dentry;
+	int ret;
+
+	h_dentry = securityfs_create_file(h->name, 0600, parent,
+			NULL, &krsi_hook_ops);
+
+	if (IS_ERR(h_dentry))
+		return PTR_ERR(h_dentry);
+	h_dentry->d_fsdata = h;
+	h->h_dentry = h_dentry;
+	return 0;
+
+error:
+	securityfs_remove(h_dentry);
+	return ret;
+}
+
+static int __init krsi_fs_init(void)
+{
+
+	struct krsi_hook *hook;
+	int ret;
+
+	krsi_dir = securityfs_create_dir(KRSI_SFS_NAME, NULL);
+	if (IS_ERR(krsi_dir)) {
+		ret = PTR_ERR(krsi_dir);
+		pr_err("Unable to create krsi sysfs dir: %d\n", ret);
+		krsi_dir = NULL;
+		return ret;
+	}
+
+	/*
+	 * If there is an error in initializing a hook, the initialization
+	 * logic makes sure that it has been freed, but this means that
+	 * cleanup should be called for all the other hooks. The cleanup
+	 * logic handles uninitialized data.
+	 */
+	krsi_for_each_hook(hook) {
+		ret = krsi_init_hook(hook, krsi_dir);
+		if (ret < 0)
+			goto error;
+	}
+
+	krsi_fs_initialized = 1;
+	return 0;
+error:
+	krsi_for_each_hook(hook)
+		krsi_free_hook(hook);
+	securityfs_remove(krsi_dir);
+	return ret;
+}
+
+late_initcall(krsi_fs_init);
-- 
2.20.1

Re: [RFC v1 05/14] krsi: Initialize KRSI hooks and create files in securityfs

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> The LSM creates files in securityfs for each hook registered with the
> LSM.
> 
>      /sys/kernel/security/bpf/<h_name>
> 
> The initialization of the hooks is done collectively in an internal
> header "hooks.h" which results in:
> 
> * Creation of a file for the hook in the securityfs.
> * Allocation of a krsi_hook data structure which stores a pointer to the
>    dentry of the newly created file in securityfs.
> * A pointer to the krsi_hook data structure is stored in the private
>    d_fsdata of dentry of the file created in securityFS.
> 
> These files will later be used to specify an attachment target during
> BPF_PROG_LOAD.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   security/krsi/Makefile            |  4 +-
>   security/krsi/include/hooks.h     | 21 ++++++++
>   security/krsi/include/krsi_fs.h   | 19 +++++++
>   security/krsi/include/krsi_init.h | 45 ++++++++++++++++
>   security/krsi/krsi.c              | 16 +++++-
>   security/krsi/krsi_fs.c           | 88 +++++++++++++++++++++++++++++++
>   6 files changed, 191 insertions(+), 2 deletions(-)
>   create mode 100644 security/krsi/include/hooks.h
>   create mode 100644 security/krsi/include/krsi_fs.h
>   create mode 100644 security/krsi/include/krsi_init.h
>   create mode 100644 security/krsi/krsi_fs.c
> 
> diff --git a/security/krsi/Makefile b/security/krsi/Makefile
> index 660cc1f422fd..4586241f16e1 100644
> --- a/security/krsi/Makefile
> +++ b/security/krsi/Makefile
> @@ -1 +1,3 @@
> -obj-$(CONFIG_SECURITY_KRSI) := krsi.o ops.o
> +obj-$(CONFIG_SECURITY_KRSI) := krsi.o krsi_fs.o ops.o
> +
> +ccflags-y := -I$(srctree)/security/krsi -I$(srctree)/security/krsi/include
> diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
> new file mode 100644
> index 000000000000..e070c452b5de
> --- /dev/null
> +++ b/security/krsi/include/hooks.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * The hooks for the KRSI LSM are declared in this file.
> + *
> + * This header MUST NOT be included directly and should
> + * be only used to initialize the hooks lists.
> + *
> + * Format:
> + *
> + *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
> + *
> + * KRSI adds one layer of indirection between the name of the hook and the name
> + * it exposes to the userspace in Security FS to prevent the userspace from
> + * breaking in case the name of the hook changes in the kernel or if there's
> + * another LSM hook that maps better to the represented security behaviour.
> + */
> +KRSI_HOOK_INIT(PROCESS_EXECUTION,
> +	       process_execution,
> +	       bprm_check_security,
> +	       krsi_process_execution)
> diff --git a/security/krsi/include/krsi_fs.h b/security/krsi/include/krsi_fs.h
> new file mode 100644
> index 000000000000..38134661d8d6
> --- /dev/null
> +++ b/security/krsi/include/krsi_fs.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _KRSI_FS_H
> +#define _KRSI_FS_H
> +
> +#include <linux/bpf.h>
> +#include <linux/fs.h>
> +#include <linux/types.h>
> +
> +bool is_krsi_hook_file(struct file *f);
> +
> +/*
> + * The name of the directory created in securityfs
> + *
> + *	/sys/kernel/security/<dir_name>
> + */
> +#define KRSI_SFS_NAME "krsi"
> +
> +#endif /* _KRSI_FS_H */
> diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
> new file mode 100644
> index 000000000000..68755182a031
> --- /dev/null
> +++ b/security/krsi/include/krsi_init.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _KRSI_INIT_H
> +#define _KRSI_INIT_H
> +
> +#include "krsi_fs.h"
> +
> +enum krsi_hook_type {
> +	PROCESS_EXECUTION,
> +	__MAX_KRSI_HOOK_TYPE, /* delimiter */
> +};
> +
> +extern int krsi_fs_initialized;
> +/*
> + * The LSM creates one file per hook.
> + *
> + * A pointer to krsi_hook data structure is stored in the
> + * private fsdata of the dentry of the per-hook file created
> + * in securityfs.
> + */
> +struct krsi_hook {
> +	/*
> +	 * The name of the security hook, a file with this name will be created
> +	 * in the securityfs.
> +	 */
> +	const char *name;
> +	/*
> +	 * The type of the LSM hook, the LSM uses this to index the list of the
> +	 * hooks to run the eBPF programs that may have been attached.
> +	 */
> +	enum krsi_hook_type h_type;
> +	/*
> +	 * The dentry of the file created in securityfs.
> +	 */
> +	struct dentry *h_dentry;
> +};
> +
> +extern struct krsi_hook krsi_hooks_list[];
> +
> +#define krsi_for_each_hook(hook) \
> +	for ((hook) = &krsi_hooks_list[0]; \
> +	     (hook) < &krsi_hooks_list[__MAX_KRSI_HOOK_TYPE]; \
> +	     (hook)++)
> +
> +#endif /* _KRSI_INIT_H */
> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
> index 9ce4f56fb78d..77d7e2f91172 100644
> --- a/security/krsi/krsi.c
> +++ b/security/krsi/krsi.c
> @@ -2,13 +2,27 @@
>   
>   #include <linux/lsm_hooks.h>
>   
> +#include "krsi_init.h"
> +
> +struct krsi_hook krsi_hooks_list[] = {
> +	#define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
> +		[TYPE] = { \
> +			.h_type = TYPE, \
> +			.name = #NAME, \
> +		},
> +	#include "hooks.h"
> +	#undef KRSI_HOOK_INIT
> +};
> +
>   static int krsi_process_execution(struct linux_binprm *bprm)
>   {
>   	return 0;
>   }
>   
>   static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
> -	LSM_HOOK_INIT(bprm_check_security, krsi_process_execution),
> +	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
> +	#include "hooks.h"
> +	#undef KRSI_HOOK_INIT
>   };
>   
>   static int __init krsi_init(void)
> diff --git a/security/krsi/krsi_fs.c b/security/krsi/krsi_fs.c
> new file mode 100644
> index 000000000000..604f826cee5c
> --- /dev/null
> +++ b/security/krsi/krsi_fs.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/types.h>
> +#include <linux/security.h>
> +
> +#include "krsi_fs.h"
> +#include "krsi_init.h"
> +
> +extern struct krsi_hook krsi_hooks_list[];
> +
> +static struct dentry *krsi_dir;
> +
> +static const struct file_operations krsi_hook_ops = {
> +	.llseek = generic_file_llseek,
> +};
> +
> +int krsi_fs_initialized;
> +
> +bool is_krsi_hook_file(struct file *f)
> +{
> +	return f->f_op == &krsi_hook_ops;
> +}
> +
> +static void __init krsi_free_hook(struct krsi_hook *h)
> +{
> +	securityfs_remove(h->h_dentry);
> +	h->h_dentry = NULL;
> +}
> +
> +static int __init krsi_init_hook(struct krsi_hook *h, struct dentry *parent)
> +{
> +	struct dentry *h_dentry;
> +	int ret;
> +
> +	h_dentry = securityfs_create_file(h->name, 0600, parent,
> +			NULL, &krsi_hook_ops);
> +
> +	if (IS_ERR(h_dentry))
> +		return PTR_ERR(h_dentry);
> +	h_dentry->d_fsdata = h;
> +	h->h_dentry = h_dentry;
> +	return 0;
> +
> +error:

The 'error' label is not used here.

> +	securityfs_remove(h_dentry);
> +	return ret;
> +}
> +
> +static int __init krsi_fs_init(void)
> +{
> +
> +	struct krsi_hook *hook;
> +	int ret;
> +
> +	krsi_dir = securityfs_create_dir(KRSI_SFS_NAME, NULL);
> +	if (IS_ERR(krsi_dir)) {
> +		ret = PTR_ERR(krsi_dir);
> +		pr_err("Unable to create krsi sysfs dir: %d\n", ret);
> +		krsi_dir = NULL;
> +		return ret;
> +	}
> +
> +	/*
> +	 * If there is an error in initializing a hook, the initialization
> +	 * logic makes sure that it has been freed, but this means that
> +	 * cleanup should be called for all the other hooks. The cleanup
> +	 * logic handles uninitialized data.
> +	 */
> +	krsi_for_each_hook(hook) {
> +		ret = krsi_init_hook(hook, krsi_dir);
> +		if (ret < 0)
> +			goto error;
> +	}
> +
> +	krsi_fs_initialized = 1;
> +	return 0;
> +error:
> +	krsi_for_each_hook(hook)
> +		krsi_free_hook(hook);
> +	securityfs_remove(krsi_dir);
> +	return ret;
> +}
> +
> +late_initcall(krsi_fs_init);
> 

[RFC v1 06/14] krsi: Implement eBPF operations, attachment and execution

[KP Singh]

From: KP Singh <kpsingh@google.com>

A user space program can attach an eBPF program by:

  hook_fd = open("/sys/kernel/security/krsi/process_execution", O_RDWR)
  prog_fd = bpf(BPF_PROG_LOAD, ...)
  bpf(BPF_PROG_ATTACH, hook_fd, prog_fd)

When such an attach call is received, the attachment logic looks up the
dentry and appends the program to the bpf_prog_array.

The BPF programs are stored in a bpf_prog_array and writes to the array
are guarded by a mutex. The eBPF programs are executed as a part of the
LSM hook they are attached to. If any of the eBPF programs return
an error (-ENOPERM) the action represented by the hook is denied.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/krsi.h              |  18 ++++++
 kernel/bpf/syscall.c              |   3 +-
 security/krsi/include/krsi_init.h |  51 +++++++++++++++
 security/krsi/krsi.c              |  13 +++-
 security/krsi/krsi_fs.c           |  28 ++++++++
 security/krsi/ops.c               | 102 ++++++++++++++++++++++++++++++
 6 files changed, 213 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/krsi.h

diff --git a/include/linux/krsi.h b/include/linux/krsi.h
new file mode 100644
index 000000000000..c7d1790d0c1f
--- /dev/null
+++ b/include/linux/krsi.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _KRSI_H
+#define _KRSI_H
+
+#include <linux/bpf.h>
+
+#ifdef CONFIG_SECURITY_KRSI
+int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
+#else
+static inline int krsi_prog_attach(const union bpf_attr *attr,
+				   struct bpf_prog *prog)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_SECURITY_KRSI */
+
+#endif /* _KRSI_H */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f38a539f7e67..ab063ed84258 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4,6 +4,7 @@
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/bpf_lirc.h>
+#include <linux/krsi.h>
 #include <linux/btf.h>
 #include <linux/syscalls.h>
 #include <linux/slab.h>
@@ -1950,7 +1951,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 		ret = lirc_prog_attach(attr, prog);
 		break;
 	case BPF_PROG_TYPE_KRSI:
-		ret = -EINVAL;
+		ret = krsi_prog_attach(attr, prog);
 		break;
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 		ret = skb_flow_dissector_bpf_prog_attach(attr, prog);
diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
index 68755182a031..4e17ecacd4ed 100644
--- a/security/krsi/include/krsi_init.h
+++ b/security/krsi/include/krsi_init.h
@@ -5,12 +5,29 @@
 
 #include "krsi_fs.h"
 
+#include <linux/binfmts.h>
+
 enum krsi_hook_type {
 	PROCESS_EXECUTION,
 	__MAX_KRSI_HOOK_TYPE, /* delimiter */
 };
 
 extern int krsi_fs_initialized;
+
+struct krsi_bprm_ctx {
+	struct linux_binprm *bprm;
+};
+
+/*
+ * krsi_ctx is the context that is passed to all KRSI eBPF
+ * programs.
+ */
+struct krsi_ctx {
+	union {
+		struct krsi_bprm_ctx bprm_ctx;
+	};
+};
+
 /*
  * The LSM creates one file per hook.
  *
@@ -33,10 +50,44 @@ struct krsi_hook {
 	 * The dentry of the file created in securityfs.
 	 */
 	struct dentry *h_dentry;
+	/*
+	 * The mutex must be held when updating the progs attached to the hook.
+	 */
+	struct mutex mutex;
+	/*
+	 * The eBPF programs that are attached to this hook.
+	 */
+	struct bpf_prog_array __rcu	*progs;
 };
 
 extern struct krsi_hook krsi_hooks_list[];
 
+static inline int krsi_run_progs(enum krsi_hook_type t, struct krsi_ctx *ctx)
+{
+	struct bpf_prog_array_item *item;
+	struct bpf_prog *prog;
+	struct krsi_hook *h = &krsi_hooks_list[t];
+	int ret, retval = 0;
+
+	preempt_disable();
+	rcu_read_lock();
+
+	item = rcu_dereference(h->progs)->items;
+	while ((prog = READ_ONCE(item->prog))) {
+		ret = BPF_PROG_RUN(prog, ctx);
+		if (ret < 0) {
+			retval = ret;
+			goto out;
+		}
+		item++;
+	}
+
+out:
+	rcu_read_unlock();
+	preempt_enable();
+	return IS_ENABLED(CONFIG_SECURITY_KRSI_ENFORCE) ? retval : 0;
+}
+
 #define krsi_for_each_hook(hook) \
 	for ((hook) = &krsi_hooks_list[0]; \
 	     (hook) < &krsi_hooks_list[__MAX_KRSI_HOOK_TYPE]; \
diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
index 77d7e2f91172..d3a4a361c192 100644
--- a/security/krsi/krsi.c
+++ b/security/krsi/krsi.c
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/lsm_hooks.h>
+#include <linux/filter.h>
+#include <linux/bpf.h>
+#include <linux/binfmts.h>
 
 #include "krsi_init.h"
 
@@ -16,7 +19,15 @@ struct krsi_hook krsi_hooks_list[] = {
 
 static int krsi_process_execution(struct linux_binprm *bprm)
 {
-	return 0;
+	int ret;
+	struct krsi_ctx ctx;
+
+	ctx.bprm_ctx = (struct krsi_bprm_ctx) {
+		.bprm = bprm,
+	};
+
+	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
+	return ret;
 }
 
 static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
diff --git a/security/krsi/krsi_fs.c b/security/krsi/krsi_fs.c
index 604f826cee5c..3ba18b52ce85 100644
--- a/security/krsi/krsi_fs.c
+++ b/security/krsi/krsi_fs.c
@@ -5,6 +5,8 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/types.h>
+#include <linux/filter.h>
+#include <linux/bpf.h>
 #include <linux/security.h>
 
 #include "krsi_fs.h"
@@ -27,12 +29,29 @@ bool is_krsi_hook_file(struct file *f)
 
 static void __init krsi_free_hook(struct krsi_hook *h)
 {
+	struct bpf_prog_array_item *item;
+	/*
+	 * This function is __init so we are guarranteed that there will be
+	 * no concurrent access.
+	 */
+	struct bpf_prog_array *progs = rcu_dereference_raw(h->progs);
+
+	if (progs) {
+		item = progs->items;
+		while (item->prog) {
+			bpf_prog_put(item->prog);
+			item++;
+		}
+		bpf_prog_array_free(progs);
+	}
+
 	securityfs_remove(h->h_dentry);
 	h->h_dentry = NULL;
 }
 
 static int __init krsi_init_hook(struct krsi_hook *h, struct dentry *parent)
 {
+	struct bpf_prog_array __rcu     *progs;
 	struct dentry *h_dentry;
 	int ret;
 
@@ -41,6 +60,15 @@ static int __init krsi_init_hook(struct krsi_hook *h, struct dentry *parent)
 
 	if (IS_ERR(h_dentry))
 		return PTR_ERR(h_dentry);
+
+	mutex_init(&h->mutex);
+	progs = bpf_prog_array_alloc(0, GFP_KERNEL);
+	if (!progs) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	RCU_INIT_POINTER(h->progs, progs);
 	h_dentry->d_fsdata = h;
 	h->h_dentry = h_dentry;
 	return 0;
diff --git a/security/krsi/ops.c b/security/krsi/ops.c
index f2de3bd9621e..cf4d06189aa1 100644
--- a/security/krsi/ops.c
+++ b/security/krsi/ops.c
@@ -1,10 +1,112 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/err.h>
+#include <linux/types.h>
 #include <linux/filter.h>
 #include <linux/bpf.h>
+#include <linux/security.h>
+#include <linux/krsi.h>
+
+#include "krsi_init.h"
+#include "krsi_fs.h"
+
+extern struct krsi_hook krsi_hooks_list[];
+
+static struct krsi_hook *get_hook_from_fd(int fd)
+{
+	struct fd f = fdget(fd);
+	struct krsi_hook *h;
+	int ret;
+
+	if (!f.file) {
+		ret = -EBADF;
+		goto error;
+	}
+
+	if (!is_krsi_hook_file(f.file)) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	/*
+	 * The securityfs dentry never disappears, so we don't need to take a
+	 * reference to it.
+	 */
+	h = file_dentry(f.file)->d_fsdata;
+	if (WARN_ON(!h)) {
+		ret = -EINVAL;
+		goto error;
+	}
+	fdput(f);
+	return h;
+
+error:
+	fdput(f);
+	return ERR_PTR(ret);
+}
+
+int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct bpf_prog_array *old_array;
+	struct bpf_prog_array *new_array;
+	struct krsi_hook *h;
+	int ret = 0;
+
+	h = get_hook_from_fd(attr->target_fd);
+	if (IS_ERR(h))
+		return PTR_ERR(h);
+
+	mutex_lock(&h->mutex);
+	old_array = rcu_dereference_protected(h->progs,
+					      lockdep_is_held(&h->mutex));
+
+	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+	if (ret < 0) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	rcu_assign_pointer(h->progs, new_array);
+	bpf_prog_array_free(old_array);
+
+unlock:
+	mutex_unlock(&h->mutex);
+	return ret;
+}
 
 const struct bpf_prog_ops krsi_prog_ops = {
 };
 
+static bool krsi_prog_is_valid_access(int off, int size,
+				      enum bpf_access_type type,
+				      const struct bpf_prog *prog,
+				      struct bpf_insn_access_aux *info)
+{
+	/*
+	 * KRSI is conservative about any direct access in eBPF to
+	 * prevent the users from depending on the internals of the kernel and
+	 * aims at providing a rich eco-system of safe eBPF helpers as an API
+	 * for accessing relevant information from the context.
+	 */
+	return false;
+}
+
+static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
+							 func_id,
+							 const struct bpf_prog
+							 *prog)
+{
+	switch (func_id) {
+	case BPF_FUNC_map_lookup_elem:
+		return &bpf_map_lookup_elem_proto;
+	case BPF_FUNC_get_current_pid_tgid:
+		return &bpf_get_current_pid_tgid_proto;
+	default:
+		return NULL;
+	}
+}
+
 const struct bpf_verifier_ops krsi_verifier_ops = {
+	.get_func_proto = krsi_prog_func_proto,
+	.is_valid_access = krsi_prog_is_valid_access,
 };
-- 
2.20.1

Re: [RFC v1 06/14] krsi: Implement eBPF operations, attachment and execution

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> A user space program can attach an eBPF program by:
> 
>    hook_fd = open("/sys/kernel/security/krsi/process_execution", O_RDWR)
>    prog_fd = bpf(BPF_PROG_LOAD, ...)
>    bpf(BPF_PROG_ATTACH, hook_fd, prog_fd)
> 
> When such an attach call is received, the attachment logic looks up the
> dentry and appends the program to the bpf_prog_array.
> 
> The BPF programs are stored in a bpf_prog_array and writes to the array
> are guarded by a mutex. The eBPF programs are executed as a part of the
> LSM hook they are attached to. If any of the eBPF programs return
> an error (-ENOPERM) the action represented by the hook is denied.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   include/linux/krsi.h              |  18 ++++++
>   kernel/bpf/syscall.c              |   3 +-
>   security/krsi/include/krsi_init.h |  51 +++++++++++++++
>   security/krsi/krsi.c              |  13 +++-
>   security/krsi/krsi_fs.c           |  28 ++++++++
>   security/krsi/ops.c               | 102 ++++++++++++++++++++++++++++++
>   6 files changed, 213 insertions(+), 2 deletions(-)
>   create mode 100644 include/linux/krsi.h
> 
> diff --git a/include/linux/krsi.h b/include/linux/krsi.h
> new file mode 100644
> index 000000000000..c7d1790d0c1f
> --- /dev/null
> +++ b/include/linux/krsi.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _KRSI_H
> +#define _KRSI_H
> +
> +#include <linux/bpf.h>
> +
> +#ifdef CONFIG_SECURITY_KRSI
> +int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
> +#else
> +static inline int krsi_prog_attach(const union bpf_attr *attr,
> +				   struct bpf_prog *prog)
> +{
> +	return -EINVAL;
> +}
> +#endif /* CONFIG_SECURITY_KRSI */
> +
> +#endif /* _KRSI_H */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index f38a539f7e67..ab063ed84258 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4,6 +4,7 @@
>   #include <linux/bpf.h>
>   #include <linux/bpf_trace.h>
>   #include <linux/bpf_lirc.h>
> +#include <linux/krsi.h>
>   #include <linux/btf.h>
>   #include <linux/syscalls.h>
>   #include <linux/slab.h>
> @@ -1950,7 +1951,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
>   		ret = lirc_prog_attach(attr, prog);
>   		break;
>   	case BPF_PROG_TYPE_KRSI:
> -		ret = -EINVAL;
> +		ret = krsi_prog_attach(attr, prog);
>   		break;
>   	case BPF_PROG_TYPE_FLOW_DISSECTOR:
>   		ret = skb_flow_dissector_bpf_prog_attach(attr, prog);
> diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
> index 68755182a031..4e17ecacd4ed 100644
> --- a/security/krsi/include/krsi_init.h
> +++ b/security/krsi/include/krsi_init.h
> @@ -5,12 +5,29 @@
>   
>   #include "krsi_fs.h"
>   
> +#include <linux/binfmts.h>
> +
>   enum krsi_hook_type {
>   	PROCESS_EXECUTION,
>   	__MAX_KRSI_HOOK_TYPE, /* delimiter */
>   };
>   
>   extern int krsi_fs_initialized;
> +
> +struct krsi_bprm_ctx {
> +	struct linux_binprm *bprm;
> +};
> +
> +/*
> + * krsi_ctx is the context that is passed to all KRSI eBPF
> + * programs.
> + */
> +struct krsi_ctx {
> +	union {
> +		struct krsi_bprm_ctx bprm_ctx;
> +	};
> +};
> +
>   /*
>    * The LSM creates one file per hook.
>    *
> @@ -33,10 +50,44 @@ struct krsi_hook {
>   	 * The dentry of the file created in securityfs.
>   	 */
>   	struct dentry *h_dentry;
> +	/*
> +	 * The mutex must be held when updating the progs attached to the hook.
> +	 */
> +	struct mutex mutex;
> +	/*
> +	 * The eBPF programs that are attached to this hook.
> +	 */
> +	struct bpf_prog_array __rcu	*progs;
>   };
>   
>   extern struct krsi_hook krsi_hooks_list[];
>   
> +static inline int krsi_run_progs(enum krsi_hook_type t, struct krsi_ctx *ctx)
> +{
> +	struct bpf_prog_array_item *item;
> +	struct bpf_prog *prog;
> +	struct krsi_hook *h = &krsi_hooks_list[t];
> +	int ret, retval = 0;

Reverse christmas tree style?

> +
> +	preempt_disable();

Do we need preempt_disable() here?

> +	rcu_read_lock();
> +
> +	item = rcu_dereference(h->progs)->items;
> +	while ((prog = READ_ONCE(item->prog))) {
> +		ret = BPF_PROG_RUN(prog, ctx);
> +		if (ret < 0) {
> +			retval = ret;
> +			goto out;
> +		}
> +		item++;
> +	}
> +
> +out:
> +	rcu_read_unlock();
> +	preempt_enable();
> +	return IS_ENABLED(CONFIG_SECURITY_KRSI_ENFORCE) ? retval : 0;
> +}
> +
>   #define krsi_for_each_hook(hook) \
>   	for ((hook) = &krsi_hooks_list[0]; \
>   	     (hook) < &krsi_hooks_list[__MAX_KRSI_HOOK_TYPE]; \
> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
> index 77d7e2f91172..d3a4a361c192 100644
> --- a/security/krsi/krsi.c
> +++ b/security/krsi/krsi.c
> @@ -1,6 +1,9 @@
>   // SPDX-License-Identifier: GPL-2.0
>   
>   #include <linux/lsm_hooks.h>
> +#include <linux/filter.h>
> +#include <linux/bpf.h>
> +#include <linux/binfmts.h>
>   
>   #include "krsi_init.h"
>   
> @@ -16,7 +19,15 @@ struct krsi_hook krsi_hooks_list[] = {
>   
>   static int krsi_process_execution(struct linux_binprm *bprm)
>   {
> -	return 0;
> +	int ret;
> +	struct krsi_ctx ctx;
> +
> +	ctx.bprm_ctx = (struct krsi_bprm_ctx) {
> +		.bprm = bprm,
> +	};
> +
> +	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
> +	return ret;
>   }
>   
>   static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
> diff --git a/security/krsi/krsi_fs.c b/security/krsi/krsi_fs.c
> index 604f826cee5c..3ba18b52ce85 100644
> --- a/security/krsi/krsi_fs.c
> +++ b/security/krsi/krsi_fs.c
> @@ -5,6 +5,8 @@
>   #include <linux/file.h>
>   #include <linux/fs.h>
>   #include <linux/types.h>
> +#include <linux/filter.h>
> +#include <linux/bpf.h>
>   #include <linux/security.h>
>   
>   #include "krsi_fs.h"
> @@ -27,12 +29,29 @@ bool is_krsi_hook_file(struct file *f)
>   
>   static void __init krsi_free_hook(struct krsi_hook *h)
>   {
> +	struct bpf_prog_array_item *item;
> +	/*
> +	 * This function is __init so we are guarranteed that there will be
> +	 * no concurrent access.
> +	 */
> +	struct bpf_prog_array *progs = rcu_dereference_raw(h->progs);
> +
> +	if (progs) {

bpf_prog_array itself should never be null?

> +		item = progs->items;
> +		while (item->prog) {
> +			bpf_prog_put(item->prog);
> +			item++;
> +		}
> +		bpf_prog_array_free(progs);
> +	}
> +
>   	securityfs_remove(h->h_dentry);
>   	h->h_dentry = NULL;
>   }
>   
>   static int __init krsi_init_hook(struct krsi_hook *h, struct dentry *parent)
>   {
> +	struct bpf_prog_array __rcu     *progs;
>   	struct dentry *h_dentry;
>   	int ret;
>   
> @@ -41,6 +60,15 @@ static int __init krsi_init_hook(struct krsi_hook *h, struct dentry *parent)
>   
>   	if (IS_ERR(h_dentry))
>   		return PTR_ERR(h_dentry);
> +
> +	mutex_init(&h->mutex);
> +	progs = bpf_prog_array_alloc(0, GFP_KERNEL);
> +	if (!progs) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	RCU_INIT_POINTER(h->progs, progs);
>   	h_dentry->d_fsdata = h;
>   	h->h_dentry = h_dentry;
>   	return 0;
> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> index f2de3bd9621e..cf4d06189aa1 100644
> --- a/security/krsi/ops.c
> +++ b/security/krsi/ops.c
> @@ -1,10 +1,112 @@
>   // SPDX-License-Identifier: GPL-2.0
>   
> +#include <linux/err.h>
> +#include <linux/types.h>
>   #include <linux/filter.h>
>   #include <linux/bpf.h>
> +#include <linux/security.h>
> +#include <linux/krsi.h>
> +
> +#include "krsi_init.h"
> +#include "krsi_fs.h"
> +
> +extern struct krsi_hook krsi_hooks_list[];
> +
> +static struct krsi_hook *get_hook_from_fd(int fd)
> +{
> +	struct fd f = fdget(fd);
> +	struct krsi_hook *h;
> +	int ret;
> +
> +	if (!f.file) {
> +		ret = -EBADF;
> +		goto error;
> +	}
> +
> +	if (!is_krsi_hook_file(f.file)) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	/*
> +	 * The securityfs dentry never disappears, so we don't need to take a
> +	 * reference to it.
> +	 */
> +	h = file_dentry(f.file)->d_fsdata;
> +	if (WARN_ON(!h)) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +	fdput(f);
> +	return h;
> +
> +error:
> +	fdput(f);
> +	return ERR_PTR(ret);
> +}
> +
> +int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> +{
> +	struct bpf_prog_array *old_array;
> +	struct bpf_prog_array *new_array;
> +	struct krsi_hook *h;
> +	int ret = 0;
> +
> +	h = get_hook_from_fd(attr->target_fd);
> +	if (IS_ERR(h))
> +		return PTR_ERR(h);
> +
> +	mutex_lock(&h->mutex);
> +	old_array = rcu_dereference_protected(h->progs,
> +					      lockdep_is_held(&h->mutex));
> +
> +	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
> +	if (ret < 0) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	rcu_assign_pointer(h->progs, new_array);
> +	bpf_prog_array_free(old_array);
> +
> +unlock:
> +	mutex_unlock(&h->mutex);
> +	return ret;
> +}
>   
>   const struct bpf_prog_ops krsi_prog_ops = {
>   };
>   
> +static bool krsi_prog_is_valid_access(int off, int size,
> +				      enum bpf_access_type type,
> +				      const struct bpf_prog *prog,
> +				      struct bpf_insn_access_aux *info)
> +{
> +	/*
> +	 * KRSI is conservative about any direct access in eBPF to
> +	 * prevent the users from depending on the internals of the kernel and
> +	 * aims at providing a rich eco-system of safe eBPF helpers as an API
> +	 * for accessing relevant information from the context.
> +	 */
> +	return false;
> +}
> +
> +static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
> +							 func_id,
> +							 const struct bpf_prog
> +							 *prog)
> +{
> +	switch (func_id) {
> +	case BPF_FUNC_map_lookup_elem:
> +		return &bpf_map_lookup_elem_proto;
> +	case BPF_FUNC_get_current_pid_tgid:
> +		return &bpf_get_current_pid_tgid_proto;
> +	default:
> +		return NULL;
> +	}
> +}
> +
>   const struct bpf_verifier_ops krsi_verifier_ops = {
> +	.get_func_proto = krsi_prog_func_proto,
> +	.is_valid_access = krsi_prog_is_valid_access,
>   };
> 

Re: Re: [RFC v1 06/14] krsi: Implement eBPF operations, attachment and execution

[Yonghong Song]

On 9/14/19 5:56 PM, Yonghong Song wrote:
> 
> 
> On 9/10/19 12:55 PM, KP Singh wrote:
>> From: KP Singh <kpsingh@google.com>
>>
>> A user space program can attach an eBPF program by:
>>
>>     hook_fd = open("/sys/kernel/security/krsi/process_execution", O_RDWR)
>>     prog_fd = bpf(BPF_PROG_LOAD, ...)
>>     bpf(BPF_PROG_ATTACH, hook_fd, prog_fd)
>>
>> When such an attach call is received, the attachment logic looks up the
>> dentry and appends the program to the bpf_prog_array.
>>
>> The BPF programs are stored in a bpf_prog_array and writes to the array
>> are guarded by a mutex. The eBPF programs are executed as a part of the
>> LSM hook they are attached to. If any of the eBPF programs return
>> an error (-ENOPERM) the action represented by the hook is denied.
>>
>> Signed-off-by: KP Singh <kpsingh@google.com>
>> ---
>>    include/linux/krsi.h              |  18 ++++++
>>    kernel/bpf/syscall.c              |   3 +-
>>    security/krsi/include/krsi_init.h |  51 +++++++++++++++
>>    security/krsi/krsi.c              |  13 +++-
>>    security/krsi/krsi_fs.c           |  28 ++++++++
>>    security/krsi/ops.c               | 102 ++++++++++++++++++++++++++++++
>>    6 files changed, 213 insertions(+), 2 deletions(-)
>>    create mode 100644 include/linux/krsi.h
>>
[...]
>>    
>> +static inline int krsi_run_progs(enum krsi_hook_type t, struct krsi_ctx *ctx)
>> +{
>> +	struct bpf_prog_array_item *item;
>> +	struct bpf_prog *prog;
>> +	struct krsi_hook *h = &krsi_hooks_list[t];
>> +	int ret, retval = 0;
> 
> Reverse christmas tree style?
> 
>> +
>> +	preempt_disable();
> 
> Do we need preempt_disable() here?

 From the following patches, I see perf_event_output() helper
and per-cpu array usage. So, indeed preempt_disable() is needed.

> 
>> +	rcu_read_lock();
>> +
>> +	item = rcu_dereference(h->progs)->items;
>> +	while ((prog = READ_ONCE(item->prog))) {
>> +		ret = BPF_PROG_RUN(prog, ctx);
>> +		if (ret < 0) {
>> +			retval = ret;
>> +			goto out;
>> +		}
>> +		item++;
>> +	}
>> +
>> +out:
>> +	rcu_read_unlock();
>> +	preempt_enable();
>> +	return IS_ENABLED(CONFIG_SECURITY_KRSI_ENFORCE) ? retval : 0;
>> +}
>> +
[...]

[RFC v1 07/14] krsi: Check for premissions on eBPF attachment

[KP Singh]

From: KP Singh <kpsingh@google.com>

Add validation checks for the attachment of eBPF programs.

The following permissions are required:

- CAP_SYS_ADMIN to load eBPF programs
- CAP_MAC_ADMIN (to update the policy of an LSM)
- The securityfs file being a KRSI hook and writable (O_RDWR)

Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/krsi/ops.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/security/krsi/ops.c b/security/krsi/ops.c
index cf4d06189aa1..a61508b7018f 100644
--- a/security/krsi/ops.c
+++ b/security/krsi/ops.c
@@ -23,11 +23,31 @@ static struct krsi_hook *get_hook_from_fd(int fd)
 		goto error;
 	}
 
+	/*
+	 * Only CAP_MAC_ADMIN users are allowed to make
+	 * changes to LSM hooks
+	 */
+	if (!capable(CAP_MAC_ADMIN)) {
+		ret = -EPERM;
+		goto error;
+	}
+
 	if (!is_krsi_hook_file(f.file)) {
 		ret = -EINVAL;
 		goto error;
 	}
 
+	/*
+	 * It's wrong to attach the program to the hook
+	 * if the file is not opened for a write. Note that,
+	 * this is an EBADF and not an EPERM because the file
+	 * has been opened with an incorrect mode.
+	 */
+	if (!(f.file->f_mode & FMODE_WRITE)) {
+		ret = -EBADF;
+		goto error;
+	}
+
 	/*
 	 * The securityfs dentry never disappears, so we don't need to take a
 	 * reference to it.
-- 
2.20.1

[RFC v1 08/14] krsi: Show attached program names in hook read handler.

[KP Singh]

From: KP Singh <kpsingh@google.com>

For inspectability the system administrator should be able to view the
list of active KRSI programs:

   bash # cat /sys/kernel/security/krsi/process_execution
   bpf_prog1

Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/krsi/krsi_fs.c | 76 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 1 deletion(-)

diff --git a/security/krsi/krsi_fs.c b/security/krsi/krsi_fs.c
index 3ba18b52ce85..0ebf4fabe935 100644
--- a/security/krsi/krsi_fs.c
+++ b/security/krsi/krsi_fs.c
@@ -6,6 +6,7 @@
 #include <linux/fs.h>
 #include <linux/types.h>
 #include <linux/filter.h>
+#include <linux/seq_file.h>
 #include <linux/bpf.h>
 #include <linux/security.h>
 
@@ -16,8 +17,81 @@ extern struct krsi_hook krsi_hooks_list[];
 
 static struct dentry *krsi_dir;
 
+static void *seq_start(struct seq_file *m, loff_t *pos)
+	__acquires(rcu)
+{
+	struct krsi_hook *h;
+	struct dentry *dentry;
+	struct bpf_prog_array *progs;
+	struct bpf_prog_array_item *item;
+
+	/*
+	 * rcu_read_lock() must be held before any return statement
+	 * because the stop() will always be called and thus call
+	 * rcu_read_unlock()
+	 */
+	rcu_read_lock();
+
+	dentry = file_dentry(m->file);
+	h = dentry->d_fsdata;
+	if (WARN_ON(!h))
+		return ERR_PTR(-EFAULT);
+
+	progs = rcu_dereference(h->progs);
+	if ((*pos) >= bpf_prog_array_length(progs))
+		return NULL;
+
+	item = progs->items + *pos;
+	if (!item->prog)
+		return NULL;
+
+	return item;
+}
+
+static void *seq_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	struct bpf_prog_array_item *item = v;
+
+	item++;
+	++*pos;
+
+	if (!item->prog)
+		return NULL;
+
+	return item;
+}
+
+static void seq_stop(struct seq_file *m, void *v)
+	__releases(rcu)
+{
+	rcu_read_unlock();
+}
+
+static int show_prog(struct seq_file *m, void *v)
+{
+	struct bpf_prog_array_item *item = v;
+
+	seq_printf(m, "%s\n", item->prog->aux->name);
+	return 0;
+}
+
+static const struct seq_operations seq_ops = {
+	.show	= show_prog,
+	.start	= seq_start,
+	.next	= seq_next,
+	.stop	= seq_stop,
+};
+
+static int hook_open(struct inode *inode, struct file *file)
+{
+	return seq_open(file, &seq_ops);
+}
+
 static const struct file_operations krsi_hook_ops = {
-	.llseek = generic_file_llseek,
+	.open		= hook_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
 };
 
 int krsi_fs_initialized;
-- 
2.20.1

[RFC v1 09/14] krsi: Add a helper function for bpf_perf_event_output

[KP Singh]

From: KP Singh <kpsingh@google.com>

This helper is mapped to the existing operation
BPF_FUNC_perf_event_output.

An example usage of this function would be:

#define BUF_SIZE 64;

struct bpf_map_def SEC("maps") perf_map = {
        .type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
        .key_size = sizeof(int),
        .value_size = sizeof(u32),
        .max_entries = MAX_CPUS,
};

SEC("krsi")
int bpf_prog1(void *ctx)
{
	char buf[BUF_SIZE];
	int len;
	u64 flags = BPF_F_CURRENT_CPU;

	/* some logic that fills up buf with len data*/
	len = fill_up_buf(buf);
	if (len < 0)
		return len;
	if (len > BU)
		return 0;

	bpf_perf_event_output(ctx, &perf_map, flags, buf len);
	return 0;
}

A sample program that showcases the use of bpf_perf_event_output is
added later.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/krsi/ops.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/security/krsi/ops.c b/security/krsi/ops.c
index a61508b7018f..57bd304a03f4 100644
--- a/security/krsi/ops.c
+++ b/security/krsi/ops.c
@@ -111,6 +111,26 @@ static bool krsi_prog_is_valid_access(int off, int size,
 	return false;
 }
 
+BPF_CALL_5(krsi_event_output, void *, log,
+	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
+{
+	if (unlikely(flags & ~(BPF_F_INDEX_MASK)))
+		return -EINVAL;
+
+	return bpf_event_output(map, flags, data, size, NULL, 0, NULL);
+}
+
+static const struct bpf_func_proto krsi_event_output_proto =  {
+	.func		= krsi_event_output,
+	.gpl_only       = true,
+	.ret_type       = RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_CTX,
+	.arg2_type      = ARG_CONST_MAP_PTR,
+	.arg3_type      = ARG_ANYTHING,
+	.arg4_type      = ARG_PTR_TO_MEM,
+	.arg5_type      = ARG_CONST_SIZE_OR_ZERO,
+};
+
 static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
 							 func_id,
 							 const struct bpf_prog
@@ -121,6 +141,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
 		return &bpf_map_lookup_elem_proto;
 	case BPF_FUNC_get_current_pid_tgid:
 		return &bpf_get_current_pid_tgid_proto;
+	case BPF_FUNC_perf_event_output:
+		return &krsi_event_output_proto;
 	default:
 		return NULL;
 	}
-- 
2.20.1

Re: [RFC v1 09/14] krsi: Add a helper function for bpf_perf_event_output

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> This helper is mapped to the existing operation
> BPF_FUNC_perf_event_output.
> 
> An example usage of this function would be:
> 
> #define BUF_SIZE 64;
> 
> struct bpf_map_def SEC("maps") perf_map = {
>          .type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
>          .key_size = sizeof(int),
>          .value_size = sizeof(u32),
>          .max_entries = MAX_CPUS,
> };

could you use a map definition similar to
tools/testing/selftests/bpf/progs/test_perf_buffer.c?

struct {
         __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
         __uint(key_size, sizeof(int));
         __uint(value_size, sizeof(u32));
} perf_map SEC(".maps");

> 
> SEC("krsi")
> int bpf_prog1(void *ctx)
> {
> 	char buf[BUF_SIZE];
> 	int len;
> 	u64 flags = BPF_F_CURRENT_CPU;
> 
> 	/* some logic that fills up buf with len data*/
> 	len = fill_up_buf(buf);
> 	if (len < 0)
> 		return len;
> 	if (len > BU)
BUF_SIZE?
> 		return 0;
> 
> 	bpf_perf_event_output(ctx, &perf_map, flags, buf len);
buf, len?
> 	return 0;
> }
> 
> A sample program that showcases the use of bpf_perf_event_output is
> added later.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   security/krsi/ops.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> index a61508b7018f..57bd304a03f4 100644
> --- a/security/krsi/ops.c
> +++ b/security/krsi/ops.c
> @@ -111,6 +111,26 @@ static bool krsi_prog_is_valid_access(int off, int size,
>   	return false;
>   }
>   
> +BPF_CALL_5(krsi_event_output, void *, log,

Maybe name the first argument as 'ctx' to follow typical helper convention?

> +	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
> +{
> +	if (unlikely(flags & ~(BPF_F_INDEX_MASK)))
> +		return -EINVAL;
> +
> +	return bpf_event_output(map, flags, data, size, NULL, 0, NULL);
> +}
> +
> +static const struct bpf_func_proto krsi_event_output_proto =  {
> +	.func		= krsi_event_output,
> +	.gpl_only       = true,
> +	.ret_type       = RET_INTEGER,
> +	.arg1_type      = ARG_PTR_TO_CTX,
> +	.arg2_type      = ARG_CONST_MAP_PTR,
> +	.arg3_type      = ARG_ANYTHING,
> +	.arg4_type      = ARG_PTR_TO_MEM,
> +	.arg5_type      = ARG_CONST_SIZE_OR_ZERO,
> +};
> +
>   static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
>   							 func_id,
>   							 const struct bpf_prog
> @@ -121,6 +141,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
>   		return &bpf_map_lookup_elem_proto;
>   	case BPF_FUNC_get_current_pid_tgid:
>   		return &bpf_get_current_pid_tgid_proto;
> +	case BPF_FUNC_perf_event_output:
> +		return &krsi_event_output_proto;
>   	default:
>   		return NULL;
>   	}
> 

[RFC v1 10/14] krsi: Handle attachment of the same program

[KP Singh]

From: KP Singh <kpsingh@google.com>

Allow the userspace to attach a newer version of a program without
having duplicates of the same program.

If BPF_F_ALLOW_OVERRIDE is passed, the attachment logic compares the
name of the new program to the names of existing attached programs. The
names are only compared till a "__" (or '\0', if there is no "__"). If
a successful match is found, the existing program is replaced with the
newer attachment.

./krsi Attaches "env_dumper__v1" followed by "env_dumper__v2"
to the process_execution hook of the KRSI LSM.

./krsi
./krsi

Before:

  cat /sys/kernel/security/krsi/process_execution
  env_dumper__v1
  env_dumper__v2

After:

  cat /sys/kernel/security/krsi/process_execution
  env_dumper__v2

Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/krsi/ops.c | 53 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/security/krsi/ops.c b/security/krsi/ops.c
index 57bd304a03f4..1f4df920139c 100644
--- a/security/krsi/ops.c
+++ b/security/krsi/ops.c
@@ -65,11 +65,52 @@ static struct krsi_hook *get_hook_from_fd(int fd)
 	return ERR_PTR(ret);
 }
 
+/*
+ * match_prog_name matches the name of the program till "__"
+ * or the end of the string is encountered. This allows
+ * a different version of the same program to be loaded.
+ *
+ * For example:
+ *
+ *	env_dumper__v1 is matched with env_dumper__v2
+ *
+ */
+static bool match_prog_name(char *a, char *b)
+{
+	int m, n;
+	char *end;
+
+	end = strstr(a, "__");
+	n = end ? end - a : strlen(a);
+
+	end = strstr(b, "__");
+	m = end ? end - b : strlen(b);
+
+	if (m != n)
+		return false;
+
+	return strncmp(a, b, n) == 0;
+}
+
+static struct bpf_prog *find_attached_prog(struct bpf_prog_array *array,
+					   struct bpf_prog *prog)
+{
+	struct bpf_prog_array_item *item = array->items;
+
+	for (; item->prog; item++) {
+		if (match_prog_name(item->prog->aux->name, prog->aux->name))
+			return item->prog;
+	}
+
+	return NULL;
+}
+
 int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	struct bpf_prog_array *old_array;
 	struct bpf_prog_array *new_array;
 	struct krsi_hook *h;
+	struct bpf_prog *old_prog;
 	int ret = 0;
 
 	h = get_hook_from_fd(attr->target_fd);
@@ -79,8 +120,18 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 	mutex_lock(&h->mutex);
 	old_array = rcu_dereference_protected(h->progs,
 					      lockdep_is_held(&h->mutex));
+	/*
+	 * Check if a matching program with already exists and replace
+	 * the existing program will be overridden if BPF_F_ALLOW_OVERRIDE
+	 * is specified in the attach flags.
+	 */
+	old_prog = find_attached_prog(old_array, prog);
+	if (old_prog && !(attr->attach_flags & BPF_F_ALLOW_OVERRIDE)) {
+		ret = -EEXIST;
+		goto unlock;
+	}
 
-	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+	ret = bpf_prog_array_copy(old_array, old_prog, prog, &new_array);
 	if (ret < 0) {
 		ret = -ENOMEM;
 		goto unlock;
-- 
2.20.1

[RFC v1 11/14] krsi: Pin argument pages in bprm_check_security hook

[KP Singh]

From: KP Singh <kpsingh@google.com>

Pin the memory allocated to the the argv + envv for the new
process and passes it in the context to the eBPF programs attached to
the hook.

The get_user_pages_remote cannot be called from an eBPF helper because
the helpers run in atomic context and the get_user_pages_remote function
can sleep.

The following heuristics can be added as an optimization:

- Don't pin the pages if no eBPF programs are attached.
- Don't pin the pages if none of the eBPF programs depend on the
  information. This would require introspection of the byte-code and
  checking if certain helpers are called.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/krsi/include/krsi_init.h |  3 ++
 security/krsi/krsi.c              | 56 +++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
index 4e17ecacd4ed..6152847c3b08 100644
--- a/security/krsi/include/krsi_init.h
+++ b/security/krsi/include/krsi_init.h
@@ -16,6 +16,9 @@ extern int krsi_fs_initialized;
 
 struct krsi_bprm_ctx {
 	struct linux_binprm *bprm;
+	char *arg_pages;
+	unsigned long num_arg_pages;
+	unsigned long max_arg_offset;
 };
 
 /*
diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
index d3a4a361c192..00a7150c1b22 100644
--- a/security/krsi/krsi.c
+++ b/security/krsi/krsi.c
@@ -4,6 +4,8 @@
 #include <linux/filter.h>
 #include <linux/bpf.h>
 #include <linux/binfmts.h>
+#include <linux/highmem.h>
+#include <linux/mm.h>
 
 #include "krsi_init.h"
 
@@ -17,6 +19,53 @@ struct krsi_hook krsi_hooks_list[] = {
 	#undef KRSI_HOOK_INIT
 };
 
+static int pin_arg_pages(struct krsi_bprm_ctx *ctx)
+{
+	int ret = 0;
+	char *kaddr;
+	struct page *page;
+	unsigned long i, pos, num_arg_pages;
+	struct linux_binprm *bprm = ctx->bprm;
+	char *buf;
+
+	/*
+	 * The bprm->vma_pages does not have the correct count
+	 * for execution that is done by a kernel thread using the UMH.
+	 * vm_pages is updated in acct_arg_size and bails
+	 * out if current->mm is NULL (which is the case for a kernel thread).
+	 * It's safer to use vma_pages(struct linux_binprm*) to get the
+	 * actual number
+	 */
+	num_arg_pages = vma_pages(bprm->vma);
+	if (!num_arg_pages)
+		return -ENOMEM;
+
+	buf = kmalloc_array(num_arg_pages, PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for (i = 0; i < num_arg_pages; i++) {
+		pos = ALIGN_DOWN(bprm->p, PAGE_SIZE) + i * PAGE_SIZE;
+		ret = get_user_pages_remote(current, bprm->mm, pos, 1,
+					    FOLL_FORCE, &page, NULL, NULL);
+		if (ret <= 0) {
+			kfree(buf);
+			return -ENOMEM;
+		}
+
+		kaddr = kmap(page);
+		memcpy(buf + i * PAGE_SIZE, kaddr, PAGE_SIZE);
+		kunmap(page);
+		put_page(page);
+	}
+
+	ctx->arg_pages = buf;
+	ctx->num_arg_pages = num_arg_pages;
+	ctx->max_arg_offset = num_arg_pages * PAGE_SIZE;
+
+	return 0;
+}
+
 static int krsi_process_execution(struct linux_binprm *bprm)
 {
 	int ret;
@@ -26,7 +75,14 @@ static int krsi_process_execution(struct linux_binprm *bprm)
 		.bprm = bprm,
 	};
 
+	ret = pin_arg_pages(&ctx.bprm_ctx);
+	if (ret < 0)
+		goto out_arg_pages;
+
 	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
+	kfree(ctx.bprm_ctx.arg_pages);
+
+out_arg_pages:
 	return ret;
 }
 
-- 
2.20.1

[RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable

[KP Singh]

From: KP Singh <kpsingh@google.com>

The helper returns the value of the environment variable in the buffer
that is passed to it. If the var is set multiple times, the helper
returns all the values as null separated strings.

If the buffer is too short for these values, the helper tries to fill it
the best it can and guarantees that the value returned in the buffer
is always null terminated. After the buffer is filled, the helper keeps
counting the number of times the environment variable is set in the
envp.

The return value of the helper is an u64 value which carries two pieces
of information.

  * The upper 32 bits are a u32 value signifying the number of times
    the environment variable is set in the envp.
  * The lower 32 bits are a s32 value signifying the number of bytes
    written to the buffer or an error code.

Since the value of the environment variable can be very long and exceed
what can be allocated on the BPF stack, a per-cpu array can be used
instead:

struct bpf_map_def SEC("maps") env_map = {
        .type = BPF_MAP_TYPE_PERCPU_ARRAY,
        .key_size = sizeof(u32),
        .value_size = 4096,
        .max_entries = 1,
};

SEC("prgrm")
int bpf_prog1(void *ctx)
{
        u32 map_id = 0;
        u64 times_ret;
        s32 ret;
        char name[48] = "LD_PRELOAD";

        char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
        if (!map_value)
                return 0;

        // Read the lower 32 bits for the return value
        times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
        ret = times_ret & 0xffffffff;
        if (ret < 0)
                return ret;
        return 0;
}

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/uapi/linux/bpf.h                  |  42 ++++++-
 security/krsi/ops.c                       | 129 ++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h            |  42 ++++++-
 tools/testing/selftests/bpf/bpf_helpers.h |   3 +
 4 files changed, 214 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 32ab38f1a2fe..a4ef07956e07 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2715,6 +2715,45 @@ union bpf_attr {
  *		**-EPERM** if no permission to send the *sig*.
  *
  *		**-EAGAIN** if bpf program can try again.
+ *
+ * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
+ *			size_t name_len, size_t buf_len)
+ *	Description
+ *		This helper can be used as a part of the
+ *		process_execution hook of the KRSI LSM in
+ *		programs of type BPF_PROG_TYPE_KRSI.
+ *
+ *		The helper returns the value of the environment
+ *		variable with the provided "name" for process that's
+ *		going to be executed in the passed buffer, "buf". If the var
+ *		is set multiple times, the helper returns all
+ *		the values as null separated strings.
+ *
+ *		If the buffer is too short for these values, the helper
+ *		tries to fill it the best it can and guarantees that the value
+ *		returned in the buffer  is always null terminated.
+ *		After the buffer is filled, the helper keeps counting the number
+ *		of times the environment variable is set in the envp.
+ *
+ *	Return:
+ *
+ *		The return value of the helper is an u64 value
+ *		which carries two pieces of information:
+ *
+ *		   The upper 32 bits are a u32 value signifying
+ *		   the number of times the environment variable
+ *		   is set in the envp.
+ *		   The lower 32 bits are an s32 value signifying
+ *		   the number of bytes written to the buffer or an error code:
+ *
+ *		**-ENOMEM** if the kernel is unable to allocate memory
+ *			    for pinning the argv and envv.
+ *
+ *		**-E2BIG** if the value is larger than the size of the
+ *			   destination buffer. The higher bits will still
+ *			   the number of times the variable was set in the envp.
+ *
+ *		**-EINVAL** if name is not a NULL terminated string.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2826,7 +2865,8 @@ union bpf_attr {
 	FN(strtoul),			\
 	FN(sk_storage_get),		\
 	FN(sk_storage_delete),		\
-	FN(send_signal),
+	FN(send_signal),		\
+	FN(krsi_get_env_var),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/security/krsi/ops.c b/security/krsi/ops.c
index 1f4df920139c..1db94dfaac15 100644
--- a/security/krsi/ops.c
+++ b/security/krsi/ops.c
@@ -6,6 +6,8 @@
 #include <linux/bpf.h>
 #include <linux/security.h>
 #include <linux/krsi.h>
+#include <linux/binfmts.h>
+#include <linux/highmem.h>
 
 #include "krsi_init.h"
 #include "krsi_fs.h"
@@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
 	return false;
 }
 
+static char *array_next_entry(char *array, unsigned long *offset,
+			      unsigned long end)
+{
+	char *entry;
+	unsigned long current_offset = *offset;
+
+	if (current_offset >= end)
+		return NULL;
+
+	/*
+	 * iterate on the array till the null byte is encountered
+	 * and check for any overflows.
+	 */
+	entry = array + current_offset;
+	while (array[current_offset]) {
+		if (unlikely(++current_offset >= end))
+			return NULL;
+	}
+
+	/*
+	 * Point the offset to the next element in the array.
+	 */
+	*offset = current_offset + 1;
+
+	return entry;
+}
+
+static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
+		u32 n_size, u32 size)
+{
+	s32 ret = 0;
+	u32 num_vars = 0;
+	int i, name_len;
+	struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
+	int argc = bprm->argc;
+	int envc = bprm->envc;
+	unsigned long end = ctx->bprm_ctx.max_arg_offset;
+	unsigned long offset = bprm->p % PAGE_SIZE;
+	char *buf = ctx->bprm_ctx.arg_pages;
+	char *curr_dest = dest;
+	char *entry;
+
+	if (unlikely(!buf))
+		return -ENOMEM;
+
+	for (i = 0; i < argc; i++) {
+		entry = array_next_entry(buf, &offset, end);
+		if (!entry)
+			return 0;
+	}
+
+	name_len = strlen(name);
+	for (i = 0; i < envc; i++) {
+		entry = array_next_entry(buf, &offset, end);
+		if (!entry)
+			return 0;
+
+		if (!strncmp(entry, name, name_len)) {
+			num_vars++;
+
+			/*
+			 * There is no need to do further copying
+			 * if the buffer is already full. Just count how many
+			 * times the environment variable is set.
+			 */
+			if (ret == -E2BIG)
+				continue;
+
+			if (entry[name_len] != '=')
+				continue;
+
+			/*
+			 * Move the buf pointer by name_len + 1
+			 * (for the "=" sign)
+			 */
+			entry += name_len + 1;
+			ret = strlcpy(curr_dest, entry, size);
+
+			if (ret >= size) {
+				ret = -E2BIG;
+				continue;
+			}
+
+			/*
+			 * strlcpy just returns the length of the string copied.
+			 * The remaining space needs to account for the added
+			 * null character.
+			 */
+			curr_dest += ret + 1;
+			size -= ret + 1;
+			/*
+			 * Update ret to be the current number of bytes written
+			 * to the destination
+			 */
+			ret = curr_dest - dest;
+		}
+	}
+
+	return (u64) num_vars << 32 | (u32) ret;
+}
+
+BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
+	  char *, dest, u32, size)
+{
+	char *name_end;
+
+	name_end = memchr(name, '\0', n_size);
+	if (!name_end)
+		return -EINVAL;
+
+	memset(dest, 0, size);
+	return get_env_var(ctx, name, dest, n_size, size);
+}
+
+static const struct bpf_func_proto krsi_get_env_var_proto = {
+	.func = krsi_get_env_var,
+	.gpl_only = true,
+	.ret_type = RET_INTEGER,
+	.arg1_type = ARG_PTR_TO_CTX,
+	.arg2_type = ARG_PTR_TO_MEM,
+	.arg3_type = ARG_CONST_SIZE_OR_ZERO,
+	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
+	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
 BPF_CALL_5(krsi_event_output, void *, log,
 	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
 {
@@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
 		return &bpf_map_lookup_elem_proto;
 	case BPF_FUNC_get_current_pid_tgid:
 		return &bpf_get_current_pid_tgid_proto;
+	case BPF_FUNC_krsi_get_env_var:
+		return &krsi_get_env_var_proto;
 	case BPF_FUNC_perf_event_output:
 		return &krsi_event_output_proto;
 	default:
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 32ab38f1a2fe..a4ef07956e07 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2715,6 +2715,45 @@ union bpf_attr {
  *		**-EPERM** if no permission to send the *sig*.
  *
  *		**-EAGAIN** if bpf program can try again.
+ *
+ * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
+ *			size_t name_len, size_t buf_len)
+ *	Description
+ *		This helper can be used as a part of the
+ *		process_execution hook of the KRSI LSM in
+ *		programs of type BPF_PROG_TYPE_KRSI.
+ *
+ *		The helper returns the value of the environment
+ *		variable with the provided "name" for process that's
+ *		going to be executed in the passed buffer, "buf". If the var
+ *		is set multiple times, the helper returns all
+ *		the values as null separated strings.
+ *
+ *		If the buffer is too short for these values, the helper
+ *		tries to fill it the best it can and guarantees that the value
+ *		returned in the buffer  is always null terminated.
+ *		After the buffer is filled, the helper keeps counting the number
+ *		of times the environment variable is set in the envp.
+ *
+ *	Return:
+ *
+ *		The return value of the helper is an u64 value
+ *		which carries two pieces of information:
+ *
+ *		   The upper 32 bits are a u32 value signifying
+ *		   the number of times the environment variable
+ *		   is set in the envp.
+ *		   The lower 32 bits are an s32 value signifying
+ *		   the number of bytes written to the buffer or an error code:
+ *
+ *		**-ENOMEM** if the kernel is unable to allocate memory
+ *			    for pinning the argv and envv.
+ *
+ *		**-E2BIG** if the value is larger than the size of the
+ *			   destination buffer. The higher bits will still
+ *			   the number of times the variable was set in the envp.
+ *
+ *		**-EINVAL** if name is not a NULL terminated string.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2826,7 +2865,8 @@ union bpf_attr {
 	FN(strtoul),			\
 	FN(sk_storage_get),		\
 	FN(sk_storage_delete),		\
-	FN(send_signal),
+	FN(send_signal),		\
+	FN(krsi_get_env_var),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index f804f210244e..ecebdb772a9d 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
 static int (*bpf_probe_read_str)(void *ctx, __u32 size,
 				 const void *unsafe_ptr) =
 	(void *) BPF_FUNC_probe_read_str;
+static unsigned long long (*krsi_get_env_var)(void *ctx,
+	void *name, __u32 n_size, void *buf, __u32 size) =
+	(void *) BPF_FUNC_krsi_get_env_var;
 static unsigned int (*bpf_get_socket_uid)(void *ctx) =
 	(void *) BPF_FUNC_get_socket_uid;
 static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
-- 
2.20.1

Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>

This patch cannot apply cleanly.

-bash-4.4$ git apply ~/p12.txt
error: patch failed: include/uapi/linux/bpf.h:2715
error: include/uapi/linux/bpf.h: patch does not apply
error: patch failed: tools/include/uapi/linux/bpf.h:2715
error: tools/include/uapi/linux/bpf.h: patch does not apply
-bash-4.4$

> 
> The helper returns the value of the environment variable in the buffer
> that is passed to it. If the var is set multiple times, the helper
> returns all the values as null separated strings.
> 
> If the buffer is too short for these values, the helper tries to fill it
> the best it can and guarantees that the value returned in the buffer
> is always null terminated. After the buffer is filled, the helper keeps
> counting the number of times the environment variable is set in the
> envp.
> 
> The return value of the helper is an u64 value which carries two pieces
> of information.
> 
>    * The upper 32 bits are a u32 value signifying the number of times
>      the environment variable is set in the envp.

Not sure how useful this 'upper 32' bit value is. What user expected to do?

Another option is to have upper 32 bits encode the required buffer size
to hold all values. This may cause some kind of user space action, e.g.,
to replace the program with new program with larger per cpu map value size?

>    * The lower 32 bits are a s32 value signifying the number of bytes
>      written to the buffer or an error code. >
> Since the value of the environment variable can be very long and exceed
> what can be allocated on the BPF stack, a per-cpu array can be used
> instead:
> 
> struct bpf_map_def SEC("maps") env_map = {
>          .type = BPF_MAP_TYPE_PERCPU_ARRAY,
>          .key_size = sizeof(u32),
>          .value_size = 4096,
>          .max_entries = 1,
> };

Could you use use map definition with SEC(".maps")?

> 
> SEC("prgrm")
> int bpf_prog1(void *ctx)
> {
>          u32 map_id = 0;
>          u64 times_ret;
>          s32 ret;
>          char name[48] = "LD_PRELOAD";

Reverse Christmas tree coding style, here and other places?

> 
>          char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
>          if (!map_value)
>                  return 0;
> 
>          // Read the lower 32 bits for the return value
>          times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
>          ret = times_ret & 0xffffffff;
>          if (ret < 0)
>                  return ret;
>          return 0;
> }
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   include/uapi/linux/bpf.h                  |  42 ++++++-
>   security/krsi/ops.c                       | 129 ++++++++++++++++++++++
>   tools/include/uapi/linux/bpf.h            |  42 ++++++-
>   tools/testing/selftests/bpf/bpf_helpers.h |   3 +
>   4 files changed, 214 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 32ab38f1a2fe..a4ef07956e07 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2715,6 +2715,45 @@ union bpf_attr {
>    *		**-EPERM** if no permission to send the *sig*.
>    *
>    *		**-EAGAIN** if bpf program can try again.
> + *
> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> + *			size_t name_len, size_t buf_len)

This signature is not the same as the later
krsi_get_env_var(...) helper definition.
BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, 
n_size,
	  char *, dest, u32, size)

> + *	Description
> + *		This helper can be used as a part of the
> + *		process_execution hook of the KRSI LSM in
> + *		programs of type BPF_PROG_TYPE_KRSI.
> + *
> + *		The helper returns the value of the environment
> + *		variable with the provided "name" for process that's
> + *		going to be executed in the passed buffer, "buf". If the var
> + *		is set multiple times, the helper returns all
> + *		the values as null separated strings.
> + *
> + *		If the buffer is too short for these values, the helper
> + *		tries to fill it the best it can and guarantees that the value
> + *		returned in the buffer  is always null terminated.
> + *		After the buffer is filled, the helper keeps counting the number
> + *		of times the environment variable is set in the envp.
> + *
> + *	Return:
> + *
> + *		The return value of the helper is an u64 value
> + *		which carries two pieces of information:
> + *
> + *		   The upper 32 bits are a u32 value signifying
> + *		   the number of times the environment variable
> + *		   is set in the envp.
> + *		   The lower 32 bits are an s32 value signifying
> + *		   the number of bytes written to the buffer or an error code:
> + *
> + *		**-ENOMEM** if the kernel is unable to allocate memory
> + *			    for pinning the argv and envv.
> + *
> + *		**-E2BIG** if the value is larger than the size of the
> + *			   destination buffer. The higher bits will still
> + *			   the number of times the variable was set in the envp.

The -E2BIG is returned because buffer sizee is not big enough.
Another possible error code is -ENOSPC, which typically indicates
buffer size not big enough.

> + *
> + *		**-EINVAL** if name is not a NULL terminated string.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -2826,7 +2865,8 @@ union bpf_attr {
>   	FN(strtoul),			\
>   	FN(sk_storage_get),		\
>   	FN(sk_storage_delete),		\
> -	FN(send_signal),
> +	FN(send_signal),		\
> +	FN(krsi_get_env_var),
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>    * function eBPF program intends to call
> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> index 1f4df920139c..1db94dfaac15 100644
> --- a/security/krsi/ops.c
> +++ b/security/krsi/ops.c
> @@ -6,6 +6,8 @@
>   #include <linux/bpf.h>
>   #include <linux/security.h>
>   #include <linux/krsi.h>
> +#include <linux/binfmts.h>
> +#include <linux/highmem.h>
>   
>   #include "krsi_init.h"
>   #include "krsi_fs.h"
> @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
>   	return false;
>   }
>   
> +static char *array_next_entry(char *array, unsigned long *offset,
> +			      unsigned long end)
> +{
> +	char *entry;
> +	unsigned long current_offset = *offset;
> +
> +	if (current_offset >= end)
> +		return NULL;
> +
> +	/*
> +	 * iterate on the array till the null byte is encountered
> +	 * and check for any overflows.
> +	 */
> +	entry = array + current_offset;
> +	while (array[current_offset]) {
> +		if (unlikely(++current_offset >= end))
> +			return NULL;
> +	}
> +
> +	/*
> +	 * Point the offset to the next element in the array.
> +	 */
> +	*offset = current_offset + 1;
> +
> +	return entry;
> +}
> +
> +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
> +		u32 n_size, u32 size)
> +{
> +	s32 ret = 0;
> +	u32 num_vars = 0;
> +	int i, name_len;
> +	struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
> +	int argc = bprm->argc;
> +	int envc = bprm->envc;
> +	unsigned long end = ctx->bprm_ctx.max_arg_offset;
> +	unsigned long offset = bprm->p % PAGE_SIZE;

why we need bprm->p % PAGE_SIZE instead of bprm->p?

> +	char *buf = ctx->bprm_ctx.arg_pages;
> +	char *curr_dest = dest;
> +	char *entry;
> +
> +	if (unlikely(!buf))
> +		return -ENOMEM;
> +
> +	for (i = 0; i < argc; i++) {
> +		entry = array_next_entry(buf, &offset, end);
> +		if (!entry)
> +			return 0;
> +	}
> +
> +	name_len = strlen(name);
> +	for (i = 0; i < envc; i++) {
> +		entry = array_next_entry(buf, &offset, end);
> +		if (!entry)
> +			return 0;

If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
we may skip the first entry?


> +
> +		if (!strncmp(entry, name, name_len)) {
> +			num_vars++;

There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.

> +
> +			/*
> +			 * There is no need to do further copying
> +			 * if the buffer is already full. Just count how many
> +			 * times the environment variable is set.
> +			 */
> +			if (ret == -E2BIG)
> +				continue;
> +
> +			if (entry[name_len] != '=')
> +				continue;
> +
> +			/*
> +			 * Move the buf pointer by name_len + 1
> +			 * (for the "=" sign)
> +			 */
> +			entry += name_len + 1;
> +			ret = strlcpy(curr_dest, entry, size);
> +
> +			if (ret >= size) {
> +				ret = -E2BIG;

Here, we have a partial copy. Should you instead nullify (memset) it as 
it is not really invalid one?

> +				continue;
> +			}
> +
> +			/*
> +			 * strlcpy just returns the length of the string copied.
> +			 * The remaining space needs to account for the added
> +			 * null character.
> +			 */
> +			curr_dest += ret + 1;
> +			size -= ret + 1;
> +			/*
> +			 * Update ret to be the current number of bytes written
> +			 * to the destination
> +			 */
> +			ret = curr_dest - dest;
> +		}
> +	}
> +
> +	return (u64) num_vars << 32 | (u32) ret;
> +}
> +
> +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> +	  char *, dest, u32, size)
> +{
> +	char *name_end;
> +
> +	name_end = memchr(name, '\0', n_size);
> +	if (!name_end)
> +		return -EINVAL;
> +
> +	memset(dest, 0, size);
> +	return get_env_var(ctx, name, dest, n_size, size);
> +}
> +
> +static const struct bpf_func_proto krsi_get_env_var_proto = {
> +	.func = krsi_get_env_var,
> +	.gpl_only = true,
> +	.ret_type = RET_INTEGER,
> +	.arg1_type = ARG_PTR_TO_CTX,
> +	.arg2_type = ARG_PTR_TO_MEM,
> +	.arg3_type = ARG_CONST_SIZE_OR_ZERO,
> +	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
> +	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
> +};
> +
>   BPF_CALL_5(krsi_event_output, void *, log,
>   	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
>   {
> @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
>   		return &bpf_map_lookup_elem_proto;
>   	case BPF_FUNC_get_current_pid_tgid:
>   		return &bpf_get_current_pid_tgid_proto;
> +	case BPF_FUNC_krsi_get_env_var:
> +		return &krsi_get_env_var_proto;
>   	case BPF_FUNC_perf_event_output:
>   		return &krsi_event_output_proto;
>   	default:
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 32ab38f1a2fe..a4ef07956e07 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2715,6 +2715,45 @@ union bpf_attr {
>    *		**-EPERM** if no permission to send the *sig*.
>    *
>    *		**-EAGAIN** if bpf program can try again.
> + *
> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> + *			size_t name_len, size_t buf_len)

Inconsistent helper definitions.

> + *	Description
> + *		This helper can be used as a part of the
> + *		process_execution hook of the KRSI LSM in
> + *		programs of type BPF_PROG_TYPE_KRSI.
> + *
> + *		The helper returns the value of the environment
> + *		variable with the provided "name" for process that's
> + *		going to be executed in the passed buffer, "buf". If the var
> + *		is set multiple times, the helper returns all
> + *		the values as null separated strings.
> + *
> + *		If the buffer is too short for these values, the helper
> + *		tries to fill it the best it can and guarantees that the value
> + *		returned in the buffer  is always null terminated.
> + *		After the buffer is filled, the helper keeps counting the number
> + *		of times the environment variable is set in the envp.
> + *
> + *	Return:
> + *
> + *		The return value of the helper is an u64 value
> + *		which carries two pieces of information:
> + *
> + *		   The upper 32 bits are a u32 value signifying
> + *		   the number of times the environment variable
> + *		   is set in the envp.
> + *		   The lower 32 bits are an s32 value signifying
> + *		   the number of bytes written to the buffer or an error code:
> + *
> + *		**-ENOMEM** if the kernel is unable to allocate memory
> + *			    for pinning the argv and envv.
> + *
> + *		**-E2BIG** if the value is larger than the size of the
> + *			   destination buffer. The higher bits will still
> + *			   the number of times the variable was set in the envp.
> + *
> + *		**-EINVAL** if name is not a NULL terminated string.
>    */
>   #define __BPF_FUNC_MAPPER(FN)		\
>   	FN(unspec),			\
> @@ -2826,7 +2865,8 @@ union bpf_attr {
>   	FN(strtoul),			\
>   	FN(sk_storage_get),		\
>   	FN(sk_storage_delete),		\
> -	FN(send_signal),
> +	FN(send_signal),		\
> +	FN(krsi_get_env_var),
>   
>   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>    * function eBPF program intends to call
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index f804f210244e..ecebdb772a9d 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
>   static int (*bpf_probe_read_str)(void *ctx, __u32 size,
>   				 const void *unsafe_ptr) =
>   	(void *) BPF_FUNC_probe_read_str;
> +static unsigned long long (*krsi_get_env_var)(void *ctx,
> +	void *name, __u32 n_size, void *buf, __u32 size) =
> +	(void *) BPF_FUNC_krsi_get_env_var;
>   static unsigned int (*bpf_get_socket_uid)(void *ctx) =
>   	(void *) BPF_FUNC_get_socket_uid;
>   static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> 

Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable

[KP Singh]

Thanks for reviewing!

On 15-Sep 00:16, Yonghong Song wrote:
> 
> 
> On 9/10/19 12:55 PM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> 
> This patch cannot apply cleanly.
> 
> -bash-4.4$ git apply ~/p12.txt
> error: patch failed: include/uapi/linux/bpf.h:2715
> error: include/uapi/linux/bpf.h: patch does not apply
> error: patch failed: tools/include/uapi/linux/bpf.h:2715
> error: tools/include/uapi/linux/bpf.h: patch does not apply
> -bash-4.4$

I am not sure why this is happening, I tried:

git clone \
  https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git && \
  cd linux-next && \
  git checkout -b review v5.3-rc6 && \
  wget -P /tmp https://lore.kernel.org/patchwork/series/410101/mbox && \
  git am /tmp/mbox

and it worked.

This seems to work too:

  patch -p1 < <file>.patch

Can you try with "git am" please?

> 
> > 
> > The helper returns the value of the environment variable in the buffer
> > that is passed to it. If the var is set multiple times, the helper
> > returns all the values as null separated strings.
> > 
> > If the buffer is too short for these values, the helper tries to fill it
> > the best it can and guarantees that the value returned in the buffer
> > is always null terminated. After the buffer is filled, the helper keeps
> > counting the number of times the environment variable is set in the
> > envp.
> > 
> > The return value of the helper is an u64 value which carries two pieces
> > of information.
> > 
> >    * The upper 32 bits are a u32 value signifying the number of times
> >      the environment variable is set in the envp.
> 
> Not sure how useful this 'upper 32' bit value is. What user expected to do?
> 
> Another option is to have upper 32 bits encode the required buffer size
> to hold all values. This may cause some kind of user space action, e.g.,
> to replace the program with new program with larger per cpu map value size?
> 

The upper 32-bit value is actually an important part of the LSM's MAC
policy. It allows the user to:

- Return an -EPERM when if the environment variable is set more than
  once.
- Log a warning (this is what we are doing in the example) so
  this is flagged as a potential malicious actor.

> >    * The lower 32 bits are a s32 value signifying the number of bytes
> >      written to the buffer or an error code. >
> > Since the value of the environment variable can be very long and exceed
> > what can be allocated on the BPF stack, a per-cpu array can be used
> > instead:
> > 
> > struct bpf_map_def SEC("maps") env_map = {
> >          .type = BPF_MAP_TYPE_PERCPU_ARRAY,
> >          .key_size = sizeof(u32),
> >          .value_size = 4096,
> >          .max_entries = 1,
> > };
> 
> Could you use use map definition with SEC(".maps")?

Sure, I added this example program in the commit message. Will update
it to be more canonical. Thanks!

> 
> > 
> > SEC("prgrm")
> > int bpf_prog1(void *ctx)
> > {
> >          u32 map_id = 0;
> >          u64 times_ret;
> >          s32 ret;
> >          char name[48] = "LD_PRELOAD";
> 
> Reverse Christmas tree coding style, here and other places?

Will happily fix it.

However, I did not find it mentioned in the style guide:

  https://www.kernel.org/doc/html/v4.10/process/coding-style.html
  https://elixir.bootlin.com/linux/v4.6/source/Documentation/CodingStyle

Is there one specific to BPF?


> 
> > 
> >          char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
> >          if (!map_value)
> >                  return 0;
> > 
> >          // Read the lower 32 bits for the return value
> >          times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
> >          ret = times_ret & 0xffffffff;
> >          if (ret < 0)
> >                  return ret;
> >          return 0;
> > }
> > 
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >   include/uapi/linux/bpf.h                  |  42 ++++++-
> >   security/krsi/ops.c                       | 129 ++++++++++++++++++++++
> >   tools/include/uapi/linux/bpf.h            |  42 ++++++-
> >   tools/testing/selftests/bpf/bpf_helpers.h |   3 +
> >   4 files changed, 214 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 32ab38f1a2fe..a4ef07956e07 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2715,6 +2715,45 @@ union bpf_attr {
> >    *		**-EPERM** if no permission to send the *sig*.
> >    *
> >    *		**-EAGAIN** if bpf program can try again.
> > + *
> > + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> > + *			size_t name_len, size_t buf_len)
> 
> This signature is not the same as the later
> krsi_get_env_var(...) helper definition.
> BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, 
> n_size,
> 	  char *, dest, u32, size)
> 

I did this because the krsi_ctx is not exposed to the userspace and
allows KRSI to modify the context without worrying about breaking
userspace.

That said, I could mark it as a (void *) here and cast it internally.
I guess that would be better/cleaner?

> > + *	Description
> > + *		This helper can be used as a part of the
> > + *		process_execution hook of the KRSI LSM in
> > + *		programs of type BPF_PROG_TYPE_KRSI.
> > + *
> > + *		The helper returns the value of the environment
> > + *		variable with the provided "name" for process that's
> > + *		going to be executed in the passed buffer, "buf". If the var
> > + *		is set multiple times, the helper returns all
> > + *		the values as null separated strings.
> > + *
> > + *		If the buffer is too short for these values, the helper
> > + *		tries to fill it the best it can and guarantees that the value
> > + *		returned in the buffer  is always null terminated.
> > + *		After the buffer is filled, the helper keeps counting the number
> > + *		of times the environment variable is set in the envp.
> > + *
> > + *	Return:
> > + *
> > + *		The return value of the helper is an u64 value
> > + *		which carries two pieces of information:
> > + *
> > + *		   The upper 32 bits are a u32 value signifying
> > + *		   the number of times the environment variable
> > + *		   is set in the envp.
> > + *		   The lower 32 bits are an s32 value signifying
> > + *		   the number of bytes written to the buffer or an error code:
> > + *
> > + *		**-ENOMEM** if the kernel is unable to allocate memory
> > + *			    for pinning the argv and envv.
> > + *
> > + *		**-E2BIG** if the value is larger than the size of the
> > + *			   destination buffer. The higher bits will still
> > + *			   the number of times the variable was set in the envp.
> 
> The -E2BIG is returned because buffer sizee is not big enough.
> Another possible error code is -ENOSPC, which typically indicates
> buffer size not big enough.

Sure, I am fine with using either.

> 
> > + *
> > + *		**-EINVAL** if name is not a NULL terminated string.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)		\
> >   	FN(unspec),			\
> > @@ -2826,7 +2865,8 @@ union bpf_attr {
> >   	FN(strtoul),			\
> >   	FN(sk_storage_get),		\
> >   	FN(sk_storage_delete),		\
> > -	FN(send_signal),
> > +	FN(send_signal),		\
> > +	FN(krsi_get_env_var),
> >   
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >    * function eBPF program intends to call
> > diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> > index 1f4df920139c..1db94dfaac15 100644
> > --- a/security/krsi/ops.c
> > +++ b/security/krsi/ops.c
> > @@ -6,6 +6,8 @@
> >   #include <linux/bpf.h>
> >   #include <linux/security.h>
> >   #include <linux/krsi.h>
> > +#include <linux/binfmts.h>
> > +#include <linux/highmem.h>
> >   
> >   #include "krsi_init.h"
> >   #include "krsi_fs.h"
> > @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
> >   	return false;
> >   }
> >   
> > +static char *array_next_entry(char *array, unsigned long *offset,
> > +			      unsigned long end)
> > +{
> > +	char *entry;
> > +	unsigned long current_offset = *offset;
> > +
> > +	if (current_offset >= end)
> > +		return NULL;
> > +
> > +	/*
> > +	 * iterate on the array till the null byte is encountered
> > +	 * and check for any overflows.
> > +	 */
> > +	entry = array + current_offset;
> > +	while (array[current_offset]) {
> > +		if (unlikely(++current_offset >= end))
> > +			return NULL;
> > +	}
> > +
> > +	/*
> > +	 * Point the offset to the next element in the array.
> > +	 */
> > +	*offset = current_offset + 1;
> > +
> > +	return entry;
> > +}
> > +
> > +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
> > +		u32 n_size, u32 size)
> > +{
> > +	s32 ret = 0;
> > +	u32 num_vars = 0;
> > +	int i, name_len;
> > +	struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
> > +	int argc = bprm->argc;
> > +	int envc = bprm->envc;
> > +	unsigned long end = ctx->bprm_ctx.max_arg_offset;
> > +	unsigned long offset = bprm->p % PAGE_SIZE;
> 
> why we need bprm->p % PAGE_SIZE instead of bprm->p?

bprm->p points to the top of the memory and it's not an offset.

The pinned buffer contains the pages for the (argv+env) and the
brpm->p % PAGE_SIZE is the offset into the first page where the
(argv+envv) starts.

> 
> > +	char *buf = ctx->bprm_ctx.arg_pages;
> > +	char *curr_dest = dest;
> > +	char *entry;
> > +
> > +	if (unlikely(!buf))
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < argc; i++) {
> > +		entry = array_next_entry(buf, &offset, end);
> > +		if (!entry)
> > +			return 0;
> > +	}
> > +
> > +	name_len = strlen(name);
> > +	for (i = 0; i < envc; i++) {
> > +		entry = array_next_entry(buf, &offset, end);
> > +		if (!entry)
> > +			return 0;
> 
> If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
> we may skip the first entry?

I think I need to rename the "array_next_entry" function / document it
better.

The function updates the offset to the next location and the
returns the entry at the current offset.

So, in the first instance:

   // offset is the offset into the first page.
   entry = buf + offset;
   offset = <updated value to the next entry>.

> 
> 
> > +
> > +		if (!strncmp(entry, name, name_len)) {
> > +			num_vars++;
> 
> There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
> in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.

Thanks, you are right, It does not make sense to have name_len = 0. I
will change it to ARG_CONST_SIZE.

> 
> > +
> > +			/*
> > +			 * There is no need to do further copying
> > +			 * if the buffer is already full. Just count how many
> > +			 * times the environment variable is set.
> > +			 */
> > +			if (ret == -E2BIG)
> > +				continue;
> > +
> > +			if (entry[name_len] != '=')
> > +				continue;
> > +
> > +			/*
> > +			 * Move the buf pointer by name_len + 1
> > +			 * (for the "=" sign)
> > +			 */
> > +			entry += name_len + 1;
> > +			ret = strlcpy(curr_dest, entry, size);
> > +
> > +			if (ret >= size) {
> > +				ret = -E2BIG;
> 
> Here, we have a partial copy. Should you instead nullify (memset) it as 
> it is not really invalid one?

The function does specify that the it will return a null terminated
value even if an -E2BIG is returned so that user does get a truncated
value. It's better to give the user some data. (I mentioned this in
the documentation for the helper).

> 
> > +				continue;
> > +			}
> > +
> > +			/*
> > +			 * strlcpy just returns the length of the string copied.
> > +			 * The remaining space needs to account for the added
> > +			 * null character.
> > +			 */
> > +			curr_dest += ret + 1;
> > +			size -= ret + 1;
> > +			/*
> > +			 * Update ret to be the current number of bytes written
> > +			 * to the destination
> > +			 */
> > +			ret = curr_dest - dest;
> > +		}
> > +	}
> > +
> > +	return (u64) num_vars << 32 | (u32) ret;
> > +}
> > +
> > +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> > +	  char *, dest, u32, size)
> > +{
> > +	char *name_end;
> > +
> > +	name_end = memchr(name, '\0', n_size);
> > +	if (!name_end)
> > +		return -EINVAL;
> > +
> > +	memset(dest, 0, size);

This memset ensures the buffer is zeroed out (incase the buffer is
fully / partially empty).

> > +	return get_env_var(ctx, name, dest, n_size, size);
> > +}
> > +
> > +static const struct bpf_func_proto krsi_get_env_var_proto = {
> > +	.func = krsi_get_env_var,
> > +	.gpl_only = true,
> > +	.ret_type = RET_INTEGER,
> > +	.arg1_type = ARG_PTR_TO_CTX,
> > +	.arg2_type = ARG_PTR_TO_MEM,
> > +	.arg3_type = ARG_CONST_SIZE_OR_ZERO,
> > +	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
> > +	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
> > +};
> > +
> >   BPF_CALL_5(krsi_event_output, void *, log,
> >   	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
> >   {
> > @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
> >   		return &bpf_map_lookup_elem_proto;
> >   	case BPF_FUNC_get_current_pid_tgid:
> >   		return &bpf_get_current_pid_tgid_proto;
> > +	case BPF_FUNC_krsi_get_env_var:
> > +		return &krsi_get_env_var_proto;
> >   	case BPF_FUNC_perf_event_output:
> >   		return &krsi_event_output_proto;
> >   	default:
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 32ab38f1a2fe..a4ef07956e07 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -2715,6 +2715,45 @@ union bpf_attr {
> >    *		**-EPERM** if no permission to send the *sig*.
> >    *
> >    *		**-EAGAIN** if bpf program can try again.
> > + *
> > + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> > + *			size_t name_len, size_t buf_len)
> 
> Inconsistent helper definitions.

As discussed above, I can change the BPF_CALL_5 declaration to have
a (void *) and cast to the krsi_ctx in the helper itself.

- KP

> 
> > + *	Description
> > + *		This helper can be used as a part of the
> > + *		process_execution hook of the KRSI LSM in
> > + *		programs of type BPF_PROG_TYPE_KRSI.
> > + *
> > + *		The helper returns the value of the environment
> > + *		variable with the provided "name" for process that's
> > + *		going to be executed in the passed buffer, "buf". If the var
> > + *		is set multiple times, the helper returns all
> > + *		the values as null separated strings.
> > + *
> > + *		If the buffer is too short for these values, the helper
> > + *		tries to fill it the best it can and guarantees that the value
> > + *		returned in the buffer  is always null terminated.
> > + *		After the buffer is filled, the helper keeps counting the number
> > + *		of times the environment variable is set in the envp.
> > + *
> > + *	Return:
> > + *
> > + *		The return value of the helper is an u64 value
> > + *		which carries two pieces of information:
> > + *
> > + *		   The upper 32 bits are a u32 value signifying
> > + *		   the number of times the environment variable
> > + *		   is set in the envp.
> > + *		   The lower 32 bits are an s32 value signifying
> > + *		   the number of bytes written to the buffer or an error code:
> > + *
> > + *		**-ENOMEM** if the kernel is unable to allocate memory
> > + *			    for pinning the argv and envv.
> > + *
> > + *		**-E2BIG** if the value is larger than the size of the
> > + *			   destination buffer. The higher bits will still
> > + *			   the number of times the variable was set in the envp.
> > + *
> > + *		**-EINVAL** if name is not a NULL terminated string.
> >    */
> >   #define __BPF_FUNC_MAPPER(FN)		\
> >   	FN(unspec),			\
> > @@ -2826,7 +2865,8 @@ union bpf_attr {
> >   	FN(strtoul),			\
> >   	FN(sk_storage_get),		\
> >   	FN(sk_storage_delete),		\
> > -	FN(send_signal),
> > +	FN(send_signal),		\
> > +	FN(krsi_get_env_var),
> >   
> >   /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >    * function eBPF program intends to call
> > diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> > index f804f210244e..ecebdb772a9d 100644
> > --- a/tools/testing/selftests/bpf/bpf_helpers.h
> > +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> > @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
> >   static int (*bpf_probe_read_str)(void *ctx, __u32 size,
> >   				 const void *unsafe_ptr) =
> >   	(void *) BPF_FUNC_probe_read_str;
> > +static unsigned long long (*krsi_get_env_var)(void *ctx,
> > +	void *name, __u32 n_size, void *buf, __u32 size) =
> > +	(void *) BPF_FUNC_krsi_get_env_var;
> >   static unsigned int (*bpf_get_socket_uid)(void *ctx) =
> >   	(void *) BPF_FUNC_get_socket_uid;
> >   static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> > 

Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable

[Yonghong Song]

On 9/16/19 6:00 AM, KP Singh wrote:
> Thanks for reviewing!
> 
> On 15-Sep 00:16, Yonghong Song wrote:
>>
>>
>> On 9/10/19 12:55 PM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>>
>> This patch cannot apply cleanly.
>>
>> -bash-4.4$ git apply ~/p12.txt
>> error: patch failed: include/uapi/linux/bpf.h:2715
>> error: include/uapi/linux/bpf.h: patch does not apply
>> error: patch failed: tools/include/uapi/linux/bpf.h:2715
>> error: tools/include/uapi/linux/bpf.h: patch does not apply
>> -bash-4.4$
> 
> I am not sure why this is happening, I tried:
> 
> git clone \
>    https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git && \
>    cd linux-next && \
>    git checkout -b review v5.3-rc6 && \
>    wget -P /tmp https://lore.kernel.org/patchwork/series/410101/mbox && \
>    git am /tmp/mbox
> 
> and it worked.
> 
> This seems to work too:
> 
>    patch -p1 < <file>.patch
> 
> Can you try with "git am" please?

Will try next time when reviewing the tree.

> 
>>
>>>
>>> The helper returns the value of the environment variable in the buffer
>>> that is passed to it. If the var is set multiple times, the helper
>>> returns all the values as null separated strings.
>>>
>>> If the buffer is too short for these values, the helper tries to fill it
>>> the best it can and guarantees that the value returned in the buffer
>>> is always null terminated. After the buffer is filled, the helper keeps
>>> counting the number of times the environment variable is set in the
>>> envp.
>>>
>>> The return value of the helper is an u64 value which carries two pieces
>>> of information.
>>>
>>>     * The upper 32 bits are a u32 value signifying the number of times
>>>       the environment variable is set in the envp.
>>
>> Not sure how useful this 'upper 32' bit value is. What user expected to do?
>>
>> Another option is to have upper 32 bits encode the required buffer size
>> to hold all values. This may cause some kind of user space action, e.g.,
>> to replace the program with new program with larger per cpu map value size?
>>
> 
> The upper 32-bit value is actually an important part of the LSM's MAC
> policy. It allows the user to:
> 
> - Return an -EPERM when if the environment variable is set more than
>    once.
> - Log a warning (this is what we are doing in the example) so
>    this is flagged as a potential malicious actor.

So the intention is to catch cases where the env variable
to set by more than once, not exactly to capture all the values
of the env variable.

Then may be there is no need to record the values once the number of 
values is more than one? Do you have use case for what user to react if
there are more than one setting?

> 
>>>     * The lower 32 bits are a s32 value signifying the number of bytes
>>>       written to the buffer or an error code. >
>>> Since the value of the environment variable can be very long and exceed
>>> what can be allocated on the BPF stack, a per-cpu array can be used
>>> instead:
>>>
>>> struct bpf_map_def SEC("maps") env_map = {
>>>           .type = BPF_MAP_TYPE_PERCPU_ARRAY,
>>>           .key_size = sizeof(u32),
>>>           .value_size = 4096,
>>>           .max_entries = 1,
>>> };
>>
>> Could you use use map definition with SEC(".maps")?
> 
> Sure, I added this example program in the commit message. Will update
> it to be more canonical. Thanks!
> 
>>
>>>
>>> SEC("prgrm")
>>> int bpf_prog1(void *ctx)
>>> {
>>>           u32 map_id = 0;
>>>           u64 times_ret;
>>>           s32 ret;
>>>           char name[48] = "LD_PRELOAD";
>>
>> Reverse Christmas tree coding style, here and other places?
> 
> Will happily fix it.
> 
> However, I did not find it mentioned in the style guide:
> 
>    https://www.kernel.org/doc/html/v4.10/process/coding-style.html
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v4.6_source_Documentation_CodingStyle&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=G_3dpLivr0lLqQlQqAVEw9EZB3GonzJjILyCBqbmMIo&s=krLmTogyg9eSdco0j4tv2etr4PmNEPZGXMMoOkWdVG4&e=
> 
> Is there one specific to BPF?

I forgot where is the documentation. We follow this for bpf/net.

> 
> 
>>
>>>
>>>           char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
>>>           if (!map_value)
>>>                   return 0;
>>>
>>>           // Read the lower 32 bits for the return value
>>>           times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
>>>           ret = times_ret & 0xffffffff;
>>>           if (ret < 0)
>>>                   return ret;
>>>           return 0;
>>> }
>>>
>>> Signed-off-by: KP Singh <kpsingh@google.com>
>>> ---
>>>    include/uapi/linux/bpf.h                  |  42 ++++++-
>>>    security/krsi/ops.c                       | 129 ++++++++++++++++++++++
>>>    tools/include/uapi/linux/bpf.h            |  42 ++++++-
>>>    tools/testing/selftests/bpf/bpf_helpers.h |   3 +
>>>    4 files changed, 214 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 32ab38f1a2fe..a4ef07956e07 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2715,6 +2715,45 @@ union bpf_attr {
>>>     *		**-EPERM** if no permission to send the *sig*.
>>>     *
>>>     *		**-EAGAIN** if bpf program can try again.
>>> + *
>>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
>>> + *			size_t name_len, size_t buf_len)
>>
>> This signature is not the same as the later
>> krsi_get_env_var(...) helper definition.
>> BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32,
>> n_size,
>> 	  char *, dest, u32, size)
>>
> 
> I did this because the krsi_ctx is not exposed to the userspace and
> allows KRSI to modify the context without worrying about breaking
> userspace.
> 
> That said, I could mark it as a (void *) here and cast it internally.
> I guess that would be better/cleaner?

"void *" is okay, I am complaining the argument ordering is different
from real definition.

> 
>>> + *	Description
>>> + *		This helper can be used as a part of the
>>> + *		process_execution hook of the KRSI LSM in
>>> + *		programs of type BPF_PROG_TYPE_KRSI.
>>> + *
>>> + *		The helper returns the value of the environment
>>> + *		variable with the provided "name" for process that's
>>> + *		going to be executed in the passed buffer, "buf". If the var
>>> + *		is set multiple times, the helper returns all
>>> + *		the values as null separated strings.
>>> + *
>>> + *		If the buffer is too short for these values, the helper
>>> + *		tries to fill it the best it can and guarantees that the value
>>> + *		returned in the buffer  is always null terminated.
>>> + *		After the buffer is filled, the helper keeps counting the number
>>> + *		of times the environment variable is set in the envp.
>>> + *
>>> + *	Return:
>>> + *
>>> + *		The return value of the helper is an u64 value
>>> + *		which carries two pieces of information:
>>> + *
>>> + *		   The upper 32 bits are a u32 value signifying
>>> + *		   the number of times the environment variable
>>> + *		   is set in the envp.
>>> + *		   The lower 32 bits are an s32 value signifying
>>> + *		   the number of bytes written to the buffer or an error code:
>>> + *
>>> + *		**-ENOMEM** if the kernel is unable to allocate memory
>>> + *			    for pinning the argv and envv.
>>> + *
>>> + *		**-E2BIG** if the value is larger than the size of the
>>> + *			   destination buffer. The higher bits will still
>>> + *			   the number of times the variable was set in the envp.
>>
>> The -E2BIG is returned because buffer sizee is not big enough.
>> Another possible error code is -ENOSPC, which typically indicates
>> buffer size not big enough.
> 
> Sure, I am fine with using either.
> 
>>
>>> + *
>>> + *		**-EINVAL** if name is not a NULL terminated string.
>>>     */
>>>    #define __BPF_FUNC_MAPPER(FN)		\
>>>    	FN(unspec),			\
>>> @@ -2826,7 +2865,8 @@ union bpf_attr {
>>>    	FN(strtoul),			\
>>>    	FN(sk_storage_get),		\
>>>    	FN(sk_storage_delete),		\
>>> -	FN(send_signal),
>>> +	FN(send_signal),		\
>>> +	FN(krsi_get_env_var),
>>>    
>>>    /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>>     * function eBPF program intends to call
>>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
>>> index 1f4df920139c..1db94dfaac15 100644
>>> --- a/security/krsi/ops.c
>>> +++ b/security/krsi/ops.c
>>> @@ -6,6 +6,8 @@
>>>    #include <linux/bpf.h>
>>>    #include <linux/security.h>
>>>    #include <linux/krsi.h>
>>> +#include <linux/binfmts.h>
>>> +#include <linux/highmem.h>
>>>    
>>>    #include "krsi_init.h"
>>>    #include "krsi_fs.h"
>>> @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
>>>    	return false;
>>>    }
>>>    
>>> +static char *array_next_entry(char *array, unsigned long *offset,
>>> +			      unsigned long end)
>>> +{
>>> +	char *entry;
>>> +	unsigned long current_offset = *offset;
>>> +
>>> +	if (current_offset >= end)
>>> +		return NULL;
>>> +
>>> +	/*
>>> +	 * iterate on the array till the null byte is encountered
>>> +	 * and check for any overflows.
>>> +	 */
>>> +	entry = array + current_offset;
>>> +	while (array[current_offset]) {
>>> +		if (unlikely(++current_offset >= end))
>>> +			return NULL;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Point the offset to the next element in the array.
>>> +	 */
>>> +	*offset = current_offset + 1;
>>> +
>>> +	return entry;
>>> +}
>>> +
>>> +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
>>> +		u32 n_size, u32 size)
>>> +{
>>> +	s32 ret = 0;
>>> +	u32 num_vars = 0;
>>> +	int i, name_len;
>>> +	struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
>>> +	int argc = bprm->argc;
>>> +	int envc = bprm->envc;
>>> +	unsigned long end = ctx->bprm_ctx.max_arg_offset;
>>> +	unsigned long offset = bprm->p % PAGE_SIZE;
>>
>> why we need bprm->p % PAGE_SIZE instead of bprm->p?
> 
> bprm->p points to the top of the memory and it's not an offset.
> 
> The pinned buffer contains the pages for the (argv+env) and the
> brpm->p % PAGE_SIZE is the offset into the first page where the
> (argv+envv) starts.

Thanks for the explanation.

> 
>>
>>> +	char *buf = ctx->bprm_ctx.arg_pages;
>>> +	char *curr_dest = dest;
>>> +	char *entry;
>>> +
>>> +	if (unlikely(!buf))
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < argc; i++) {
>>> +		entry = array_next_entry(buf, &offset, end);
>>> +		if (!entry)
>>> +			return 0;
>>> +	}
>>> +
>>> +	name_len = strlen(name);
>>> +	for (i = 0; i < envc; i++) {
>>> +		entry = array_next_entry(buf, &offset, end);
>>> +		if (!entry)
>>> +			return 0;
>>
>> If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
>> we may skip the first entry?
> 
> I think I need to rename the "array_next_entry" function / document it
> better.
> 
> The function updates the offset to the next location and the
> returns the entry at the current offset.
> 
> So, in the first instance:
> 
>     // offset is the offset into the first page.
>     entry = buf + offset;
>     offset = <updated value to the next entry>.

Yes, some documentation will help.

> 
>>
>>
>>> +
>>> +		if (!strncmp(entry, name, name_len)) {
>>> +			num_vars++;
>>
>> There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
>> in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.
> 
> Thanks, you are right, It does not make sense to have name_len = 0. I
> will change it to ARG_CONST_SIZE.
> 
>>
>>> +
>>> +			/*
>>> +			 * There is no need to do further copying
>>> +			 * if the buffer is already full. Just count how many
>>> +			 * times the environment variable is set.
>>> +			 */
>>> +			if (ret == -E2BIG)
>>> +				continue;
>>> +
>>> +			if (entry[name_len] != '=')
>>> +				continue;
>>> +
>>> +			/*
>>> +			 * Move the buf pointer by name_len + 1
>>> +			 * (for the "=" sign)
>>> +			 */
>>> +			entry += name_len + 1;
>>> +			ret = strlcpy(curr_dest, entry, size);
>>> +
>>> +			if (ret >= size) {
>>> +				ret = -E2BIG;
>>
>> Here, we have a partial copy. Should you instead nullify (memset) it as
>> it is not really invalid one?
> 
> The function does specify that the it will return a null terminated
> value even if an -E2BIG is returned so that user does get a truncated
> value. It's better to give the user some data. (I mentioned this in
> the documentation for the helper).

Do you know what user could do with this partial data?
Again, I am not a security expert, maybe partial data can still
be used in meaningful.

> 
>>
>>> +				continue;
>>> +			}
>>> +
>>> +			/*
>>> +			 * strlcpy just returns the length of the string copied.
>>> +			 * The remaining space needs to account for the added
>>> +			 * null character.
>>> +			 */
>>> +			curr_dest += ret + 1;
>>> +			size -= ret + 1;
>>> +			/*
>>> +			 * Update ret to be the current number of bytes written
>>> +			 * to the destination
>>> +			 */
>>> +			ret = curr_dest - dest;
>>> +		}
>>> +	}
>>> +
>>> +	return (u64) num_vars << 32 | (u32) ret;
>>> +}
>>> +
>>> +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
>>> +	  char *, dest, u32, size)
>>> +{
>>> +	char *name_end;
>>> +
>>> +	name_end = memchr(name, '\0', n_size);
>>> +	if (!name_end)
>>> +		return -EINVAL;
>>> +
>>> +	memset(dest, 0, size);
> 
> This memset ensures the buffer is zeroed out (incase the buffer is
> fully / partially empty).
> 
>>> +	return get_env_var(ctx, name, dest, n_size, size);
>>> +}
>>> +
>>> +static const struct bpf_func_proto krsi_get_env_var_proto = {
>>> +	.func = krsi_get_env_var,
>>> +	.gpl_only = true,
>>> +	.ret_type = RET_INTEGER,
>>> +	.arg1_type = ARG_PTR_TO_CTX,
>>> +	.arg2_type = ARG_PTR_TO_MEM,
>>> +	.arg3_type = ARG_CONST_SIZE_OR_ZERO,
>>> +	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
>>> +	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
>>> +};
>>> +
>>>    BPF_CALL_5(krsi_event_output, void *, log,
>>>    	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
>>>    {
>>> @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
>>>    		return &bpf_map_lookup_elem_proto;
>>>    	case BPF_FUNC_get_current_pid_tgid:
>>>    		return &bpf_get_current_pid_tgid_proto;
>>> +	case BPF_FUNC_krsi_get_env_var:
>>> +		return &krsi_get_env_var_proto;
>>>    	case BPF_FUNC_perf_event_output:
>>>    		return &krsi_event_output_proto;
>>>    	default:
>>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>>> index 32ab38f1a2fe..a4ef07956e07 100644
>>> --- a/tools/include/uapi/linux/bpf.h
>>> +++ b/tools/include/uapi/linux/bpf.h
>>> @@ -2715,6 +2715,45 @@ union bpf_attr {
>>>     *		**-EPERM** if no permission to send the *sig*.
>>>     *
>>>     *		**-EAGAIN** if bpf program can try again.
>>> + *
>>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
>>> + *			size_t name_len, size_t buf_len)
>>
>> Inconsistent helper definitions.
> 
> As discussed above, I can change the BPF_CALL_5 declaration to have
> a (void *) and cast to the krsi_ctx in the helper itself.
> 
> - KP
> 
>>
>>> + *	Description
>>> + *		This helper can be used as a part of the
>>> + *		process_execution hook of the KRSI LSM in
>>> + *		programs of type BPF_PROG_TYPE_KRSI.
>>> + *
>>> + *		The helper returns the value of the environment
>>> + *		variable with the provided "name" for process that's
>>> + *		going to be executed in the passed buffer, "buf". If the var
>>> + *		is set multiple times, the helper returns all
>>> + *		the values as null separated strings.
>>> + *
>>> + *		If the buffer is too short for these values, the helper
>>> + *		tries to fill it the best it can and guarantees that the value
>>> + *		returned in the buffer  is always null terminated.
>>> + *		After the buffer is filled, the helper keeps counting the number
>>> + *		of times the environment variable is set in the envp.
>>> + *
>>> + *	Return:
>>> + *
>>> + *		The return value of the helper is an u64 value
>>> + *		which carries two pieces of information:
>>> + *
>>> + *		   The upper 32 bits are a u32 value signifying
>>> + *		   the number of times the environment variable
>>> + *		   is set in the envp.
>>> + *		   The lower 32 bits are an s32 value signifying
>>> + *		   the number of bytes written to the buffer or an error code:
>>> + *
>>> + *		**-ENOMEM** if the kernel is unable to allocate memory
>>> + *			    for pinning the argv and envv.
>>> + *
>>> + *		**-E2BIG** if the value is larger than the size of the
>>> + *			   destination buffer. The higher bits will still
>>> + *			   the number of times the variable was set in the envp.
>>> + *
>>> + *		**-EINVAL** if name is not a NULL terminated string.
>>>     */
>>>    #define __BPF_FUNC_MAPPER(FN)		\
>>>    	FN(unspec),			\
>>> @@ -2826,7 +2865,8 @@ union bpf_attr {
>>>    	FN(strtoul),			\
>>>    	FN(sk_storage_get),		\
>>>    	FN(sk_storage_delete),		\
>>> -	FN(send_signal),
>>> +	FN(send_signal),		\
>>> +	FN(krsi_get_env_var),
>>>    
>>>    /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>>     * function eBPF program intends to call
>>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
>>> index f804f210244e..ecebdb772a9d 100644
>>> --- a/tools/testing/selftests/bpf/bpf_helpers.h
>>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
>>> @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
>>>    static int (*bpf_probe_read_str)(void *ctx, __u32 size,
>>>    				 const void *unsafe_ptr) =
>>>    	(void *) BPF_FUNC_probe_read_str;
>>> +static unsigned long long (*krsi_get_env_var)(void *ctx,
>>> +	void *name, __u32 n_size, void *buf, __u32 size) =
>>> +	(void *) BPF_FUNC_krsi_get_env_var;
>>>    static unsigned int (*bpf_get_socket_uid)(void *ctx) =
>>>    	(void *) BPF_FUNC_get_socket_uid;
>>>    static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
>>>

Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable

[KP Singh]

On 17-Sep 16:58, Yonghong Song wrote:
> 
> 
> On 9/16/19 6:00 AM, KP Singh wrote:
> > Thanks for reviewing!
> > 
> > On 15-Sep 00:16, Yonghong Song wrote:
> >>
> >>
> >> On 9/10/19 12:55 PM, KP Singh wrote:
> >>> From: KP Singh <kpsingh@google.com>
> >>
> >> This patch cannot apply cleanly.
> >>
> >> -bash-4.4$ git apply ~/p12.txt
> >> error: patch failed: include/uapi/linux/bpf.h:2715
> >> error: include/uapi/linux/bpf.h: patch does not apply
> >> error: patch failed: tools/include/uapi/linux/bpf.h:2715
> >> error: tools/include/uapi/linux/bpf.h: patch does not apply
> >> -bash-4.4$
> > 
> > I am not sure why this is happening, I tried:
> > 
> > git clone \
> >    https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git && \
> >    cd linux-next && \
> >    git checkout -b review v5.3-rc6 && \
> >    wget -P /tmp https://lore.kernel.org/patchwork/series/410101/mbox && \
> >    git am /tmp/mbox
> > 
> > and it worked.
> > 
> > This seems to work too:
> > 
> >    patch -p1 < <file>.patch
> > 
> > Can you try with "git am" please?
> 
> Will try next time when reviewing the tree.

Thanks!

> 
> > 
> >>
> >>>
> >>> The helper returns the value of the environment variable in the buffer
> >>> that is passed to it. If the var is set multiple times, the helper
> >>> returns all the values as null separated strings.
> >>>
> >>> If the buffer is too short for these values, the helper tries to fill it
> >>> the best it can and guarantees that the value returned in the buffer
> >>> is always null terminated. After the buffer is filled, the helper keeps
> >>> counting the number of times the environment variable is set in the
> >>> envp.
> >>>
> >>> The return value of the helper is an u64 value which carries two pieces
> >>> of information.
> >>>
> >>>     * The upper 32 bits are a u32 value signifying the number of times
> >>>       the environment variable is set in the envp.
> >>
> >> Not sure how useful this 'upper 32' bit value is. What user expected to do?
> >>
> >> Another option is to have upper 32 bits encode the required buffer size
> >> to hold all values. This may cause some kind of user space action, e.g.,
> >> to replace the program with new program with larger per cpu map value size?
> >>
> > 
> > The upper 32-bit value is actually an important part of the LSM's MAC
> > policy. It allows the user to:
> > 
> > - Return an -EPERM when if the environment variable is set more than
> >    once.
> > - Log a warning (this is what we are doing in the example) so
> >    this is flagged as a potential malicious actor.
> 
> So the intention is to catch cases where the env variable
> to set by more than once, not exactly to capture all the values
> of the env variable.
> 
> Then may be there is no need to record the values once the number of 
> values is more than one? Do you have use case for what user to react if
> there are more than one setting?

Since KRSI intends to be an API for creating MAC and Audit Policies,
it would be best to leave the decision to the user, as to what they 
intend to do with the extra information. In general, having both the 
values helps in building more context about the potential malicious actor.

What should ideally be done is returning an -EPERM error, because there
should be no reason to set an env variable twice. 
In fact, two+ env vars can only be passed directly to the 
execve system call arguments. Shells (e.g. bash) remove the duplicate 
value before passing the envv to the system call.

> 
> > 
> >>>     * The lower 32 bits are a s32 value signifying the number of bytes
> >>>       written to the buffer or an error code. >
> >>> Since the value of the environment variable can be very long and exceed
> >>> what can be allocated on the BPF stack, a per-cpu array can be used
> >>> instead:
> >>>
> >>> struct bpf_map_def SEC("maps") env_map = {
> >>>           .type = BPF_MAP_TYPE_PERCPU_ARRAY,
> >>>           .key_size = sizeof(u32),
> >>>           .value_size = 4096,
> >>>           .max_entries = 1,
> >>> };
> >>
> >> Could you use use map definition with SEC(".maps")?
> > 
> > Sure, I added this example program in the commit message. Will update
> > it to be more canonical. Thanks!
> > 
> >>
> >>>
> >>> SEC("prgrm")
> >>> int bpf_prog1(void *ctx)
> >>> {
> >>>           u32 map_id = 0;
> >>>           u64 times_ret;
> >>>           s32 ret;
> >>>           char name[48] = "LD_PRELOAD";
> >>
> >> Reverse Christmas tree coding style, here and other places?
> > 
> > Will happily fix it.
> > 
> > However, I did not find it mentioned in the style guide:
> > 
> >    https://www.kernel.org/doc/html/v4.10/process/coding-style.html
> >    https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v4.6_source_Documentation_CodingStyle&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=G_3dpLivr0lLqQlQqAVEw9EZB3GonzJjILyCBqbmMIo&s=krLmTogyg9eSdco0j4tv2etr4PmNEPZGXMMoOkWdVG4&e=
> > 
> > Is there one specific to BPF?
> 
> I forgot where is the documentation. We follow this for bpf/net.

Thanks, I searched and found some threads where this is recommended
for bpf/net. I will update it in the next iteration.

> 
> > 
> > 
> >>
> >>>
> >>>           char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
> >>>           if (!map_value)
> >>>                   return 0;
> >>>
> >>>           // Read the lower 32 bits for the return value
> >>>           times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
> >>>           ret = times_ret & 0xffffffff;
> >>>           if (ret < 0)
> >>>                   return ret;
> >>>           return 0;
> >>> }
> >>>
> >>> Signed-off-by: KP Singh <kpsingh@google.com>
> >>> ---
> >>>    include/uapi/linux/bpf.h                  |  42 ++++++-
> >>>    security/krsi/ops.c                       | 129 ++++++++++++++++++++++
> >>>    tools/include/uapi/linux/bpf.h            |  42 ++++++-
> >>>    tools/testing/selftests/bpf/bpf_helpers.h |   3 +
> >>>    4 files changed, 214 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 32ab38f1a2fe..a4ef07956e07 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -2715,6 +2715,45 @@ union bpf_attr {
> >>>     *		**-EPERM** if no permission to send the *sig*.
> >>>     *
> >>>     *		**-EAGAIN** if bpf program can try again.
> >>> + *
> >>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> >>> + *			size_t name_len, size_t buf_len)
> >>
> >> This signature is not the same as the later
> >> krsi_get_env_var(...) helper definition.
> >> BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32,
> >> n_size,
> >> 	  char *, dest, u32, size)
> >>
> > 
> > I did this because the krsi_ctx is not exposed to the userspace and
> > allows KRSI to modify the context without worrying about breaking
> > userspace.
> > 
> > That said, I could mark it as a (void *) here and cast it internally.
> > I guess that would be better/cleaner?
> 
> "void *" is okay, I am complaining the argument ordering is different
> from real definition.

Ah, nice catch! Apologies for overlooking.

> 
> > 
> >>> + *	Description
> >>> + *		This helper can be used as a part of the
> >>> + *		process_execution hook of the KRSI LSM in
> >>> + *		programs of type BPF_PROG_TYPE_KRSI.
> >>> + *
> >>> + *		The helper returns the value of the environment
> >>> + *		variable with the provided "name" for process that's
> >>> + *		going to be executed in the passed buffer, "buf". If the var
> >>> + *		is set multiple times, the helper returns all
> >>> + *		the values as null separated strings.
> >>> + *
> >>> + *		If the buffer is too short for these values, the helper
> >>> + *		tries to fill it the best it can and guarantees that the value
> >>> + *		returned in the buffer  is always null terminated.
> >>> + *		After the buffer is filled, the helper keeps counting the number
> >>> + *		of times the environment variable is set in the envp.
> >>> + *
> >>> + *	Return:
> >>> + *
> >>> + *		The return value of the helper is an u64 value
> >>> + *		which carries two pieces of information:
> >>> + *
> >>> + *		   The upper 32 bits are a u32 value signifying
> >>> + *		   the number of times the environment variable
> >>> + *		   is set in the envp.
> >>> + *		   The lower 32 bits are an s32 value signifying
> >>> + *		   the number of bytes written to the buffer or an error code:
> >>> + *
> >>> + *		**-ENOMEM** if the kernel is unable to allocate memory
> >>> + *			    for pinning the argv and envv.
> >>> + *
> >>> + *		**-E2BIG** if the value is larger than the size of the
> >>> + *			   destination buffer. The higher bits will still
> >>> + *			   the number of times the variable was set in the envp.
> >>
> >> The -E2BIG is returned because buffer sizee is not big enough.
> >> Another possible error code is -ENOSPC, which typically indicates
> >> buffer size not big enough.
> > 
> > Sure, I am fine with using either.
> > 
> >>
> >>> + *
> >>> + *		**-EINVAL** if name is not a NULL terminated string.
> >>>     */
> >>>    #define __BPF_FUNC_MAPPER(FN)		\
> >>>    	FN(unspec),			\
> >>> @@ -2826,7 +2865,8 @@ union bpf_attr {
> >>>    	FN(strtoul),			\
> >>>    	FN(sk_storage_get),		\
> >>>    	FN(sk_storage_delete),		\
> >>> -	FN(send_signal),
> >>> +	FN(send_signal),		\
> >>> +	FN(krsi_get_env_var),
> >>>    
> >>>    /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >>>     * function eBPF program intends to call
> >>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> >>> index 1f4df920139c..1db94dfaac15 100644
> >>> --- a/security/krsi/ops.c
> >>> +++ b/security/krsi/ops.c
> >>> @@ -6,6 +6,8 @@
> >>>    #include <linux/bpf.h>
> >>>    #include <linux/security.h>
> >>>    #include <linux/krsi.h>
> >>> +#include <linux/binfmts.h>
> >>> +#include <linux/highmem.h>
> >>>    
> >>>    #include "krsi_init.h"
> >>>    #include "krsi_fs.h"
> >>> @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
> >>>    	return false;
> >>>    }
> >>>    
> >>> +static char *array_next_entry(char *array, unsigned long *offset,
> >>> +			      unsigned long end)
> >>> +{
> >>> +	char *entry;
> >>> +	unsigned long current_offset = *offset;
> >>> +
> >>> +	if (current_offset >= end)
> >>> +		return NULL;
> >>> +
> >>> +	/*
> >>> +	 * iterate on the array till the null byte is encountered
> >>> +	 * and check for any overflows.
> >>> +	 */
> >>> +	entry = array + current_offset;
> >>> +	while (array[current_offset]) {
> >>> +		if (unlikely(++current_offset >= end))
> >>> +			return NULL;
> >>> +	}
> >>> +
> >>> +	/*
> >>> +	 * Point the offset to the next element in the array.
> >>> +	 */
> >>> +	*offset = current_offset + 1;
> >>> +
> >>> +	return entry;
> >>> +}
> >>> +
> >>> +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
> >>> +		u32 n_size, u32 size)
> >>> +{
> >>> +	s32 ret = 0;
> >>> +	u32 num_vars = 0;
> >>> +	int i, name_len;
> >>> +	struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
> >>> +	int argc = bprm->argc;
> >>> +	int envc = bprm->envc;
> >>> +	unsigned long end = ctx->bprm_ctx.max_arg_offset;
> >>> +	unsigned long offset = bprm->p % PAGE_SIZE;
> >>
> >> why we need bprm->p % PAGE_SIZE instead of bprm->p?
> > 
> > bprm->p points to the top of the memory and it's not an offset.
> > 
> > The pinned buffer contains the pages for the (argv+env) and the
> > brpm->p % PAGE_SIZE is the offset into the first page where the
> > (argv+envv) starts.
> 
> Thanks for the explanation.
> 
> > 
> >>
> >>> +	char *buf = ctx->bprm_ctx.arg_pages;
> >>> +	char *curr_dest = dest;
> >>> +	char *entry;
> >>> +
> >>> +	if (unlikely(!buf))
> >>> +		return -ENOMEM;
> >>> +
> >>> +	for (i = 0; i < argc; i++) {
> >>> +		entry = array_next_entry(buf, &offset, end);
> >>> +		if (!entry)
> >>> +			return 0;
> >>> +	}
> >>> +
> >>> +	name_len = strlen(name);
> >>> +	for (i = 0; i < envc; i++) {
> >>> +		entry = array_next_entry(buf, &offset, end);
> >>> +		if (!entry)
> >>> +			return 0;
> >>
> >> If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
> >> we may skip the first entry?
> > 
> > I think I need to rename the "array_next_entry" function / document it
> > better.
> > 
> > The function updates the offset to the next location and the
> > returns the entry at the current offset.
> > 
> > So, in the first instance:
> > 
> >     // offset is the offset into the first page.
> >     entry = buf + offset;
> >     offset = <updated value to the next entry>.
> 
> Yes, some documentation will help.
> 
> > 
> >>
> >>
> >>> +
> >>> +		if (!strncmp(entry, name, name_len)) {
> >>> +			num_vars++;
> >>
> >> There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
> >> in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.
> > 
> > Thanks, you are right, It does not make sense to have name_len = 0. I
> > will change it to ARG_CONST_SIZE.
> > 
> >>
> >>> +
> >>> +			/*
> >>> +			 * There is no need to do further copying
> >>> +			 * if the buffer is already full. Just count how many
> >>> +			 * times the environment variable is set.
> >>> +			 */
> >>> +			if (ret == -E2BIG)
> >>> +				continue;
> >>> +
> >>> +			if (entry[name_len] != '=')
> >>> +				continue;
> >>> +
> >>> +			/*
> >>> +			 * Move the buf pointer by name_len + 1
> >>> +			 * (for the "=" sign)
> >>> +			 */
> >>> +			entry += name_len + 1;
> >>> +			ret = strlcpy(curr_dest, entry, size);
> >>> +
> >>> +			if (ret >= size) {
> >>> +				ret = -E2BIG;
> >>
> >> Here, we have a partial copy. Should you instead nullify (memset) it as
> >> it is not really invalid one?
> > 
> > The function does specify that the it will return a null terminated
> > value even if an -E2BIG is returned so that user does get a truncated
> > value. It's better to give the user some data. (I mentioned this in
> > the documentation for the helper).
> 
> Do you know what user could do with this partial data?
> Again, I am not a security expert, maybe partial data can still
> be used in meaningful.

It's best to give the userspace as much data as possible and indicate an
overflow so that user-space can use this information to create more
context around the possible malicious activity.

- KP

> 
> > 
> >>
> >>> +				continue;
> >>> +			}
> >>> +
> >>> +			/*
> >>> +			 * strlcpy just returns the length of the string copied.
> >>> +			 * The remaining space needs to account for the added
> >>> +			 * null character.
> >>> +			 */
> >>> +			curr_dest += ret + 1;
> >>> +			size -= ret + 1;
> >>> +			/*
> >>> +			 * Update ret to be the current number of bytes written
> >>> +			 * to the destination
> >>> +			 */
> >>> +			ret = curr_dest - dest;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return (u64) num_vars << 32 | (u32) ret;
> >>> +}
> >>> +
> >>> +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> >>> +	  char *, dest, u32, size)
> >>> +{
> >>> +	char *name_end;
> >>> +
> >>> +	name_end = memchr(name, '\0', n_size);
> >>> +	if (!name_end)
> >>> +		return -EINVAL;
> >>> +
> >>> +	memset(dest, 0, size);
> > 
> > This memset ensures the buffer is zeroed out (incase the buffer is
> > fully / partially empty).
> > 
> >>> +	return get_env_var(ctx, name, dest, n_size, size);
> >>> +}
> >>> +
> >>> +static const struct bpf_func_proto krsi_get_env_var_proto = {
> >>> +	.func = krsi_get_env_var,
> >>> +	.gpl_only = true,
> >>> +	.ret_type = RET_INTEGER,
> >>> +	.arg1_type = ARG_PTR_TO_CTX,
> >>> +	.arg2_type = ARG_PTR_TO_MEM,
> >>> +	.arg3_type = ARG_CONST_SIZE_OR_ZERO,
> >>> +	.arg4_type = ARG_PTR_TO_UNINIT_MEM,
> >>> +	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
> >>> +};
> >>> +
> >>>    BPF_CALL_5(krsi_event_output, void *, log,
> >>>    	   struct bpf_map *, map, u64, flags, void *, data, u64, size)
> >>>    {
> >>> @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
> >>>    		return &bpf_map_lookup_elem_proto;
> >>>    	case BPF_FUNC_get_current_pid_tgid:
> >>>    		return &bpf_get_current_pid_tgid_proto;
> >>> +	case BPF_FUNC_krsi_get_env_var:
> >>> +		return &krsi_get_env_var_proto;
> >>>    	case BPF_FUNC_perf_event_output:
> >>>    		return &krsi_event_output_proto;
> >>>    	default:
> >>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> >>> index 32ab38f1a2fe..a4ef07956e07 100644
> >>> --- a/tools/include/uapi/linux/bpf.h
> >>> +++ b/tools/include/uapi/linux/bpf.h
> >>> @@ -2715,6 +2715,45 @@ union bpf_attr {
> >>>     *		**-EPERM** if no permission to send the *sig*.
> >>>     *
> >>>     *		**-EAGAIN** if bpf program can try again.
> >>> + *
> >>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> >>> + *			size_t name_len, size_t buf_len)
> >>
> >> Inconsistent helper definitions.
> > 
> > As discussed above, I can change the BPF_CALL_5 declaration to have
> > a (void *) and cast to the krsi_ctx in the helper itself.
> > 
> > - KP
> > 
> >>
> >>> + *	Description
> >>> + *		This helper can be used as a part of the
> >>> + *		process_execution hook of the KRSI LSM in
> >>> + *		programs of type BPF_PROG_TYPE_KRSI.
> >>> + *
> >>> + *		The helper returns the value of the environment
> >>> + *		variable with the provided "name" for process that's
> >>> + *		going to be executed in the passed buffer, "buf". If the var
> >>> + *		is set multiple times, the helper returns all
> >>> + *		the values as null separated strings.
> >>> + *
> >>> + *		If the buffer is too short for these values, the helper
> >>> + *		tries to fill it the best it can and guarantees that the value
> >>> + *		returned in the buffer  is always null terminated.
> >>> + *		After the buffer is filled, the helper keeps counting the number
> >>> + *		of times the environment variable is set in the envp.
> >>> + *
> >>> + *	Return:
> >>> + *
> >>> + *		The return value of the helper is an u64 value
> >>> + *		which carries two pieces of information:
> >>> + *
> >>> + *		   The upper 32 bits are a u32 value signifying
> >>> + *		   the number of times the environment variable
> >>> + *		   is set in the envp.
> >>> + *		   The lower 32 bits are an s32 value signifying
> >>> + *		   the number of bytes written to the buffer or an error code:
> >>> + *
> >>> + *		**-ENOMEM** if the kernel is unable to allocate memory
> >>> + *			    for pinning the argv and envv.
> >>> + *
> >>> + *		**-E2BIG** if the value is larger than the size of the
> >>> + *			   destination buffer. The higher bits will still
> >>> + *			   the number of times the variable was set in the envp.
> >>> + *
> >>> + *		**-EINVAL** if name is not a NULL terminated string.
> >>>     */
> >>>    #define __BPF_FUNC_MAPPER(FN)		\
> >>>    	FN(unspec),			\
> >>> @@ -2826,7 +2865,8 @@ union bpf_attr {
> >>>    	FN(strtoul),			\
> >>>    	FN(sk_storage_get),		\
> >>>    	FN(sk_storage_delete),		\
> >>> -	FN(send_signal),
> >>> +	FN(send_signal),		\
> >>> +	FN(krsi_get_env_var),
> >>>    
> >>>    /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >>>     * function eBPF program intends to call
> >>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> >>> index f804f210244e..ecebdb772a9d 100644
> >>> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> >>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> >>> @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
> >>>    static int (*bpf_probe_read_str)(void *ctx, __u32 size,
> >>>    				 const void *unsafe_ptr) =
> >>>    	(void *) BPF_FUNC_probe_read_str;
> >>> +static unsigned long long (*krsi_get_env_var)(void *ctx,
> >>> +	void *name, __u32 n_size, void *buf, __u32 size) =
> >>> +	(void *) BPF_FUNC_krsi_get_env_var;
> >>>    static unsigned int (*bpf_get_socket_uid)(void *ctx) =
> >>>    	(void *) BPF_FUNC_get_socket_uid;
> >>>    static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> >>>

[RFC v1 13/14] krsi: Provide an example to read and log environment variables

[KP Singh]

From: KP Singh <kpsingh@google.com>

	*  The program takes the name of an environment variable as an
	   argument.
	*  An eBPF program is loaded and attached to the
	   process_execution hook.
	*  The name of the environment variable passed is updated in a
	   eBPF per-cpu map.
	*  The eBPF program uses the krsi_get_env_var helper to get the
	   the value of this variable and logs the result to the perf
	   events buffer.
	*  The user-space program listens to the perf events and prints
	   the values.

Example execution:

	./krsi LD_PRELOAD

	[p_pid=123] LD_PRELOAD is not set
	[p_pid=456] LD_PRELOAD=/lib/bad.so
	[p_pid=789] WARNING! LD_PRELOAD is set 2 times
	[p_pid=789] LD_PRELOAD=/lib/decoy.so
	[p_pid=789] LD_PRELOAD=/lib/bad.so

In a separate session the following [1, 2, 3] exec system calls
are made where:

	[1, 2, 3] char *argv[] = {"/bin/ls", 0};

	[1] char *envp = {0};
	[2] char *envp = {"LD_PRELOAD=/lib/bad.so", 0};
	[3] char *envp = {"LD_PRELOAD=/lib/decoy.so, "LD_PRELOAD=/lib/bad.so", 0};

This example demonstrates that user-space is free to choose the format
in which the data is logged and can use very specific helpers like
krsi_get_env_var to populate only the data that is required.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 MAINTAINERS                |   3 +
 samples/bpf/.gitignore     |   1 +
 samples/bpf/Makefile       |   3 +
 samples/bpf/krsi_helpers.h |  31 ++++++
 samples/bpf/krsi_kern.c    |  52 ++++++++++
 samples/bpf/krsi_user.c    | 202 +++++++++++++++++++++++++++++++++++++
 6 files changed, 292 insertions(+)
 create mode 100644 samples/bpf/krsi_helpers.h
 create mode 100644 samples/bpf/krsi_kern.c
 create mode 100644 samples/bpf/krsi_user.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e0364391d8b..ec378abb4c23 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9005,6 +9005,9 @@ F:	kernel/kprobes.c
 KRSI SECURITY MODULE
 M:	KP Singh <kpsingh@chromium.org>
 S:	Supported
+F:	samples/bpf/krsi_helpers.h
+F:	samples/bpf/krsi_kern.c
+F:	samples/bpf/krsi_user.c
 F:	security/krsi/
 
 KS0108 LCD CONTROLLER DRIVER
diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
index 74d31fd3c99c..6bbf5a04877f 100644
--- a/samples/bpf/.gitignore
+++ b/samples/bpf/.gitignore
@@ -2,6 +2,7 @@ cpustat
 fds_example
 hbm
 ibumad
+krsi
 lathist
 lwt_len_hist
 map_perf_test
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 1d9be26b4edd..33d3bef17549 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -8,6 +8,7 @@ hostprogs-y := test_lru_dist
 hostprogs-y += sock_example
 hostprogs-y += fds_example
 hostprogs-y += sockex1
+hostprogs-y += krsi
 hostprogs-y += sockex2
 hostprogs-y += sockex3
 hostprogs-y += tracex1
@@ -62,6 +63,7 @@ TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
 
 fds_example-objs := fds_example.o
 sockex1-objs := sockex1_user.o
+krsi-objs := krsi_user.o $(TRACE_HELPERS)
 sockex2-objs := sockex2_user.o
 sockex3-objs := bpf_load.o sockex3_user.o
 tracex1-objs := bpf_load.o tracex1_user.o
@@ -113,6 +115,7 @@ hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
 # Tell kbuild to always build the programs
 always := $(hostprogs-y)
 always += sockex1_kern.o
+always += krsi_kern.o
 always += sockex2_kern.o
 always += sockex3_kern.o
 always += tracex1_kern.o
diff --git a/samples/bpf/krsi_helpers.h b/samples/bpf/krsi_helpers.h
new file mode 100644
index 000000000000..3007bfd6212e
--- /dev/null
+++ b/samples/bpf/krsi_helpers.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _KRSI_HELPERS_H
+#define _KRSI_HELPERS_H
+
+#define __bpf_percpu_val_align __aligned(8)
+
+#define ENV_VAR_NAME_MAX_LEN 48
+#define ENV_VAR_VAL_MAX_LEN 4096
+
+#define MAX_CPUS 128
+
+#define __LOWER(x) (x & 0xffffffff)
+#define __UPPER(x) (x >> 32)
+
+struct krsi_env_value {
+	// The name of the environment variable.
+	char name[ENV_VAR_NAME_MAX_LEN];
+	// The value of the environment variable (if set).
+	char value[ENV_VAR_VAL_MAX_LEN];
+	// Indicates if an overflow occurred while reading the value of the
+	// of the environment variable. This means that an -E2BIG was received
+	// from the krsi_get_env_var helper.
+	bool overflow;
+	// The number of times the environment variable was set.
+	__u32 times;
+	// The PID of the parent process.
+	__u32 p_pid;
+} __bpf_percpu_val_align;
+
+#endif // _KRSI_HELPERS_H
diff --git a/samples/bpf/krsi_kern.c b/samples/bpf/krsi_kern.c
new file mode 100644
index 000000000000..087a6f0cc81d
--- /dev/null
+++ b/samples/bpf/krsi_kern.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ptrace.h>
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/ip.h>
+#include "bpf_helpers.h"
+#include "krsi_helpers.h"
+
+#define MAX_CPUS 128
+
+struct bpf_map_def SEC("maps") env_map = {
+	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
+	.key_size = sizeof(u32),
+	.value_size = sizeof(struct krsi_env_value),
+	.max_entries = 1,
+};
+
+struct bpf_map_def SEC("maps") perf_map = {
+	.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
+	.key_size = sizeof(int),
+	.value_size = sizeof(u32),
+	.max_entries = MAX_CPUS,
+};
+
+SEC("krsi")
+int env_dumper(void *ctx)
+{
+	u64 times_ret;
+	s32 ret;
+	u32 map_id = 0;
+	char *map_value;
+	struct krsi_env_value *env;
+
+	env = bpf_map_lookup_elem(&env_map, &map_id);
+	if (!env)
+		return -ENOMEM;
+	times_ret = krsi_get_env_var(ctx, env->name, ENV_VAR_NAME_MAX_LEN,
+				     env->value, ENV_VAR_VAL_MAX_LEN);
+	ret = __LOWER(times_ret);
+	if (ret == -E2BIG)
+		env->overflow = true;
+	else if (ret < 0)
+		return ret;
+
+	env->times = __UPPER(times_ret);
+	env->p_pid = bpf_get_current_pid_tgid();
+	bpf_perf_event_output(ctx, &perf_map, BPF_F_CURRENT_CPU, env,
+			      sizeof(struct krsi_env_value));
+
+	return 0;
+}
+char _license[] SEC("license") = "GPL";
diff --git a/samples/bpf/krsi_user.c b/samples/bpf/krsi_user.c
new file mode 100644
index 000000000000..1fad29bf017a
--- /dev/null
+++ b/samples/bpf/krsi_user.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <err.h>
+#include <assert.h>
+#include <linux/limits.h>
+#include <linux/bpf.h>
+#include <bpf/bpf.h>
+#include "bpf/libbpf.h"
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/sysinfo.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <sys/resource.h>
+
+#include "perf-sys.h"
+#include "trace_helpers.h"
+#include "krsi_helpers.h"
+
+#define LSM_HOOK_PATH "/sys/kernel/security/krsi/process_execution"
+
+static int pmu_fds[MAX_CPUS];
+static struct perf_event_mmap_page *headers[MAX_CPUS];
+
+static int print_env(void *d, int size)
+{
+	struct krsi_env_value *env = d;
+	int times = env->times;
+	char *next = env->value;
+	size_t total = 0;
+
+	if (env->times > 1)
+		printf("[p_pid=%u] WARNING! %s is set %u times\n",
+			env->p_pid, env->name, env->times);
+
+	/*
+	 * krsi_get_env_var ensures that even overflows
+	 * are null terminated. Incase of an overflow,
+	 * this logic tries to print as much information
+	 * that was gathered.
+	 */
+	while (times && total < ENV_VAR_NAME_MAX_LEN) {
+		next += total;
+		if (env->overflow)
+			printf("[p_pid=%u] OVERFLOW! %s=%s\n",
+			       env->p_pid, env->name, next);
+		else
+			printf("[p_pid=%u] %s=%s\n",
+			       env->p_pid, env->name, next);
+		times--;
+		total += strlen(next) + 1;
+	}
+
+	if (!env->times)
+		printf("p_pid=%u] %s is not set\n",
+		       env->p_pid, env->name);
+
+	return LIBBPF_PERF_EVENT_CONT;
+}
+
+static int open_perf_events(int map_fd, int num)
+{
+	int i;
+	struct perf_event_attr attr = {
+		.sample_type = PERF_SAMPLE_RAW,
+		.type = PERF_TYPE_SOFTWARE,
+		.config = PERF_COUNT_SW_BPF_OUTPUT,
+		.wakeup_events = 1, /* get an fd notification for every event */
+	};
+
+	for (i = 0; i < num; i++) {
+		int key = i;
+		int ret;
+
+		ret = sys_perf_event_open(&attr, -1 /*pid*/, i/*cpu*/,
+					 -1/*group_fd*/, 0);
+		if (ret < 0)
+			return ret;
+		pmu_fds[i] = ret;
+		ret = bpf_map_update_elem(map_fd, &key, &pmu_fds[i], BPF_ANY);
+		if (ret < 0)
+			return ret;
+		ioctl(pmu_fds[i], PERF_EVENT_IOC_ENABLE, 0);
+	}
+	return 0;
+}
+
+static int update_env_map(struct bpf_object *prog_obj, const char *env_var_name,
+			  int numcpus)
+{
+	struct bpf_map *map;
+	struct krsi_env_value *env;
+	int map_fd;
+	int key = 0, ret = 0, i;
+
+	map = bpf_object__find_map_by_name(prog_obj, "env_map");
+	if (!map)
+		return -EINVAL;
+
+	map_fd = bpf_map__fd(map);
+	if (map_fd < 0)
+		return map_fd;
+
+	env = malloc(numcpus * sizeof(struct krsi_env_value));
+	if (!env) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	for (i = 0; i < numcpus; i++)
+		strcpy(env[i].name, env_var_name);
+
+	ret = bpf_map_update_elem(map_fd, &key, env, BPF_ANY);
+	if (ret < 0)
+		goto out;
+
+out:
+	free(env);
+	return ret;
+}
+
+int main(int argc, char **argv)
+{
+	struct bpf_object *prog_obj;
+	const char *env_var_name;
+	struct bpf_prog_load_attr attr;
+	int prog_fd, target_fd, map_fd;
+	int ret, i, numcpus;
+	struct bpf_map *map;
+	char filename[PATH_MAX];
+	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+
+
+	if (argc != 2)
+		errx(EXIT_FAILURE, "Usage %s env_var_name\n", argv[0]);
+
+	env_var_name = argv[1];
+	if (strlen(env_var_name) > ENV_VAR_NAME_MAX_LEN - 1)
+		errx(EXIT_FAILURE,
+		     "<env_var_name> cannot be more than %d in length",
+		     ENV_VAR_NAME_MAX_LEN - 1);
+
+
+	setrlimit(RLIMIT_MEMLOCK, &r);
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+
+	memset(&attr, 0, sizeof(struct bpf_prog_load_attr));
+	attr.prog_type = BPF_PROG_TYPE_KRSI;
+	attr.expected_attach_type = BPF_KRSI;
+	attr.file = filename;
+
+	/* Attach the BPF program to the given hook */
+	target_fd = open(LSM_HOOK_PATH, O_RDWR);
+	if (target_fd < 0)
+		err(EXIT_FAILURE, "Failed to open target file");
+
+	if (bpf_prog_load_xattr(&attr, &prog_obj, &prog_fd))
+		err(EXIT_FAILURE, "Failed to load eBPF program");
+
+	numcpus = get_nprocs();
+	if (numcpus > MAX_CPUS)
+		numcpus = MAX_CPUS;
+
+	ret = update_env_map(prog_obj, env_var_name, numcpus);
+	if (ret < 0)
+		err(EXIT_FAILURE, "Failed to update env map");
+
+	map = bpf_object__find_map_by_name(prog_obj, "perf_map");
+	if (!map)
+		err(EXIT_FAILURE,
+		    "Finding the perf event map in obj file failed");
+
+	map_fd = bpf_map__fd(map);
+	if (map_fd < 0)
+		err(EXIT_FAILURE, "Failed to get fd for perf events map");
+
+	ret = bpf_prog_attach(prog_fd, target_fd, BPF_KRSI,
+			      BPF_F_ALLOW_OVERRIDE);
+	if (ret < 0)
+		err(EXIT_FAILURE, "Failed to attach prog to LSM hook");
+
+	ret = open_perf_events(map_fd, numcpus);
+	if (ret < 0)
+		err(EXIT_FAILURE, "Failed to open perf events handler");
+
+	for (i = 0; i < numcpus; i++) {
+		ret = perf_event_mmap_header(pmu_fds[i], &headers[i]);
+		if (ret < 0)
+			err(EXIT_FAILURE, "perf_event_mmap_header");
+	}
+
+	ret = perf_event_poller_multi(pmu_fds, headers, numcpus, print_env);
+	if (ret < 0)
+		err(EXIT_FAILURE, "Failed to poll perf events");
+
+	return EXIT_SUCCESS;
+}
-- 
2.20.1

Re: [RFC v1 13/14] krsi: Provide an example to read and log environment variables

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> 	*  The program takes the name of an environment variable as an
> 	   argument.
> 	*  An eBPF program is loaded and attached to the
> 	   process_execution hook.
> 	*  The name of the environment variable passed is updated in a
> 	   eBPF per-cpu map.
> 	*  The eBPF program uses the krsi_get_env_var helper to get the
> 	   the value of this variable and logs the result to the perf
> 	   events buffer.
> 	*  The user-space program listens to the perf events and prints
> 	   the values.
> 
> Example execution:
> 
> 	./krsi LD_PRELOAD
> 
> 	[p_pid=123] LD_PRELOAD is not set
> 	[p_pid=456] LD_PRELOAD=/lib/bad.so
> 	[p_pid=789] WARNING! LD_PRELOAD is set 2 times
> 	[p_pid=789] LD_PRELOAD=/lib/decoy.so
> 	[p_pid=789] LD_PRELOAD=/lib/bad.so
> 
> In a separate session the following [1, 2, 3] exec system calls
> are made where:
> 
> 	[1, 2, 3] char *argv[] = {"/bin/ls", 0};
> 
> 	[1] char *envp = {0};
> 	[2] char *envp = {"LD_PRELOAD=/lib/bad.so", 0};
> 	[3] char *envp = {"LD_PRELOAD=/lib/decoy.so, "LD_PRELOAD=/lib/bad.so", 0};
> 
> This example demonstrates that user-space is free to choose the format
> in which the data is logged and can use very specific helpers like
> krsi_get_env_var to populate only the data that is required.
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   MAINTAINERS                |   3 +
>   samples/bpf/.gitignore     |   1 +
>   samples/bpf/Makefile       |   3 +
>   samples/bpf/krsi_helpers.h |  31 ++++++
>   samples/bpf/krsi_kern.c    |  52 ++++++++++
>   samples/bpf/krsi_user.c    | 202 +++++++++++++++++++++++++++++++++++++

The KRSI does not depend on kernel headers.
I would recommend to put this into tools/testing/selftests/bpf
as a selftest. Selftest is the place to test regressions with
bpf changes.

The krsi_kern.c can be put into tools/testing/selftests/bpf/progs
and krsi_user.c can be put into tools/testing/selftests/bpf/prog_tests.

>   6 files changed, 292 insertions(+)
>   create mode 100644 samples/bpf/krsi_helpers.h
>   create mode 100644 samples/bpf/krsi_kern.c
>   create mode 100644 samples/bpf/krsi_user.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e0364391d8b..ec378abb4c23 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9005,6 +9005,9 @@ F:	kernel/kprobes.c
>   KRSI SECURITY MODULE
>   M:	KP Singh <kpsingh@chromium.org>
>   S:	Supported
> +F:	samples/bpf/krsi_helpers.h
> +F:	samples/bpf/krsi_kern.c
> +F:	samples/bpf/krsi_user.c
>   F:	security/krsi/
>   
>   KS0108 LCD CONTROLLER DRIVER
> diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
> index 74d31fd3c99c..6bbf5a04877f 100644
> --- a/samples/bpf/.gitignore
> +++ b/samples/bpf/.gitignore
> @@ -2,6 +2,7 @@ cpustat
>   fds_example
>   hbm
>   ibumad
> +krsi
>   lathist
>   lwt_len_hist
>   map_perf_test
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 1d9be26b4edd..33d3bef17549 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -8,6 +8,7 @@ hostprogs-y := test_lru_dist
>   hostprogs-y += sock_example
>   hostprogs-y += fds_example
>   hostprogs-y += sockex1
> +hostprogs-y += krsi
>   hostprogs-y += sockex2
>   hostprogs-y += sockex3
>   hostprogs-y += tracex1
> @@ -62,6 +63,7 @@ TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
>   
>   fds_example-objs := fds_example.o
>   sockex1-objs := sockex1_user.o
> +krsi-objs := krsi_user.o $(TRACE_HELPERS)
>   sockex2-objs := sockex2_user.o
>   sockex3-objs := bpf_load.o sockex3_user.o
>   tracex1-objs := bpf_load.o tracex1_user.o
> @@ -113,6 +115,7 @@ hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
>   # Tell kbuild to always build the programs
>   always := $(hostprogs-y)
>   always += sockex1_kern.o
> +always += krsi_kern.o
>   always += sockex2_kern.o
>   always += sockex3_kern.o
>   always += tracex1_kern.o
> diff --git a/samples/bpf/krsi_helpers.h b/samples/bpf/krsi_helpers.h
> new file mode 100644
> index 000000000000..3007bfd6212e
> --- /dev/null
> +++ b/samples/bpf/krsi_helpers.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _KRSI_HELPERS_H
> +#define _KRSI_HELPERS_H
> +
> +#define __bpf_percpu_val_align __aligned(8)
> +
> +#define ENV_VAR_NAME_MAX_LEN 48
> +#define ENV_VAR_VAL_MAX_LEN 4096
> +
> +#define MAX_CPUS 128
> +
> +#define __LOWER(x) (x & 0xffffffff)
> +#define __UPPER(x) (x >> 32)
> +
> +struct krsi_env_value {
> +	// The name of the environment variable.
> +	char name[ENV_VAR_NAME_MAX_LEN];
> +	// The value of the environment variable (if set).
> +	char value[ENV_VAR_VAL_MAX_LEN];
> +	// Indicates if an overflow occurred while reading the value of the
> +	// of the environment variable. This means that an -E2BIG was received
> +	// from the krsi_get_env_var helper.
> +	bool overflow;
> +	// The number of times the environment variable was set.
> +	__u32 times;
> +	// The PID of the parent process.
> +	__u32 p_pid;
> +} __bpf_percpu_val_align;
> +
> +#endif // _KRSI_HELPERS_H
> diff --git a/samples/bpf/krsi_kern.c b/samples/bpf/krsi_kern.c
> new file mode 100644
> index 000000000000..087a6f0cc81d
> --- /dev/null
> +++ b/samples/bpf/krsi_kern.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/ptrace.h>
> +#include <uapi/linux/bpf.h>
> +#include <uapi/linux/ip.h>
> +#include "bpf_helpers.h"
> +#include "krsi_helpers.h"
> +
> +#define MAX_CPUS 128
> +
> +struct bpf_map_def SEC("maps") env_map = {
> +	.type = BPF_MAP_TYPE_PERCPU_ARRAY,
> +	.key_size = sizeof(u32),
> +	.value_size = sizeof(struct krsi_env_value),
> +	.max_entries = 1,
> +};
> +
> +struct bpf_map_def SEC("maps") perf_map = {
> +	.type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> +	.key_size = sizeof(int),
> +	.value_size = sizeof(u32),
> +	.max_entries = MAX_CPUS,
> +};
> +
> +SEC("krsi")
> +int env_dumper(void *ctx)
> +{
> +	u64 times_ret;
> +	s32 ret;
> +	u32 map_id = 0;
> +	char *map_value;
> +	struct krsi_env_value *env;
> +
> +	env = bpf_map_lookup_elem(&env_map, &map_id);
> +	if (!env)
> +		return -ENOMEM;
> +	times_ret = krsi_get_env_var(ctx, env->name, ENV_VAR_NAME_MAX_LEN,
> +				     env->value, ENV_VAR_VAL_MAX_LEN);
> +	ret = __LOWER(times_ret);
> +	if (ret == -E2BIG)
> +		env->overflow = true;
> +	else if (ret < 0)
> +		return ret;
> +
> +	env->times = __UPPER(times_ret);
> +	env->p_pid = bpf_get_current_pid_tgid();
> +	bpf_perf_event_output(ctx, &perf_map, BPF_F_CURRENT_CPU, env,
> +			      sizeof(struct krsi_env_value));
> +
> +	return 0;
> +}
> +char _license[] SEC("license") = "GPL";
> diff --git a/samples/bpf/krsi_user.c b/samples/bpf/krsi_user.c
> new file mode 100644
> index 000000000000..1fad29bf017a
> --- /dev/null
> +++ b/samples/bpf/krsi_user.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <err.h>
> +#include <assert.h>
> +#include <linux/limits.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf.h>
> +#include "bpf/libbpf.h"
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/sysinfo.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +#include <sys/resource.h>
> +
> +#include "perf-sys.h"
> +#include "trace_helpers.h"
> +#include "krsi_helpers.h"
> +
> +#define LSM_HOOK_PATH "/sys/kernel/security/krsi/process_execution"
> +
> +static int pmu_fds[MAX_CPUS];
> +static struct perf_event_mmap_page *headers[MAX_CPUS];
> +
> +static int print_env(void *d, int size)
> +{
> +	struct krsi_env_value *env = d;
> +	int times = env->times;
> +	char *next = env->value;
> +	size_t total = 0;
> +
> +	if (env->times > 1)
> +		printf("[p_pid=%u] WARNING! %s is set %u times\n",
> +			env->p_pid, env->name, env->times);
> +
> +	/*
> +	 * krsi_get_env_var ensures that even overflows
> +	 * are null terminated. Incase of an overflow,
> +	 * this logic tries to print as much information
> +	 * that was gathered.
> +	 */
> +	while (times && total < ENV_VAR_NAME_MAX_LEN) {
> +		next += total;
> +		if (env->overflow)
> +			printf("[p_pid=%u] OVERFLOW! %s=%s\n",
> +			       env->p_pid, env->name, next);
> +		else
> +			printf("[p_pid=%u] %s=%s\n",
> +			       env->p_pid, env->name, next);
> +		times--;
> +		total += strlen(next) + 1;
> +	}
> +
> +	if (!env->times)
> +		printf("p_pid=%u] %s is not set\n",
> +		       env->p_pid, env->name);
> +
> +	return LIBBPF_PERF_EVENT_CONT;
> +}
> +
> +static int open_perf_events(int map_fd, int num)
> +{
> +	int i;
> +	struct perf_event_attr attr = {
> +		.sample_type = PERF_SAMPLE_RAW,
> +		.type = PERF_TYPE_SOFTWARE,
> +		.config = PERF_COUNT_SW_BPF_OUTPUT,
> +		.wakeup_events = 1, /* get an fd notification for every event */
> +	};
> +
> +	for (i = 0; i < num; i++) {
> +		int key = i;
> +		int ret;
> +
> +		ret = sys_perf_event_open(&attr, -1 /*pid*/, i/*cpu*/,
> +					 -1/*group_fd*/, 0);
> +		if (ret < 0)
> +			return ret;
> +		pmu_fds[i] = ret;
> +		ret = bpf_map_update_elem(map_fd, &key, &pmu_fds[i], BPF_ANY);
> +		if (ret < 0)
> +			return ret;
> +		ioctl(pmu_fds[i], PERF_EVENT_IOC_ENABLE, 0);
> +	}
> +	return 0;
> +}
> +
> +static int update_env_map(struct bpf_object *prog_obj, const char *env_var_name,
> +			  int numcpus)
> +{
> +	struct bpf_map *map;
> +	struct krsi_env_value *env;
> +	int map_fd;
> +	int key = 0, ret = 0, i;
> +
> +	map = bpf_object__find_map_by_name(prog_obj, "env_map");
> +	if (!map)
> +		return -EINVAL;
> +
> +	map_fd = bpf_map__fd(map);
> +	if (map_fd < 0)
> +		return map_fd;
> +
> +	env = malloc(numcpus * sizeof(struct krsi_env_value));
> +	if (!env) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	for (i = 0; i < numcpus; i++)
> +		strcpy(env[i].name, env_var_name);
> +
> +	ret = bpf_map_update_elem(map_fd, &key, env, BPF_ANY);
> +	if (ret < 0)
> +		goto out;
> +
> +out:
> +	free(env);
> +	return ret;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	struct bpf_object *prog_obj;
> +	const char *env_var_name;
> +	struct bpf_prog_load_attr attr;
> +	int prog_fd, target_fd, map_fd;
> +	int ret, i, numcpus;
> +	struct bpf_map *map;
> +	char filename[PATH_MAX];
> +	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
> +
> +
> +	if (argc != 2)
> +		errx(EXIT_FAILURE, "Usage %s env_var_name\n", argv[0]);
> +
> +	env_var_name = argv[1];
> +	if (strlen(env_var_name) > ENV_VAR_NAME_MAX_LEN - 1)
> +		errx(EXIT_FAILURE,
> +		     "<env_var_name> cannot be more than %d in length",
> +		     ENV_VAR_NAME_MAX_LEN - 1);
> +
> +
> +	setrlimit(RLIMIT_MEMLOCK, &r);
> +	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> +
> +	memset(&attr, 0, sizeof(struct bpf_prog_load_attr));
> +	attr.prog_type = BPF_PROG_TYPE_KRSI;
> +	attr.expected_attach_type = BPF_KRSI;
> +	attr.file = filename;
> +
> +	/* Attach the BPF program to the given hook */
> +	target_fd = open(LSM_HOOK_PATH, O_RDWR);
> +	if (target_fd < 0)
> +		err(EXIT_FAILURE, "Failed to open target file");
> +
> +	if (bpf_prog_load_xattr(&attr, &prog_obj, &prog_fd))
> +		err(EXIT_FAILURE, "Failed to load eBPF program");
> +
> +	numcpus = get_nprocs();
> +	if (numcpus > MAX_CPUS)
> +		numcpus = MAX_CPUS;
> +
> +	ret = update_env_map(prog_obj, env_var_name, numcpus);
> +	if (ret < 0)
> +		err(EXIT_FAILURE, "Failed to update env map");
> +
> +	map = bpf_object__find_map_by_name(prog_obj, "perf_map");
> +	if (!map)
> +		err(EXIT_FAILURE,
> +		    "Finding the perf event map in obj file failed");
> +
> +	map_fd = bpf_map__fd(map);
> +	if (map_fd < 0)
> +		err(EXIT_FAILURE, "Failed to get fd for perf events map");
> +
> +	ret = bpf_prog_attach(prog_fd, target_fd, BPF_KRSI,
> +			      BPF_F_ALLOW_OVERRIDE);
> +	if (ret < 0)
> +		err(EXIT_FAILURE, "Failed to attach prog to LSM hook");
> +
> +	ret = open_perf_events(map_fd, numcpus);
> +	if (ret < 0)
> +		err(EXIT_FAILURE, "Failed to open perf events handler");
> +
> +	for (i = 0; i < numcpus; i++) {
> +		ret = perf_event_mmap_header(pmu_fds[i], &headers[i]);
> +		if (ret < 0)
> +			err(EXIT_FAILURE, "perf_event_mmap_header");
> +	}
> +
> +	ret = perf_event_poller_multi(pmu_fds, headers, numcpus, print_env);
> +	if (ret < 0)
> +		err(EXIT_FAILURE, "Failed to poll perf events");
> +
> +	return EXIT_SUCCESS;
> +}
> 

[RFC v1 14/14] krsi: Pin arg pages only when needed

[KP Singh]

From: KP Singh <kpsingh@google.com>

Adds a callback which is called when a new program is attached
to a hook. The callback registered by the process_exection hook
checks if a program that has calls to a helper that requires pages to
be pinned (eg. krsi_get_env_var).

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/krsi.h              |  1 +
 security/krsi/include/hooks.h     |  5 ++-
 security/krsi/include/krsi_init.h |  7 ++++
 security/krsi/krsi.c              | 62 ++++++++++++++++++++++++++++---
 security/krsi/ops.c               | 10 ++++-
 5 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/include/linux/krsi.h b/include/linux/krsi.h
index c7d1790d0c1f..e443d0309764 100644
--- a/include/linux/krsi.h
+++ b/include/linux/krsi.h
@@ -7,6 +7,7 @@
 
 #ifdef CONFIG_SECURITY_KRSI
 int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
+extern const struct bpf_func_proto krsi_get_env_var_proto;
 #else
 static inline int krsi_prog_attach(const union bpf_attr *attr,
 				   struct bpf_prog *prog)
diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
index e070c452b5de..38293125ff99 100644
--- a/security/krsi/include/hooks.h
+++ b/security/krsi/include/hooks.h
@@ -8,7 +8,7 @@
  *
  * Format:
  *
- *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
+ *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK)
  *
  * KRSI adds one layer of indirection between the name of the hook and the name
  * it exposes to the userspace in Security FS to prevent the userspace from
@@ -18,4 +18,5 @@
 KRSI_HOOK_INIT(PROCESS_EXECUTION,
 	       process_execution,
 	       bprm_check_security,
-	       krsi_process_execution)
+	       krsi_process_execution,
+	       krsi_process_execution_cb)
diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
index 6152847c3b08..99801d5b273a 100644
--- a/security/krsi/include/krsi_init.h
+++ b/security/krsi/include/krsi_init.h
@@ -31,6 +31,8 @@ struct krsi_ctx {
 	};
 };
 
+typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *);
+
 /*
  * The LSM creates one file per hook.
  *
@@ -61,6 +63,11 @@ struct krsi_hook {
 	 * The eBPF programs that are attached to this hook.
 	 */
 	struct bpf_prog_array __rcu	*progs;
+	/*
+	 * The attach callback is called before a new program is attached
+	 * to the hook and is passed the updated bpf_prog_array as an argument.
+	 */
+	krsi_prog_attach_t attach_callback;
 };
 
 extern struct krsi_hook krsi_hooks_list[];
diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
index 00a7150c1b22..a4443d7aa150 100644
--- a/security/krsi/krsi.c
+++ b/security/krsi/krsi.c
@@ -5,15 +5,65 @@
 #include <linux/bpf.h>
 #include <linux/binfmts.h>
 #include <linux/highmem.h>
+#include <linux/krsi.h>
 #include <linux/mm.h>
 
 #include "krsi_init.h"
 
+/*
+ * need_arg_pages is only updated in bprm_check_security_cb
+ * when a mutex on krsi_hook for bprm_check_security is already
+ * held. need_arg_pages avoids pinning pages when no program
+ * that needs them is attached to the hook.
+ */
+static bool need_arg_pages;
+
+/*
+ * Checks if the instruction is a BPF_CALL to an eBPF helper located
+ * at the given address.
+ */
+static inline bool bpf_is_call_to_func(struct bpf_insn *insn,
+				       void *func_addr)
+{
+	u8 opcode = BPF_OP(insn->code);
+
+	if (opcode != BPF_CALL)
+		return false;
+
+	if (insn->src_reg == BPF_PSEUDO_CALL)
+		return false;
+
+	/*
+	 * The BPF verifier updates the value of insn->imm from the
+	 * enum bpf_func_id to the offset of the address of helper
+	 * from the __bpf_call_base.
+	 */
+	return __bpf_call_base + insn->imm == func_addr;
+}
+
+static int krsi_process_execution_cb(struct bpf_prog_array *array)
+{
+	struct bpf_prog_array_item *item = array->items;
+	struct bpf_prog *p;
+	const struct bpf_func_proto *proto = &krsi_get_env_var_proto;
+	int i;
+
+	while ((p = READ_ONCE(item->prog))) {
+		for (i = 0; i < p->len; i++) {
+			if (bpf_is_call_to_func(&p->insnsi[i], proto->func))
+				need_arg_pages = true;
+		}
+		item++;
+	}
+	return 0;
+}
+
 struct krsi_hook krsi_hooks_list[] = {
-	#define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
+	#define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \
 		[TYPE] = { \
 			.h_type = TYPE, \
 			.name = #NAME, \
+			.attach_callback = CB, \
 		},
 	#include "hooks.h"
 	#undef KRSI_HOOK_INIT
@@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm)
 		.bprm = bprm,
 	};
 
-	ret = pin_arg_pages(&ctx.bprm_ctx);
-	if (ret < 0)
-		goto out_arg_pages;
+	if (READ_ONCE(need_arg_pages)) {
+		ret = pin_arg_pages(&ctx.bprm_ctx);
+		if (ret < 0)
+			goto out_arg_pages;
+	}
 
 	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
 	kfree(ctx.bprm_ctx.arg_pages);
@@ -87,7 +139,7 @@ static int krsi_process_execution(struct linux_binprm *bprm)
 }
 
 static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
-	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
+	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL, CB) LSM_HOOK_INIT(HOOK, IMPL),
 	#include "hooks.h"
 	#undef KRSI_HOOK_INIT
 };
diff --git a/security/krsi/ops.c b/security/krsi/ops.c
index 1db94dfaac15..2de682371eff 100644
--- a/security/krsi/ops.c
+++ b/security/krsi/ops.c
@@ -139,6 +139,14 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 		goto unlock;
 	}
 
+	if (h->attach_callback) {
+		ret = h->attach_callback(new_array);
+		if (ret < 0) {
+			bpf_prog_array_free(new_array);
+			goto unlock;
+		}
+	}
+
 	rcu_assign_pointer(h->progs, new_array);
 	bpf_prog_array_free(old_array);
 
@@ -278,7 +286,7 @@ BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
 	return get_env_var(ctx, name, dest, n_size, size);
 }
 
-static const struct bpf_func_proto krsi_get_env_var_proto = {
+const struct bpf_func_proto krsi_get_env_var_proto = {
 	.func = krsi_get_env_var,
 	.gpl_only = true,
 	.ret_type = RET_INTEGER,
-- 
2.20.1

Re: [RFC v1 14/14] krsi: Pin arg pages only when needed

[Yonghong Song]

On 9/10/19 12:55 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> Adds a callback which is called when a new program is attached
> to a hook. The callback registered by the process_exection hook
> checks if a program that has calls to a helper that requires pages to
> be pinned (eg. krsi_get_env_var).
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>   include/linux/krsi.h              |  1 +
>   security/krsi/include/hooks.h     |  5 ++-
>   security/krsi/include/krsi_init.h |  7 ++++
>   security/krsi/krsi.c              | 62 ++++++++++++++++++++++++++++---
>   security/krsi/ops.c               | 10 ++++-
>   5 files changed, 77 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/krsi.h b/include/linux/krsi.h
> index c7d1790d0c1f..e443d0309764 100644
> --- a/include/linux/krsi.h
> +++ b/include/linux/krsi.h
> @@ -7,6 +7,7 @@
>   
>   #ifdef CONFIG_SECURITY_KRSI
>   int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
> +extern const struct bpf_func_proto krsi_get_env_var_proto;
>   #else
>   static inline int krsi_prog_attach(const union bpf_attr *attr,
>   				   struct bpf_prog *prog)
> diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
> index e070c452b5de..38293125ff99 100644
> --- a/security/krsi/include/hooks.h
> +++ b/security/krsi/include/hooks.h
> @@ -8,7 +8,7 @@
>    *
>    * Format:
>    *
> - *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
> + *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK)
>    *
>    * KRSI adds one layer of indirection between the name of the hook and the name
>    * it exposes to the userspace in Security FS to prevent the userspace from
> @@ -18,4 +18,5 @@
>   KRSI_HOOK_INIT(PROCESS_EXECUTION,
>   	       process_execution,
>   	       bprm_check_security,
> -	       krsi_process_execution)
> +	       krsi_process_execution,
> +	       krsi_process_execution_cb)
> diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
> index 6152847c3b08..99801d5b273a 100644
> --- a/security/krsi/include/krsi_init.h
> +++ b/security/krsi/include/krsi_init.h
> @@ -31,6 +31,8 @@ struct krsi_ctx {
>   	};
>   };
>   
> +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *);
> +
>   /*
>    * The LSM creates one file per hook.
>    *
> @@ -61,6 +63,11 @@ struct krsi_hook {
>   	 * The eBPF programs that are attached to this hook.
>   	 */
>   	struct bpf_prog_array __rcu	*progs;
> +	/*
> +	 * The attach callback is called before a new program is attached
> +	 * to the hook and is passed the updated bpf_prog_array as an argument.
> +	 */
> +	krsi_prog_attach_t attach_callback;
>   };
>   
>   extern struct krsi_hook krsi_hooks_list[];
> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
> index 00a7150c1b22..a4443d7aa150 100644
> --- a/security/krsi/krsi.c
> +++ b/security/krsi/krsi.c
> @@ -5,15 +5,65 @@
>   #include <linux/bpf.h>
>   #include <linux/binfmts.h>
>   #include <linux/highmem.h>
> +#include <linux/krsi.h>
>   #include <linux/mm.h>
>   
>   #include "krsi_init.h"
>   
> +/*
> + * need_arg_pages is only updated in bprm_check_security_cb
> + * when a mutex on krsi_hook for bprm_check_security is already
> + * held. need_arg_pages avoids pinning pages when no program
> + * that needs them is attached to the hook.
> + */
> +static bool need_arg_pages;
> +
> +/*
> + * Checks if the instruction is a BPF_CALL to an eBPF helper located
> + * at the given address.
> + */
> +static inline bool bpf_is_call_to_func(struct bpf_insn *insn,
> +				       void *func_addr)
> +{
> +	u8 opcode = BPF_OP(insn->code);
> +
> +	if (opcode != BPF_CALL)
> +		return false;
> +
> +	if (insn->src_reg == BPF_PSEUDO_CALL)
> +		return false;
> +
> +	/*
> +	 * The BPF verifier updates the value of insn->imm from the
> +	 * enum bpf_func_id to the offset of the address of helper
> +	 * from the __bpf_call_base.
> +	 */
> +	return __bpf_call_base + insn->imm == func_addr;

In how many cases, krsi program does not have krsi_get_env_var() helper?

> +}
> +
> +static int krsi_process_execution_cb(struct bpf_prog_array *array)
> +{
> +	struct bpf_prog_array_item *item = array->items;
> +	struct bpf_prog *p;
> +	const struct bpf_func_proto *proto = &krsi_get_env_var_proto;
> +	int i;
> +
> +	while ((p = READ_ONCE(item->prog))) {
> +		for (i = 0; i < p->len; i++) {
> +			if (bpf_is_call_to_func(&p->insnsi[i], proto->func))
> +				need_arg_pages = true;
> +		}
> +		item++;
> +	}
> +	return 0;
> +}
> +
>   struct krsi_hook krsi_hooks_list[] = {
> -	#define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
> +	#define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \
>   		[TYPE] = { \
>   			.h_type = TYPE, \
>   			.name = #NAME, \
> +			.attach_callback = CB, \
>   		},
>   	#include "hooks.h"
>   	#undef KRSI_HOOK_INIT
> @@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm)
>   		.bprm = bprm,
>   	};
>   
> -	ret = pin_arg_pages(&ctx.bprm_ctx);
> -	if (ret < 0)
> -		goto out_arg_pages;
> +	if (READ_ONCE(need_arg_pages)) {
> +		ret = pin_arg_pages(&ctx.bprm_ctx);
> +		if (ret < 0)
> +			goto out_arg_pages;
> +	}
>   
>   	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
>   	kfree(ctx.bprm_ctx.arg_pages);
> @@ -87,7 +139,7 @@ static int krsi_process_execution(struct linux_binprm *bprm)
>   }
>   
>   static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
> -	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
> +	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL, CB) LSM_HOOK_INIT(HOOK, IMPL),
>   	#include "hooks.h"
>   	#undef KRSI_HOOK_INIT
>   };
> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> index 1db94dfaac15..2de682371eff 100644
> --- a/security/krsi/ops.c
> +++ b/security/krsi/ops.c
> @@ -139,6 +139,14 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>   		goto unlock;
>   	}
>   
> +	if (h->attach_callback) {
> +		ret = h->attach_callback(new_array);
> +		if (ret < 0) {
> +			bpf_prog_array_free(new_array);
> +			goto unlock;
> +		}
> +	}
> +
>   	rcu_assign_pointer(h->progs, new_array);
>   	bpf_prog_array_free(old_array);
>   
> @@ -278,7 +286,7 @@ BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
>   	return get_env_var(ctx, name, dest, n_size, size);
>   }
>   
> -static const struct bpf_func_proto krsi_get_env_var_proto = {
> +const struct bpf_func_proto krsi_get_env_var_proto = {
>   	.func = krsi_get_env_var,
>   	.gpl_only = true,
>   	.ret_type = RET_INTEGER,
> 

Re: [RFC v1 14/14] krsi: Pin arg pages only when needed

[KP Singh]

On 15-Sep 00:33, Yonghong Song wrote:
> 
> 
> On 9/10/19 12:55 PM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> > 
> > Adds a callback which is called when a new program is attached
> > to a hook. The callback registered by the process_exection hook
> > checks if a program that has calls to a helper that requires pages to
> > be pinned (eg. krsi_get_env_var).
> > 
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >   include/linux/krsi.h              |  1 +
> >   security/krsi/include/hooks.h     |  5 ++-
> >   security/krsi/include/krsi_init.h |  7 ++++
> >   security/krsi/krsi.c              | 62 ++++++++++++++++++++++++++++---
> >   security/krsi/ops.c               | 10 ++++-
> >   5 files changed, 77 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/linux/krsi.h b/include/linux/krsi.h
> > index c7d1790d0c1f..e443d0309764 100644
> > --- a/include/linux/krsi.h
> > +++ b/include/linux/krsi.h
> > @@ -7,6 +7,7 @@
> >   
> >   #ifdef CONFIG_SECURITY_KRSI
> >   int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
> > +extern const struct bpf_func_proto krsi_get_env_var_proto;
> >   #else
> >   static inline int krsi_prog_attach(const union bpf_attr *attr,
> >   				   struct bpf_prog *prog)
> > diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
> > index e070c452b5de..38293125ff99 100644
> > --- a/security/krsi/include/hooks.h
> > +++ b/security/krsi/include/hooks.h
> > @@ -8,7 +8,7 @@
> >    *
> >    * Format:
> >    *
> > - *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
> > + *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK)
> >    *
> >    * KRSI adds one layer of indirection between the name of the hook and the name
> >    * it exposes to the userspace in Security FS to prevent the userspace from
> > @@ -18,4 +18,5 @@
> >   KRSI_HOOK_INIT(PROCESS_EXECUTION,
> >   	       process_execution,
> >   	       bprm_check_security,
> > -	       krsi_process_execution)
> > +	       krsi_process_execution,
> > +	       krsi_process_execution_cb)
> > diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
> > index 6152847c3b08..99801d5b273a 100644
> > --- a/security/krsi/include/krsi_init.h
> > +++ b/security/krsi/include/krsi_init.h
> > @@ -31,6 +31,8 @@ struct krsi_ctx {
> >   	};
> >   };
> >   
> > +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *);
> > +
> >   /*
> >    * The LSM creates one file per hook.
> >    *
> > @@ -61,6 +63,11 @@ struct krsi_hook {
> >   	 * The eBPF programs that are attached to this hook.
> >   	 */
> >   	struct bpf_prog_array __rcu	*progs;
> > +	/*
> > +	 * The attach callback is called before a new program is attached
> > +	 * to the hook and is passed the updated bpf_prog_array as an argument.
> > +	 */
> > +	krsi_prog_attach_t attach_callback;
> >   };
> >   
> >   extern struct krsi_hook krsi_hooks_list[];
> > diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
> > index 00a7150c1b22..a4443d7aa150 100644
> > --- a/security/krsi/krsi.c
> > +++ b/security/krsi/krsi.c
> > @@ -5,15 +5,65 @@
> >   #include <linux/bpf.h>
> >   #include <linux/binfmts.h>
> >   #include <linux/highmem.h>
> > +#include <linux/krsi.h>
> >   #include <linux/mm.h>
> >   
> >   #include "krsi_init.h"
> >   
> > +/*
> > + * need_arg_pages is only updated in bprm_check_security_cb
> > + * when a mutex on krsi_hook for bprm_check_security is already
> > + * held. need_arg_pages avoids pinning pages when no program
> > + * that needs them is attached to the hook.
> > + */
> > +static bool need_arg_pages;
> > +
> > +/*
> > + * Checks if the instruction is a BPF_CALL to an eBPF helper located
> > + * at the given address.
> > + */
> > +static inline bool bpf_is_call_to_func(struct bpf_insn *insn,
> > +				       void *func_addr)
> > +{
> > +	u8 opcode = BPF_OP(insn->code);
> > +
> > +	if (opcode != BPF_CALL)
> > +		return false;
> > +
> > +	if (insn->src_reg == BPF_PSEUDO_CALL)
> > +		return false;
> > +
> > +	/*
> > +	 * The BPF verifier updates the value of insn->imm from the
> > +	 * enum bpf_func_id to the offset of the address of helper
> > +	 * from the __bpf_call_base.
> > +	 */
> > +	return __bpf_call_base + insn->imm == func_addr;
> 
> In how many cases, krsi program does not have krsi_get_env_var() helper?

It depends, if the user does not choose to use log environment
variables or use the the value as a part of their MAC policy, the
pinning of the pages is not needed.

Also, the pinning is needed since eBPF helpers cannot run a non-atomic
context. It would not be needed if "sleepable eBPF" becomes a thing.

- KP

> 
> > +}
> > +
> > +static int krsi_process_execution_cb(struct bpf_prog_array *array)
> > +{
> > +	struct bpf_prog_array_item *item = array->items;
> > +	struct bpf_prog *p;
> > +	const struct bpf_func_proto *proto = &krsi_get_env_var_proto;
> > +	int i;
> > +
> > +	while ((p = READ_ONCE(item->prog))) {
> > +		for (i = 0; i < p->len; i++) {
> > +			if (bpf_is_call_to_func(&p->insnsi[i], proto->func))
> > +				need_arg_pages = true;
> > +		}
> > +		item++;
> > +	}
> > +	return 0;
> > +}
> > +
> >   struct krsi_hook krsi_hooks_list[] = {
> > -	#define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
> > +	#define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \
> >   		[TYPE] = { \
> >   			.h_type = TYPE, \
> >   			.name = #NAME, \
> > +			.attach_callback = CB, \
> >   		},
> >   	#include "hooks.h"
> >   	#undef KRSI_HOOK_INIT
> > @@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm)
> >   		.bprm = bprm,
> >   	};
> >   
> > -	ret = pin_arg_pages(&ctx.bprm_ctx);
> > -	if (ret < 0)
> > -		goto out_arg_pages;
> > +	if (READ_ONCE(need_arg_pages)) {
> > +		ret = pin_arg_pages(&ctx.bprm_ctx);
> > +		if (ret < 0)
> > +			goto out_arg_pages;
> > +	}
> >   
> >   	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
> >   	kfree(ctx.bprm_ctx.arg_pages);
> > @@ -87,7 +139,7 @@ static int krsi_process_execution(struct linux_binprm *bprm)
> >   }
> >   
> >   static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
> > -	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
> > +	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL, CB) LSM_HOOK_INIT(HOOK, IMPL),
> >   	#include "hooks.h"
> >   	#undef KRSI_HOOK_INIT
> >   };
> > diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> > index 1db94dfaac15..2de682371eff 100644
> > --- a/security/krsi/ops.c
> > +++ b/security/krsi/ops.c
> > @@ -139,6 +139,14 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> >   		goto unlock;
> >   	}
> >   
> > +	if (h->attach_callback) {
> > +		ret = h->attach_callback(new_array);
> > +		if (ret < 0) {
> > +			bpf_prog_array_free(new_array);
> > +			goto unlock;
> > +		}
> > +	}
> > +
> >   	rcu_assign_pointer(h->progs, new_array);
> >   	bpf_prog_array_free(old_array);
> >   
> > @@ -278,7 +286,7 @@ BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> >   	return get_env_var(ctx, name, dest, n_size, size);
> >   }
> >   
> > -static const struct bpf_func_proto krsi_get_env_var_proto = {
> > +const struct bpf_func_proto krsi_get_env_var_proto = {
> >   	.func = krsi_get_env_var,
> >   	.gpl_only = true,
> >   	.ret_type = RET_INTEGER,
> > 

Re: [RFC v1 14/14] krsi: Pin arg pages only when needed

[Yonghong Song]

On 9/15/19 2:40 AM, KP Singh wrote:
> On 15-Sep 00:33, Yonghong Song wrote:
>>
>>
>> On 9/10/19 12:55 PM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>>>
>>> Adds a callback which is called when a new program is attached
>>> to a hook. The callback registered by the process_exection hook
>>> checks if a program that has calls to a helper that requires pages to
>>> be pinned (eg. krsi_get_env_var).
>>>
>>> Signed-off-by: KP Singh <kpsingh@google.com>
>>> ---
>>>    include/linux/krsi.h              |  1 +
>>>    security/krsi/include/hooks.h     |  5 ++-
>>>    security/krsi/include/krsi_init.h |  7 ++++
>>>    security/krsi/krsi.c              | 62 ++++++++++++++++++++++++++++---
>>>    security/krsi/ops.c               | 10 ++++-
>>>    5 files changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/linux/krsi.h b/include/linux/krsi.h
>>> index c7d1790d0c1f..e443d0309764 100644
>>> --- a/include/linux/krsi.h
>>> +++ b/include/linux/krsi.h
>>> @@ -7,6 +7,7 @@
>>>    
>>>    #ifdef CONFIG_SECURITY_KRSI
>>>    int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
>>> +extern const struct bpf_func_proto krsi_get_env_var_proto;
>>>    #else
>>>    static inline int krsi_prog_attach(const union bpf_attr *attr,
>>>    				   struct bpf_prog *prog)
>>> diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
>>> index e070c452b5de..38293125ff99 100644
>>> --- a/security/krsi/include/hooks.h
>>> +++ b/security/krsi/include/hooks.h
>>> @@ -8,7 +8,7 @@
>>>     *
>>>     * Format:
>>>     *
>>> - *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
>>> + *   KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK)
>>>     *
>>>     * KRSI adds one layer of indirection between the name of the hook and the name
>>>     * it exposes to the userspace in Security FS to prevent the userspace from
>>> @@ -18,4 +18,5 @@
>>>    KRSI_HOOK_INIT(PROCESS_EXECUTION,
>>>    	       process_execution,
>>>    	       bprm_check_security,
>>> -	       krsi_process_execution)
>>> +	       krsi_process_execution,
>>> +	       krsi_process_execution_cb)
>>> diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
>>> index 6152847c3b08..99801d5b273a 100644
>>> --- a/security/krsi/include/krsi_init.h
>>> +++ b/security/krsi/include/krsi_init.h
>>> @@ -31,6 +31,8 @@ struct krsi_ctx {
>>>    	};
>>>    };
>>>    
>>> +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *);
>>> +
>>>    /*
>>>     * The LSM creates one file per hook.
>>>     *
>>> @@ -61,6 +63,11 @@ struct krsi_hook {
>>>    	 * The eBPF programs that are attached to this hook.
>>>    	 */
>>>    	struct bpf_prog_array __rcu	*progs;
>>> +	/*
>>> +	 * The attach callback is called before a new program is attached
>>> +	 * to the hook and is passed the updated bpf_prog_array as an argument.
>>> +	 */
>>> +	krsi_prog_attach_t attach_callback;
>>>    };
>>>    
>>>    extern struct krsi_hook krsi_hooks_list[];
>>> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
>>> index 00a7150c1b22..a4443d7aa150 100644
>>> --- a/security/krsi/krsi.c
>>> +++ b/security/krsi/krsi.c
>>> @@ -5,15 +5,65 @@
>>>    #include <linux/bpf.h>
>>>    #include <linux/binfmts.h>
>>>    #include <linux/highmem.h>
>>> +#include <linux/krsi.h>
>>>    #include <linux/mm.h>
>>>    
>>>    #include "krsi_init.h"
>>>    
>>> +/*
>>> + * need_arg_pages is only updated in bprm_check_security_cb
>>> + * when a mutex on krsi_hook for bprm_check_security is already
>>> + * held. need_arg_pages avoids pinning pages when no program
>>> + * that needs them is attached to the hook.
>>> + */
>>> +static bool need_arg_pages;
>>> +
>>> +/*
>>> + * Checks if the instruction is a BPF_CALL to an eBPF helper located
>>> + * at the given address.
>>> + */
>>> +static inline bool bpf_is_call_to_func(struct bpf_insn *insn,
>>> +				       void *func_addr)
>>> +{
>>> +	u8 opcode = BPF_OP(insn->code);
>>> +
>>> +	if (opcode != BPF_CALL)
>>> +		return false;
>>> +
>>> +	if (insn->src_reg == BPF_PSEUDO_CALL)
>>> +		return false;
>>> +
>>> +	/*
>>> +	 * The BPF verifier updates the value of insn->imm from the
>>> +	 * enum bpf_func_id to the offset of the address of helper
>>> +	 * from the __bpf_call_base.
>>> +	 */
>>> +	return __bpf_call_base + insn->imm == func_addr;
>>
>> In how many cases, krsi program does not have krsi_get_env_var() helper?
> 
> It depends, if the user does not choose to use log environment
> variables or use the the value as a part of their MAC policy, the
> pinning of the pages is not needed.

Thanks. I just want to know whether we want to optimize such cases.
I am not a security expert, so I am okay with whatever decision you
made.

> 
> Also, the pinning is needed since eBPF helpers cannot run a non-atomic
> context. It would not be needed if "sleepable eBPF" becomes a thing.

Indeed. 'sleepable BPF' might be also a good thing for realtime linux.
Some works need to address bpf area spin lock, per cpu variables, etc.
before that can happen.

> 
> - KP
> 
>>
>>> +}
>>> +
>>> +static int krsi_process_execution_cb(struct bpf_prog_array *array)
>>> +{
>>> +	struct bpf_prog_array_item *item = array->items;
>>> +	struct bpf_prog *p;
>>> +	const struct bpf_func_proto *proto = &krsi_get_env_var_proto;
>>> +	int i;
>>> +
>>> +	while ((p = READ_ONCE(item->prog))) {
>>> +		for (i = 0; i < p->len; i++) {
>>> +			if (bpf_is_call_to_func(&p->insnsi[i], proto->func))
>>> +				need_arg_pages = true;
>>> +		}
>>> +		item++;
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>    struct krsi_hook krsi_hooks_list[] = {
>>> -	#define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
>>> +	#define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \
>>>    		[TYPE] = { \
>>>    			.h_type = TYPE, \
>>>    			.name = #NAME, \
>>> +			.attach_callback = CB, \
>>>    		},
>>>    	#include "hooks.h"
>>>    	#undef KRSI_HOOK_INIT
>>> @@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm)
>>>    		.bprm = bprm,
>>>    	};
>>>    
>>> -	ret = pin_arg_pages(&ctx.bprm_ctx);
>>> -	if (ret < 0)
>>> -		goto out_arg_pages;
>>> +	if (READ_ONCE(need_arg_pages)) {
>>> +		ret = pin_arg_pages(&ctx.bprm_ctx);
>>> +		if (ret < 0)
>>> +			goto out_arg_pages;
>>> +	}
>>>    
>>>    	ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
>>>    	kfree(ctx.bprm_ctx.arg_pages);
>>> @@ -87,7 +139,7 @@ static int krsi_process_execution(struct linux_binprm *bprm)
>>>    }
>>>    
>>>    static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
>>> -	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
>>> +	#define KRSI_HOOK_INIT(T, N, HOOK, IMPL, CB) LSM_HOOK_INIT(HOOK, IMPL),
>>>    	#include "hooks.h"
>>>    	#undef KRSI_HOOK_INIT
>>>    };
>>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
>>> index 1db94dfaac15..2de682371eff 100644
>>> --- a/security/krsi/ops.c
>>> +++ b/security/krsi/ops.c
>>> @@ -139,6 +139,14 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>>>    		goto unlock;
>>>    	}
>>>    
>>> +	if (h->attach_callback) {
>>> +		ret = h->attach_callback(new_array);
>>> +		if (ret < 0) {
>>> +			bpf_prog_array_free(new_array);
>>> +			goto unlock;
>>> +		}
>>> +	}
>>> +
>>>    	rcu_assign_pointer(h->progs, new_array);
>>>    	bpf_prog_array_free(old_array);
>>>    
>>> @@ -278,7 +286,7 @@ BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
>>>    	return get_env_var(ctx, name, dest, n_size, size);
>>>    }
>>>    
>>> -static const struct bpf_func_proto krsi_get_env_var_proto = {
>>> +const struct bpf_func_proto krsi_get_env_var_proto = {
>>>    	.func = krsi_get_env_var,
>>>    	.gpl_only = true,
>>>    	.ret_type = RET_INTEGER,
>>>