[PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI)

[PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI)

[KP Singh]

From: KP Singh <kpsingh@google.com>

# Changes since v1 (https://lore.kernel.org/bpf/20191220154208.15895-1-kpsingh@chromium.org/):

* Eliminate the requirement to maintain LSM hooks separately in
  security/bpf/hooks.h Use BPF trampolines to dynamically allocate
  security hooks
* Drop the use of securityfs as bpftool provides the required
  introspection capabilities.  Update the tests to use the bpf_skeleton
  and global variables
* Use O_CLOEXEC anonymous fds to represent BPF attachment in line with
  the other BPF programs with the possibility to use bpf program pinning
  in the future to provide "permanent attachment".
* Drop the logic based on prog names for handling re-attachment.
* Drop bpf_lsm_event_output from this series and send it as a separate
  patch.

# Motivation

Google does analysis of rich runtime security data to detect and thwart
threats in real-time. Currently, this is done in custom kernel modules
but we would like to replace this with something that's upstream and
useful to others.

The current kernel infrastructure for providing telemetry (Audit, Perf
etc.) is disjoint from access enforcement (i.e. LSMs).  Augmenting the
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 telemetry data requires changes to
various LSMs and their respective policy languages.

This patchset proposes a new stackable and privileged LSM which allows
the LSM hooks to be implemented using eBPF. This facilitates a unified
and dynamic (not requiring re-compilation of the kernel) audit and MAC
policy.

# Why an LSM?

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

Allowing users to implement LSM hooks at runtime also benefits the LSM
eco-system by enabling a quick feedback loop from the security community
about the kind of behaviours that the LSM Framework should be targeting.

# How does it work?

The LSM introduces a new eBPF (https://docs.cilium.io/en/v1.6/bpf/)
program type BPF_PROG_TYPE_LSM which can only be attached to LSM hooks.
Attachment requires CAP_SYS_ADMIN for loading eBPF programs and
CAP_MAC_ADMIN for modifying MAC policies.

The eBPF programs are attached to a separate security_hook_heads
maintained by the BPF LSM for mutable hooks and executed after all the
statically defined hooks (i.e. the ones declared by SELinux, AppArmor,
Smack etc). This also ensures that statically defined LSM hooks retain
the behaviour of "being read-only after init", i.e. __lsm_ro_after_init.

Upon attachment, a security hook is dynamically allocated with
arch_bpf_prepare_trampoline which generates code to handle the
conversion from the signature of the hook to the BPF context and allows
the JIT'ed BPF program to be called as a C function with the same
arguments as the LSM hooks. If any of the attached eBPF programs returns
an error (like ENOPERM), the behaviour represented by the hook is
denied.

Audit logs can be written using a format chosen by the eBPF program to
the perf events buffer or to global eBPF variables or maps and can be
further processed in user-space.

# BTF Based Design

The current design uses BTF
(https://facebookmicrosites.github.io/bpf/blog/2018/11/14/btf-enhancement.html,
https://lwn.net/Articles/803258/) which allows verifiable read-only
structure accesses by field names rather than fixed offsets. This allows
accessing the hook parameters using a dynamically created context which
provides a certain degree of ABI stability:


// Only declare the structure and fields intended to be used
// in the program
struct vm_area_struct {
	unsigned long vm_start;
} __attribute__((preserve_access_index));

// Declare the eBPF program mprotect_audit which attaches to
// to the file_mprotect LSM hook and accepts three arguments.
SEC("lsm/file_mprotect")
int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
	     unsigned long reqprot, unsigned long prot)
{
	unsigned long vm_start = vma->vm_start;

	return 0;
}

By relocating field offsets, BTF makes a large portion of kernel data
structures readily accessible across kernel versions without requiring a
large corpus of BPF helper functions and requiring recompilation with
every kernel version. The BTF type information is also used by the BPF
verifier to validate memory accesses within the BPF program and also
prevents arbitrary writes to the kernel memory.

The limitations of BTF compatibility are described in BPF Co-Re
(http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf, i.e. field
renames, #defines and changes to the signature of LSM hooks).

This design imposes that the MAC policy (eBPF programs) be updated when
the inspected kernel structures change outside of BTF compatibility
guarantees. In practice, this is only required when a structure field
used by a current policy is removed (or renamed) or when the used LSM
hooks change. We expect the maintenance cost of these changes to be
acceptable as compared to the previous design
(https://lore.kernel.org/bpf/20190910115527.5235-1-kpsingh@chromium.org/).

# Why not tracepoints or kprobes?

In order to do MAC with tracepoints or kprobes, we would need to
override the return value of the security hook. This is not possible
with tracepoints or call-site kprobes.

Attaching to the return boundary (kretprobe) implies that BPF programs
would always get called after all the other LSM hooks are called and
clobber the pre-existing LSM semantics.

Enforcing MAC policy with an actual LSM helps leverage the verified
semantics of the framework.

# Usage Examples

A simple example and some documentation is included in the patchset.

In order to better illustrate the capabilities of the framework some
more advanced prototype (not-ready for review) code has also been
published separately:

* Logging execution events (including environment variables and
  arguments)
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_audit_env.c
* Detecting deletion of running executables:
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c
* Detection of writes to /proc/<pid>/mem:

https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_audit_env.c

We have updated Google's internal telemetry infrastructure and have
started deploying this LSM on our Linux Workstations. This gives us more
confidence in the real-world applications of such a system.

KP Singh (10):
  bpf: btf: Make some of the API visible outside BTF
  bpf: lsm: Add a skeleton and config options
  bpf: lsm: Introduce types for eBPF based LSM
  bpf: lsm: Add mutable hooks list for the BPF LSM
  bpf: lsm: BTF API for LSM hooks
  bpf: lsm: Implement attach, detach and execution
  bpf: lsm: Make the allocated callback RO+X
  tools/libbpf: Add support for BPF_PROG_TYPE_LSM
  bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
  bpf: lsm: Add Documentation

 Documentation/security/bpf.rst                | 150 ++++++++
 Documentation/security/index.rst              |   1 +
 MAINTAINERS                                   |  11 +
 include/linux/bpf.h                           |   4 +
 include/linux/bpf_lsm.h                       |  98 +++++
 include/linux/bpf_types.h                     |   4 +
 include/linux/btf.h                           |   8 +
 include/uapi/linux/bpf.h                      |   6 +
 kernel/bpf/btf.c                              |  17 +
 kernel/bpf/syscall.c                          |  51 ++-
 kernel/bpf/verifier.c                         |  74 +++-
 security/Kconfig                              |  11 +-
 security/Makefile                             |   2 +
 security/bpf/Kconfig                          |  25 ++
 security/bpf/Makefile                         |   7 +
 security/bpf/hooks.c                          | 337 ++++++++++++++++++
 security/bpf/include/bpf_lsm.h                |  75 ++++
 security/bpf/lsm.c                            |  92 +++++
 security/bpf/ops.c                            |  30 ++
 security/security.c                           |  24 +-
 tools/include/uapi/linux/bpf.h                |   6 +
 tools/lib/bpf/bpf.c                           |   6 +-
 tools/lib/bpf/bpf.h                           |   1 +
 tools/lib/bpf/libbpf.c                        | 143 +++++++-
 tools/lib/bpf/libbpf.h                        |   4 +
 tools/lib/bpf/libbpf.map                      |   3 +
 tools/lib/bpf/libbpf_probes.c                 |   1 +
 tools/testing/selftests/bpf/lsm_helpers.h     |  19 +
 .../bpf/prog_tests/lsm_mprotect_audit.c       |  58 +++
 .../selftests/bpf/progs/lsm_mprotect_audit.c  |  48 +++
 30 files changed, 1271 insertions(+), 45 deletions(-)
 create mode 100644 Documentation/security/bpf.rst
 create mode 100644 include/linux/bpf_lsm.h
 create mode 100644 security/bpf/Kconfig
 create mode 100644 security/bpf/Makefile
 create mode 100644 security/bpf/hooks.c
 create mode 100644 security/bpf/include/bpf_lsm.h
 create mode 100644 security/bpf/lsm.c
 create mode 100644 security/bpf/ops.c
 create mode 100644 tools/testing/selftests/bpf/lsm_helpers.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
 create mode 100644 tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c

-- 
2.20.1

[PATCH bpf-next v2 01/10] bpf: btf: Make some of the API visible outside BTF

[KP Singh]

From: KP Singh <kpsingh@google.com>

- Add an extern for btf_vmlinux in btf.h
- Add btf_type_by_name_kind, the LSM code does the combination of
  btf_find_by_name_kind and btf_type_by_id quite often.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/btf.h |  8 ++++++++
 kernel/bpf/btf.c    | 17 +++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 881e9b76ef49..dc650d294bc4 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -15,6 +15,7 @@ struct btf_type;
 union bpf_attr;
 
 extern const struct file_operations btf_fops;
+extern struct btf *btf_vmlinux;
 
 void btf_put(struct btf *btf);
 int btf_new_fd(const union bpf_attr *attr);
@@ -66,6 +67,8 @@ const struct btf_type *
 btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		 u32 *type_size, const struct btf_type **elem_type,
 		 u32 *total_nelems);
+const struct btf_type *btf_type_by_name_kind(
+	struct btf *btf, const char *name, u8 kind);
 
 #define for_each_member(i, struct_type, member)			\
 	for (i = 0, member = btf_type_member(struct_type);	\
@@ -142,6 +145,11 @@ static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
 {
 	return NULL;
 }
+static inline const struct btf_type *btf_type_by_name_kind(
+	struct btf *btf, const char *name, u8 kind)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
 static inline const char *btf_name_by_offset(const struct btf *btf,
 					     u32 offset)
 {
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 832b5d7fd892..b8968cec8718 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -436,6 +436,23 @@ const struct btf_type *btf_type_resolve_func_ptr(const struct btf *btf,
 	return NULL;
 }
 
+const struct btf_type *btf_type_by_name_kind(
+	struct btf *btf, const char *name, u8 kind)
+{
+	const struct btf_type *t;
+	s32 type_id;
+
+	type_id = btf_find_by_name_kind(btf, name, kind);
+	if (type_id < 0)
+		return ERR_PTR(-EINVAL);
+
+	t = btf_type_by_id(btf, type_id);
+	if (!t)
+		return ERR_PTR(-EINVAL);
+
+	return t;
+}
+
 /* Types that act only as a source, not sink or intermediate
  * type when resolving.
  */
-- 
2.20.1

[PATCH bpf-next v2 02/10] bpf: lsm: Add a skeleton and config options

[KP Singh]

From: KP Singh <kpsingh@google.com>

The LSM can be enabled by CONFIG_SECURITY_BPF.
Without CONFIG_SECURITY_BPF_ENFORCE, the LSM will run the
attached eBPF programs but not enforce MAC policy based
on the return value of the attached programs.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 MAINTAINERS           |  7 +++++++
 security/Kconfig      | 11 ++++++-----
 security/Makefile     |  2 ++
 security/bpf/Kconfig  | 22 ++++++++++++++++++++++
 security/bpf/Makefile |  5 +++++
 security/bpf/lsm.c    | 25 +++++++++++++++++++++++++
 6 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100644 security/bpf/Kconfig
 create mode 100644 security/bpf/Makefile
 create mode 100644 security/bpf/lsm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 66a2e5e07117..0941f478cfa5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3203,6 +3203,13 @@ S:	Supported
 F:	arch/x86/net/
 X:	arch/x86/net/bpf_jit_comp32.c
 
+BPF SECURITY MODULE
+M:	KP Singh <kpsingh@chromium.org>
+L:	linux-security-module@vger.kernel.org
+L:	bpf@vger.kernel.org
+S:	Maintained
+F:	security/bpf/
+
 BROADCOM B44 10/100 ETHERNET DRIVER
 M:	Michael Chan <michael.chan@broadcom.com>
 L:	netdev@vger.kernel.org
diff --git a/security/Kconfig b/security/Kconfig
index 2a1a2d396228..6f1aab195e7d 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/bpf/Kconfig"
 source "security/safesetid/Kconfig"
 source "security/lockdown/Kconfig"
 
@@ -277,11 +278,11 @@ endchoice
 
 config LSM
 	string "Ordered list of enabled LSMs"
-	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
-	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
-	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
-	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
-	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
+	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
+	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
+	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
+	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
+	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
 	help
 	  A comma-separated list of LSMs, in initialization order.
 	  Any LSMs left off this list will be ignored. This can be
diff --git a/security/Makefile b/security/Makefile
index be1dd9d2cb2f..50e6821dd7b7 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA)		+= yama
 subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
 subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
 subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
+subdir-$(CONFIG_SECURITY_BPF)		+= bpf
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -29,6 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA)		+= yama/
 obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
 obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
 obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
+obj-$(CONFIG_SECURITY_BPF)		+= bpf/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 
 # Object integrity file lists
diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
new file mode 100644
index 000000000000..a5f6c67ae526
--- /dev/null
+++ b/security/bpf/Kconfig
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright 2019 Google LLC.
+
+config SECURITY_BPF
+	bool "BPF-based MAC and audit policy"
+	depends on SECURITY
+	depends on BPF_SYSCALL
+	help
+	  This enables instrumentation of the security hooks with
+	  eBPF programs.
+
+	  If you are unsure how to answer this question, answer N.
+
+config SECURITY_BPF_ENFORCE
+	bool "Deny operations based on the evaluation of the attached programs"
+	depends on SECURITY_BPF
+	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 or deny the operation
+	  guarded by the security hook.
diff --git a/security/bpf/Makefile b/security/bpf/Makefile
new file mode 100644
index 000000000000..26a0ab6f99b7
--- /dev/null
+++ b/security/bpf/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright 2019 Google LLC.
+
+obj-$(CONFIG_SECURITY_BPF) := lsm.o
diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
new file mode 100644
index 000000000000..5c5c14f990ce
--- /dev/null
+++ b/security/bpf/lsm.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#include <linux/lsm_hooks.h>
+
+/* This is only for internal hooks, always statically shipped as part of the
+ * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
+ * which is common for LSMs and R/O after init.
+ */
+static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
+
+static int __init lsm_init(void)
+{
+	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
+	pr_info("eBPF and LSM are friends now.\n");
+	return 0;
+}
+
+DEFINE_LSM(bpf) = {
+	.name = "bpf",
+	.init = lsm_init,
+};
-- 
2.20.1

[PATCH bpf-next v2 03/10] bpf: lsm: Introduce types for eBPF based LSM

[KP Singh]

From: KP Singh <kpsingh@google.com>

A new eBPF program type BPF_PROG_TYPE_LSM with an
expected attach type of BPF_LSM_MAC. Attachment to LSM hooks is not
implemented in this patch.

On defining the types for the program, the macros expect that
<prog_name>_prog_ops and <prog_name>_verifier_ops exist. This is
implicitly required by the macro:

  BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm, ...)

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/bpf_types.h      |  4 ++++
 include/uapi/linux/bpf.h       |  2 ++
 kernel/bpf/syscall.c           |  6 ++++++
 security/bpf/Makefile          |  2 +-
 security/bpf/ops.c             | 28 ++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h |  2 ++
 tools/lib/bpf/libbpf_probes.c  |  1 +
 7 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 security/bpf/ops.c

diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 9f326e6ef885..2f5b054500d7 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -68,6 +68,10 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_SK_REUSEPORT, sk_reuseport,
 #if defined(CONFIG_BPF_JIT)
 BPF_PROG_TYPE(BPF_PROG_TYPE_STRUCT_OPS, bpf_struct_ops,
 	      void *, void *)
+#ifdef CONFIG_SECURITY_BPF
+BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
+	       void *, void *)
+#endif /* CONFIG_SECURITY_BPF */
 #endif
 
 BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 52966e758fe5..b6a725a8a21d 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -176,6 +176,7 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_CGROUP_SOCKOPT,
 	BPF_PROG_TYPE_TRACING,
 	BPF_PROG_TYPE_STRUCT_OPS,
+	BPF_PROG_TYPE_LSM,
 };
 
 enum bpf_attach_type {
@@ -205,6 +206,7 @@ enum bpf_attach_type {
 	BPF_TRACE_RAW_TP,
 	BPF_TRACE_FENTRY,
 	BPF_TRACE_FEXIT,
+	BPF_LSM_MAC,
 	__MAX_BPF_ATTACH_TYPE
 };
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f9db72a96ec0..2945739618c9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2151,6 +2151,9 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 	case BPF_LIRC_MODE2:
 		ptype = BPF_PROG_TYPE_LIRC_MODE2;
 		break;
+	case BPF_LSM_MAC:
+		ptype = BPF_PROG_TYPE_LSM;
+		break;
 	case BPF_FLOW_DISSECTOR:
 		ptype = BPF_PROG_TYPE_FLOW_DISSECTOR;
 		break;
@@ -2182,6 +2185,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_LSM:
+		ret = -EINVAL;
+		break;
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 		ret = skb_flow_dissector_bpf_prog_attach(attr, prog);
 		break;
diff --git a/security/bpf/Makefile b/security/bpf/Makefile
index 26a0ab6f99b7..c78a8a056e7e 100644
--- a/security/bpf/Makefile
+++ b/security/bpf/Makefile
@@ -2,4 +2,4 @@
 #
 # Copyright 2019 Google LLC.
 
-obj-$(CONFIG_SECURITY_BPF) := lsm.o
+obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o
diff --git a/security/bpf/ops.c b/security/bpf/ops.c
new file mode 100644
index 000000000000..81c2bd9c0495
--- /dev/null
+++ b/security/bpf/ops.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#include <linux/filter.h>
+#include <linux/bpf.h>
+
+const struct bpf_prog_ops lsm_prog_ops = {
+};
+
+static const struct bpf_func_proto *get_bpf_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 lsm_verifier_ops = {
+	.get_func_proto = get_bpf_func_proto,
+};
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 52966e758fe5..b6a725a8a21d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -176,6 +176,7 @@ enum bpf_prog_type {
 	BPF_PROG_TYPE_CGROUP_SOCKOPT,
 	BPF_PROG_TYPE_TRACING,
 	BPF_PROG_TYPE_STRUCT_OPS,
+	BPF_PROG_TYPE_LSM,
 };
 
 enum bpf_attach_type {
@@ -205,6 +206,7 @@ enum bpf_attach_type {
 	BPF_TRACE_RAW_TP,
 	BPF_TRACE_FENTRY,
 	BPF_TRACE_FEXIT,
+	BPF_LSM_MAC,
 	__MAX_BPF_ATTACH_TYPE
 };
 
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index 8cc992bc532a..2314889369cc 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -107,6 +107,7 @@ probe_load(enum bpf_prog_type prog_type, const struct bpf_insn *insns,
 	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
 	case BPF_PROG_TYPE_TRACING:
 	case BPF_PROG_TYPE_STRUCT_OPS:
+	case BPF_PROG_TYPE_LSM:
 	default:
 		break;
 	}
-- 
2.20.1

[PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[KP Singh]

From: KP Singh <kpsingh@google.com>

- The list of hooks registered by an LSM is currently immutable as they
  are declared with __lsm_ro_after_init and they are attached to a
  security_hook_heads struct.
- For the BPF LSM we need to de/register the hooks at runtime. Making
  the existing security_hook_heads mutable broadens an
  attack vector, so a separate security_hook_heads is added for only
  those that ~must~ be mutable.
- These mutable hooks are run only after all the static hooks have
  successfully executed.

This is based on the ideas discussed in:

  https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal

Signed-off-by: KP Singh <kpsingh@google.com>
---
 MAINTAINERS             |  1 +
 include/linux/bpf_lsm.h | 71 +++++++++++++++++++++++++++++++++++++++++
 security/bpf/Kconfig    |  1 +
 security/bpf/Makefile   |  2 +-
 security/bpf/hooks.c    | 20 ++++++++++++
 security/bpf/lsm.c      |  9 +++++-
 security/security.c     | 24 +++++++-------
 7 files changed, 115 insertions(+), 13 deletions(-)
 create mode 100644 include/linux/bpf_lsm.h
 create mode 100644 security/bpf/hooks.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0941f478cfa5..02d7e05e9b75 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3209,6 +3209,7 @@ L:	linux-security-module@vger.kernel.org
 L:	bpf@vger.kernel.org
 S:	Maintained
 F:	security/bpf/
+F:	include/linux/bpf_lsm.h
 
 BROADCOM B44 10/100 ETHERNET DRIVER
 M:	Michael Chan <michael.chan@broadcom.com>
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
new file mode 100644
index 000000000000..9883cf25241c
--- /dev/null
+++ b/include/linux/bpf_lsm.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#ifndef _LINUX_BPF_LSM_H
+#define _LINUX_BPF_LSM_H
+
+#include <linux/bpf.h>
+#include <linux/lsm_hooks.h>
+
+#ifdef CONFIG_SECURITY_BPF
+
+/* Mutable hooks defined at runtime and executed after all the statically
+ * define LSM hooks.
+ */
+extern struct security_hook_heads bpf_lsm_hook_heads;
+
+int bpf_lsm_srcu_read_lock(void);
+void bpf_lsm_srcu_read_unlock(int idx);
+
+#define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)			\
+	do {							\
+		struct security_hook_list *P;			\
+		int _idx;					\
+								\
+		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
+			break;					\
+								\
+		_idx = bpf_lsm_srcu_read_lock();		\
+		hlist_for_each_entry(P, &bpf_lsm_hook_heads.FUNC, list) \
+			P->hook.FUNC(__VA_ARGS__);		\
+		bpf_lsm_srcu_read_unlock(_idx);			\
+	} while (0)
+
+#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) ({		\
+	do {							\
+		struct security_hook_list *P;			\
+		int _idx;					\
+								\
+		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
+			break;					\
+								\
+		_idx = bpf_lsm_srcu_read_lock();		\
+								\
+		hlist_for_each_entry(P,				\
+			&bpf_lsm_hook_heads.FUNC, list) {	\
+			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (RC && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
+				break;				\
+		}						\
+		bpf_lsm_srcu_read_unlock(_idx);			\
+	} while (0);						\
+	IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? RC : 0;	\
+})
+
+#else /* !CONFIG_SECURITY_BPF */
+
+#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) (RC)
+#define CALL_BPF_LSM_VOID_HOOKS(...)
+
+static inline int bpf_lsm_srcu_read_lock(void)
+{
+	return 0;
+}
+static inline void bpf_lsm_srcu_read_unlock(int idx) {}
+
+#endif /* CONFIG_SECURITY_BPF */
+
+#endif /* _LINUX_BPF_LSM_H */
diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
index a5f6c67ae526..595e4ad597ae 100644
--- a/security/bpf/Kconfig
+++ b/security/bpf/Kconfig
@@ -6,6 +6,7 @@ config SECURITY_BPF
 	bool "BPF-based MAC and audit policy"
 	depends on SECURITY
 	depends on BPF_SYSCALL
+	depends on SRCU
 	help
 	  This enables instrumentation of the security hooks with
 	  eBPF programs.
diff --git a/security/bpf/Makefile b/security/bpf/Makefile
index c78a8a056e7e..c526927c337d 100644
--- a/security/bpf/Makefile
+++ b/security/bpf/Makefile
@@ -2,4 +2,4 @@
 #
 # Copyright 2019 Google LLC.
 
-obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o
+obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
new file mode 100644
index 000000000000..b123d9cb4cd4
--- /dev/null
+++ b/security/bpf/hooks.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#include <linux/bpf_lsm.h>
+#include <linux/srcu.h>
+
+DEFINE_STATIC_SRCU(security_hook_srcu);
+
+int bpf_lsm_srcu_read_lock(void)
+{
+	return srcu_read_lock(&security_hook_srcu);
+}
+
+void bpf_lsm_srcu_read_unlock(int idx)
+{
+	return srcu_read_unlock(&security_hook_srcu, idx);
+}
diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
index 5c5c14f990ce..d4ea6aa9ddb8 100644
--- a/security/bpf/lsm.c
+++ b/security/bpf/lsm.c
@@ -4,14 +4,21 @@
  * Copyright 2019 Google LLC.
  */
 
+#include <linux/bpf_lsm.h>
 #include <linux/lsm_hooks.h>
 
 /* This is only for internal hooks, always statically shipped as part of the
- * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
+ * BPF LSM. Statically defined hooks are appended to the security_hook_heads
  * which is common for LSMs and R/O after init.
  */
 static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
 
+/* Security hooks registered dynamically by the BPF LSM and must be accessed
+ * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock. The mutable
+ * hooks dynamically allocated by the BPF LSM are appeneded here.
+ */
+struct security_hook_heads bpf_lsm_hook_heads;
+
 static int __init lsm_init(void)
 {
 	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
diff --git a/security/security.c b/security/security.c
index cd2d18d2d279..4a2eb4c089b2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -27,6 +27,7 @@
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <linux/msg.h>
+#include <linux/bpf_lsm.h>
 #include <net/flow.h>
 
 #define MAX_LSM_EVM_XATTR	2
@@ -652,20 +653,21 @@ static void __init lsm_early_task(struct task_struct *task)
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
 			P->hook.FUNC(__VA_ARGS__);		\
+		CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__);	\
 	} while (0)
 
-#define call_int_hook(FUNC, IRC, ...) ({			\
-	int RC = IRC;						\
-	do {							\
-		struct security_hook_list *P;			\
-								\
+#define call_int_hook(FUNC, IRC, ...) ({				\
+	int RC = IRC;							\
+	do {								\
+		struct security_hook_list *P;				\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
-			RC = P->hook.FUNC(__VA_ARGS__);		\
-			if (RC != 0)				\
-				break;				\
-		}						\
-	} while (0);						\
-	RC;							\
+			RC = P->hook.FUNC(__VA_ARGS__);			\
+			if (RC != 0)					\
+				break;					\
+		}							\
+		RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__);	\
+	} while (0);							\
+	RC;								\
 })
 
 /* Security operations */
-- 
2.20.1

[PATCH bpf-next v2 05/10] bpf: lsm: BTF API for LSM hooks

[KP Singh]

From: KP Singh <kpsingh@google.com>

The BTF API provides information required by the BPF verifier to
attach eBPF programs to the LSM hooks by using the BTF information of
two types:

- struct security_hook_heads: This type provides the offset which
  a new dynamically allocated security hook must be attached to.
- union security_list_options: This provides the information about the
  function prototype required by the hook.

When the program is loaded:

- The verifier receives the index of a member in struct
  security_hook_heads to which a program must be attached as
  prog->aux->lsm_hook_index. The index is one-based for better
  verification.
- bpf_lsm_type_by_index is used to determine the func_proto of
  the LSM hook and updates prog->aux->attach_func_proto
- bpf_lsm_head_by_index is used to determine the hlist_head to which
  the BPF program must be attached.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/bpf_lsm.h |  12 +++++
 security/bpf/Kconfig    |   1 +
 security/bpf/hooks.c    | 104 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 9883cf25241c..a9b4f7b41c65 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -19,6 +19,8 @@ extern struct security_hook_heads bpf_lsm_hook_heads;
 
 int bpf_lsm_srcu_read_lock(void);
 void bpf_lsm_srcu_read_unlock(int idx);
+const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 offset);
+const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 id);
 
 #define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)			\
 	do {							\
@@ -65,6 +67,16 @@ static inline int bpf_lsm_srcu_read_lock(void)
 	return 0;
 }
 static inline void bpf_lsm_srcu_read_unlock(int idx) {}
+static inline const struct btf_type *bpf_lsm_type_by_index(
+	struct btf *btf, u32 index)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+static inline const struct btf_member *bpf_lsm_head_by_index(
+	struct btf *btf, u32 id)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
 
 #endif /* CONFIG_SECURITY_BPF */
 
diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
index 595e4ad597ae..9438d899b618 100644
--- a/security/bpf/Kconfig
+++ b/security/bpf/Kconfig
@@ -7,6 +7,7 @@ config SECURITY_BPF
 	depends on SECURITY
 	depends on BPF_SYSCALL
 	depends on SRCU
+	depends on DEBUG_INFO_BTF
 	help
 	  This enables instrumentation of the security hooks with
 	  eBPF programs.
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index b123d9cb4cd4..82725611693d 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -5,6 +5,8 @@
  */
 
 #include <linux/bpf_lsm.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
 #include <linux/srcu.h>
 
 DEFINE_STATIC_SRCU(security_hook_srcu);
@@ -18,3 +20,105 @@ void bpf_lsm_srcu_read_unlock(int idx)
 {
 	return srcu_read_unlock(&security_hook_srcu, idx);
 }
+
+static inline int validate_hlist_head(struct btf *btf, u32 type_id)
+{
+	s32 hlist_id;
+
+	hlist_id = btf_find_by_name_kind(btf, "hlist_head", BTF_KIND_STRUCT);
+	if (hlist_id < 0 || hlist_id != type_id)
+		return -EINVAL;
+
+	return 0;
+}
+
+/* Find the BTF representation of the security_hook_heads member for a member
+ * with a given index in struct security_hook_heads.
+ */
+const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 index)
+{
+	const struct btf_member *member;
+	const struct btf_type *t;
+	u32 off, i;
+	int ret;
+
+	t = btf_type_by_name_kind(btf, "security_hook_heads", BTF_KIND_STRUCT);
+	if (WARN_ON_ONCE(IS_ERR(t)))
+		return ERR_CAST(t);
+
+	for_each_member(i, t, member) {
+		/* We've found the id requested and need to check the
+		 * the following:
+		 *
+		 * - Is it at a valid alignment for struct hlist_head?
+		 *
+		 * - Is it a valid hlist_head struct?
+		 */
+		if (index == i) {
+			off = btf_member_bit_offset(t, member);
+			if (off % 8)
+				/* valid c code cannot generate such btf */
+				return ERR_PTR(-EINVAL);
+			off /= 8;
+
+			if (off % __alignof__(struct hlist_head))
+				return ERR_PTR(-EINVAL);
+
+			ret = validate_hlist_head(btf, member->type);
+			if (ret < 0)
+				return ERR_PTR(ret);
+
+			return member;
+		}
+	}
+
+	return ERR_PTR(-ENOENT);
+}
+
+/* Given an index of a member in security_hook_heads return the
+ * corresponding type for the LSM hook. The members of the union
+ * security_list_options have the same name as the security_hook_heads which
+ * is ensured by the LSM_HOOK_INIT macro defined in include/linux/lsm_hooks.h
+ */
+const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 index)
+{
+	const struct btf_member *member, *hook_head = NULL;
+	const struct btf_type *t, *hook_type = NULL;
+	u32 i;
+
+	hook_head = bpf_lsm_head_by_index(btf, index);
+	if (IS_ERR(hook_head))
+		return ERR_PTR(PTR_ERR(hook_head));
+
+	t = btf_type_by_name_kind(btf, "security_list_options", BTF_KIND_UNION);
+	if (WARN_ON_ONCE(IS_ERR(t)))
+		return ERR_CAST(t);
+
+	for_each_member(i, t, member) {
+		if (hook_head->name_off == member->name_off) {
+			/* There should be only one member with the same name
+			 * as the LSM hook. This should never really happen
+			 * and either indicates malformed BTF or someone trying
+			 * trick the LSM.
+			 */
+			if (WARN_ON(hook_type))
+				return ERR_PTR(-EINVAL);
+
+			hook_type = btf_type_by_id(btf, member->type);
+			if (unlikely(!hook_type))
+				return ERR_PTR(-EINVAL);
+
+			if (!btf_type_is_ptr(hook_type))
+				return ERR_PTR(-EINVAL);
+		}
+	}
+
+	if (!hook_type)
+		return ERR_PTR(-ENOENT);
+
+	t = btf_type_by_id(btf, hook_type->type);
+	if (unlikely(!t))
+		return ERR_PTR(-EINVAL);
+
+	return t;
+}
-- 
2.20.1

[PATCH bpf-next v2 06/10] bpf: lsm: Implement attach, detach and execution

[KP Singh]

From: KP Singh <kpsingh@google.com>

JITed BPF programs are used by the BPF LSM as dynamically allocated
security hooks. arch_bpf_prepare_trampoline handles the
arch_bpf_prepare_trampoline generates code to handle conversion of the
signature of the hook to the BPF context and allows the BPF program to
be called directly as a C function.

The following permissions are required to attach a program to a hook:

- CAP_SYS_ADMIN to load the program
- CAP_MAC_ADMIN to attach it (i.e. to update the security policy)

When the program is loaded (BPF_PROG_LOAD), the verifier receives the
index (one-based) of the member in the security_hook_heads struct in the
prog->aux->lsm_hook_index and uses the BTF API provided by the LSM to:

- Populate the name of the hook in prog->aux->attach_func_name and
  the prototype in prog->aux->attach_func_proto.
- Verify if the offset is valid for a type struct hlist_head.
- The program is verified for accesses based on the attach_func_proto
  similar to raw_tp BPF programs.

When an attachment (BPF_PROG_ATTACH) is requested:

- The information required to set-up of a callback is populated in the
  struct bpf_lsm_list.
- A new callback and a bpf_lsm_hook is allocated and the address of
  the hook is set to the be the address of the allocated callback.
- The attachment returns an anonymous O_CLOEXEC fd which detaches the
  program on close.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 include/linux/bpf.h            |   4 +
 include/linux/bpf_lsm.h        |  15 +++
 include/uapi/linux/bpf.h       |   4 +
 kernel/bpf/syscall.c           |  47 ++++++-
 kernel/bpf/verifier.c          |  74 +++++++++--
 security/bpf/Kconfig           |   1 +
 security/bpf/Makefile          |   2 +
 security/bpf/hooks.c           | 230 +++++++++++++++++++++++++++++++--
 security/bpf/include/bpf_lsm.h |  75 +++++++++++
 security/bpf/lsm.c             |  60 +++++++++
 security/bpf/ops.c             |   2 +
 tools/include/uapi/linux/bpf.h |   4 +
 12 files changed, 494 insertions(+), 24 deletions(-)
 create mode 100644 security/bpf/include/bpf_lsm.h

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index aed2bc39d72b..5ed4780c2091 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -599,6 +599,10 @@ struct bpf_prog_aux {
 	u32 func_cnt; /* used by non-func prog as the number of func progs */
 	u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */
 	u32 attach_btf_id; /* in-kernel BTF type id to attach to */
+	/* Index (one-based) of the hlist_head in security_hook_heads to which
+	 * the program must be attached.
+	 */
+	u32 lsm_hook_index;
 	struct bpf_prog *linked_prog;
 	bool verifier_zext; /* Zero extensions has been inserted by verifier. */
 	bool offload_requested;
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index a9b4f7b41c65..8c86672437f0 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -19,8 +19,11 @@ extern struct security_hook_heads bpf_lsm_hook_heads;
 
 int bpf_lsm_srcu_read_lock(void);
 void bpf_lsm_srcu_read_unlock(int idx);
+int bpf_lsm_verify_prog(const struct bpf_prog *prog);
 const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 offset);
 const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 id);
+int bpf_lsm_attach(struct bpf_prog *prog);
+int bpf_lsm_detach(struct bpf_prog *prog);
 
 #define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)			\
 	do {							\
@@ -67,6 +70,10 @@ static inline int bpf_lsm_srcu_read_lock(void)
 	return 0;
 }
 static inline void bpf_lsm_srcu_read_unlock(int idx) {}
+static inline int bpf_lsm_verify_prog(const struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
 static inline const struct btf_type *bpf_lsm_type_by_index(
 	struct btf *btf, u32 index)
 {
@@ -77,6 +84,14 @@ static inline const struct btf_member *bpf_lsm_head_by_index(
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
+static inline int bpf_lsm_attach(struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
+static inline int bpf_lsm_detach(struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
 
 #endif /* CONFIG_SECURITY_BPF */
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b6a725a8a21d..fa5513cb3332 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -448,6 +448,10 @@ union bpf_attr {
 		__u32		line_info_cnt;	/* number of bpf_line_info records */
 		__u32		attach_btf_id;	/* in-kernel BTF type id to attach to */
 		__u32		attach_prog_fd; /* 0 to attach to vmlinux */
+		/* Index (one-based) of the hlist_head in security_hook_heads to
+		 * which the program must be attached.
+		 */
+		__u32		lsm_hook_index;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 2945739618c9..a54eb1032fdd 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/bpf_lsm.h>
 #include <linux/btf.h>
 #include <linux/syscalls.h>
 #include <linux/slab.h>
@@ -1751,7 +1752,7 @@ bpf_prog_load_check_attach(enum bpf_prog_type prog_type,
 }
 
 /* last field in 'union bpf_attr' used by this command */
-#define	BPF_PROG_LOAD_LAST_FIELD attach_prog_fd
+#define	BPF_PROG_LOAD_LAST_FIELD lsm_hook_index
 
 static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 {
@@ -1805,6 +1806,10 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 
 	prog->expected_attach_type = attr->expected_attach_type;
 	prog->aux->attach_btf_id = attr->attach_btf_id;
+
+	if (type == BPF_PROG_TYPE_LSM)
+		prog->aux->lsm_hook_index = attr->lsm_hook_index;
+
 	if (attr->attach_prog_fd) {
 		struct bpf_prog *tgt_prog;
 
@@ -1970,6 +1975,44 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
 	return err;
 }
 
+static int bpf_lsm_prog_release(struct inode *inode, struct file *filp)
+{
+	struct bpf_prog *prog = filp->private_data;
+
+	WARN_ON_ONCE(bpf_lsm_detach(prog));
+	bpf_prog_put(prog);
+	return 0;
+}
+
+static const struct file_operations bpf_lsm_prog_fops = {
+	.release	= bpf_lsm_prog_release,
+	.read		= bpf_dummy_read,
+	.write		= bpf_dummy_write,
+};
+
+static int bpf_lsm_prog_attach(struct bpf_prog *prog)
+{
+	int ret;
+
+	if (prog->expected_attach_type != BPF_LSM_MAC)
+		return -EINVAL;
+
+	/* The attach increments the references to the program which is
+	 * decremented on detach as a part of bpf_lsm_hook_free.
+	 */
+	ret = bpf_lsm_attach(prog);
+	if (ret)
+		return ret;
+
+	ret = anon_inode_getfd("bpf-lsm-prog", &bpf_lsm_prog_fops,
+				prog, O_CLOEXEC);
+	if (ret < 0) {
+		bpf_lsm_detach(prog);
+		return ret;
+	}
+	return 0;
+}
+
 struct bpf_raw_tracepoint {
 	struct bpf_raw_event_map *btp;
 	struct bpf_prog *prog;
@@ -2186,7 +2229,7 @@ static int bpf_prog_attach(const union bpf_attr *attr)
 		ret = lirc_prog_attach(attr, prog);
 		break;
 	case BPF_PROG_TYPE_LSM:
-		ret = -EINVAL;
+		ret = bpf_lsm_prog_attach(prog);
 		break;
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 		ret = skb_flow_dissector_bpf_prog_attach(attr, prog);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ca17dccc17ba..10f44c3b15ec 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19,6 +19,7 @@
 #include <linux/sort.h>
 #include <linux/perf_event.h>
 #include <linux/ctype.h>
+#include <linux/bpf_lsm.h>
 
 #include "disasm.h"
 
@@ -6393,8 +6394,9 @@ static int check_return_code(struct bpf_verifier_env *env)
 	struct tnum range = tnum_range(0, 1);
 	int err;
 
-	/* The struct_ops func-ptr's return type could be "void" */
-	if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS &&
+	/* LSM and struct_ops func-ptr's return type could be "void" */
+	if ((env->prog->type == BPF_PROG_TYPE_STRUCT_OPS ||
+	     env->prog->type == BPF_PROG_TYPE_LSM) &&
 	    !prog->aux->attach_func_proto->type)
 		return 0;
 
@@ -9734,7 +9736,51 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	return 0;
 }
 
-static int check_attach_btf_id(struct bpf_verifier_env *env)
+/* BPF_PROG_TYPE_LSM programs pass the member index of the LSM hook in the
+ * security_hook_heads as the lsm_hook_index. The verifier determines the
+ * name and the prototype for the LSM hook using the information in
+ * security_list_options, validates if the offset is a valid hlist_head, and
+ * updates the attach_btf_id to the byte offset in the security_hook_heads
+ * struct.
+ */
+static inline int check_attach_btf_id_lsm(struct bpf_verifier_env *env)
+{
+	struct bpf_prog *prog = env->prog;
+	u32 index = prog->aux->lsm_hook_index;
+	const struct btf_member *head;
+	const struct btf_type *t;
+	const char *tname;
+	int ret;
+
+	ret = bpf_lsm_verify_prog(prog);
+	if (ret < 0)
+		return -EINVAL;
+
+	t = bpf_lsm_type_by_index(btf_vmlinux, index);
+	if (!t) {
+		verbose(env, "unable to find security_list_option for index %u in security_hook_heads\n", index);
+		return -EINVAL;
+	}
+
+	if (!btf_type_is_func_proto(t))
+		return -EINVAL;
+
+	head = bpf_lsm_head_by_index(btf_vmlinux, index);
+	if (IS_ERR(head)) {
+		verbose(env, "no security_hook_heads index = %u\n", index);
+		return PTR_ERR(head);
+	}
+
+	tname = btf_name_by_offset(btf_vmlinux, head->name_off);
+	if (!tname || !tname[0])
+		return -EINVAL;
+
+	prog->aux->attach_func_name = tname;
+	prog->aux->attach_func_proto = t;
+	return 0;
+}
+
+static int check_attach_btf_id_tracing(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog;
 	struct bpf_prog *tgt_prog = prog->aux->linked_prog;
@@ -9749,12 +9795,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	long addr;
 	u64 key;
 
-	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
-		return check_struct_ops_btf_id(env);
-
-	if (prog->type != BPF_PROG_TYPE_TRACING)
-		return 0;
-
 	if (!btf_id) {
 		verbose(env, "Tracing programs must provide btf_id\n");
 		return -EINVAL;
@@ -9895,6 +9935,22 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
 	}
 }
 
+static int check_attach_btf_id(struct bpf_verifier_env *env)
+{
+	struct bpf_prog *prog = env->prog;
+
+	switch (prog->type) {
+	case BPF_PROG_TYPE_TRACING:
+		return check_attach_btf_id_tracing(env);
+	case BPF_PROG_TYPE_STRUCT_OPS:
+		return check_struct_ops_btf_id(env);
+	case BPF_PROG_TYPE_LSM:
+		return check_attach_btf_id_lsm(env);
+	default:
+		return 0;
+	}
+}
+
 int bpf_check(struct bpf_prog **prog, union bpf_attr *attr,
 	      union bpf_attr __user *uattr)
 {
diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
index 9438d899b618..a915c549f4b8 100644
--- a/security/bpf/Kconfig
+++ b/security/bpf/Kconfig
@@ -8,6 +8,7 @@ config SECURITY_BPF
 	depends on BPF_SYSCALL
 	depends on SRCU
 	depends on DEBUG_INFO_BTF
+	depends on BPF_JIT && HAVE_EBPF_JIT
 	help
 	  This enables instrumentation of the security hooks with
 	  eBPF programs.
diff --git a/security/bpf/Makefile b/security/bpf/Makefile
index c526927c337d..748b9b7d4bc7 100644
--- a/security/bpf/Makefile
+++ b/security/bpf/Makefile
@@ -3,3 +3,5 @@
 # Copyright 2019 Google LLC.
 
 obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o
+
+ccflags-y := -I$(srctree)/security/bpf -I$(srctree)/security/bpf/include
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 82725611693d..4e71da0e8e9e 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -6,9 +6,14 @@
 
 #include <linux/bpf_lsm.h>
 #include <linux/bpf.h>
+#include <linux/bpf_verifier.h>
 #include <linux/btf.h>
 #include <linux/srcu.h>
 
+#include "bpf_lsm.h"
+
+#define SECURITY_LIST_HEAD(off) ((void *)&bpf_lsm_hook_heads + off)
+
 DEFINE_STATIC_SRCU(security_hook_srcu);
 
 int bpf_lsm_srcu_read_lock(void)
@@ -32,21 +37,36 @@ static inline int validate_hlist_head(struct btf *btf, u32 type_id)
 	return 0;
 }
 
+int bpf_lsm_verify_prog(const struct bpf_prog *prog)
+{
+	u32 index = prog->aux->lsm_hook_index;
+	struct bpf_verifier_log log = {};
+
+	if (!prog->gpl_compatible) {
+		bpf_log(&log,
+			"LSM programs must have a GPL compatible license\n");
+		return -EINVAL;
+	}
+
+	if (index < 1 || index > bpf_lsm_info.num_hooks) {
+		bpf_log(&log, "lsm_hook_index should be between 1 and %lu\n",
+			bpf_lsm_info.num_hooks);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* Find the BTF representation of the security_hook_heads member for a member
  * with a given index in struct security_hook_heads.
  */
 const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 index)
 {
 	const struct btf_member *member;
-	const struct btf_type *t;
 	u32 off, i;
 	int ret;
 
-	t = btf_type_by_name_kind(btf, "security_hook_heads", BTF_KIND_STRUCT);
-	if (WARN_ON_ONCE(IS_ERR(t)))
-		return ERR_CAST(t);
-
-	for_each_member(i, t, member) {
+	for_each_member(i, bpf_lsm_info.btf_hook_heads, member) {
 		/* We've found the id requested and need to check the
 		 * the following:
 		 *
@@ -54,8 +74,9 @@ const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 index)
 		 *
 		 * - Is it a valid hlist_head struct?
 		 */
-		if (index == i) {
-			off = btf_member_bit_offset(t, member);
+		if (index == i + 1) {
+			off = btf_member_bit_offset(
+				bpf_lsm_info.btf_hook_heads, member);
 			if (off % 8)
 				/* valid c code cannot generate such btf */
 				return ERR_PTR(-EINVAL);
@@ -90,11 +111,7 @@ const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 index)
 	if (IS_ERR(hook_head))
 		return ERR_PTR(PTR_ERR(hook_head));
 
-	t = btf_type_by_name_kind(btf, "security_list_options", BTF_KIND_UNION);
-	if (WARN_ON_ONCE(IS_ERR(t)))
-		return ERR_CAST(t);
-
-	for_each_member(i, t, member) {
+	for_each_member(i, bpf_lsm_info.btf_hook_types, member) {
 		if (hook_head->name_off == member->name_off) {
 			/* There should be only one member with the same name
 			 * as the LSM hook. This should never really happen
@@ -122,3 +139,190 @@ const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 index)
 
 	return t;
 }
+
+static void *bpf_lsm_get_func_addr(struct security_hook_list *s,
+				   const char *name)
+{
+	const struct btf_member *member;
+	void *addr = NULL;
+	s32 i;
+
+	for_each_member(i, bpf_lsm_info.btf_hook_types, member) {
+		if (!strncmp(btf_name_by_offset(btf_vmlinux, member->name_off),
+				name, strlen(name) + 1)) {
+			/* There should be only one member with the same name
+			 * as the LSM hook.
+			 */
+			if (WARN_ON(addr))
+				return ERR_PTR(-EINVAL);
+			addr = (void *)&s->hook + member->offset / 8;
+		}
+	}
+
+	if (!addr)
+		return ERR_PTR(-ENOENT);
+	return addr;
+}
+
+static struct bpf_lsm_list *bpf_lsm_list_lookup(struct bpf_prog *prog)
+{
+	u32 index = prog->aux->lsm_hook_index;
+	struct bpf_verifier_log bpf_log = {};
+	const struct btf_member *head;
+	struct bpf_lsm_list *list;
+	int ret = 0;
+
+	list = &bpf_lsm_info.hook_lists[index - 1];
+
+	mutex_lock(&list->mutex);
+
+	if (list->initialized)
+		goto unlock;
+
+	list->attach_type = prog->aux->attach_func_proto;
+
+	ret = btf_distill_func_proto(&bpf_log, btf_vmlinux, list->attach_type,
+				     prog->aux->attach_func_name,
+				     &list->func_model);
+	if (ret)
+		goto unlock;
+
+	head = bpf_lsm_head_by_index(btf_vmlinux, index);
+	if (IS_ERR(head)) {
+		ret = PTR_ERR(head);
+		goto unlock;
+	}
+
+	list->security_list_head = SECURITY_LIST_HEAD(head->offset / 8);
+	list->initialized = true;
+unlock:
+	mutex_unlock(&list->mutex);
+	if (ret)
+		return ERR_PTR(ret);
+	return list;
+}
+
+static struct bpf_lsm_hook *bpf_lsm_hook_alloc(
+	struct bpf_lsm_list *list, struct bpf_prog *prog)
+{
+	struct bpf_lsm_hook *hook;
+	void *image;
+	int ret = 0;
+
+	image = bpf_jit_alloc_exec(PAGE_SIZE);
+	if (!image)
+		return ERR_PTR(-ENOMEM);
+
+	set_vm_flush_reset_perms(image);
+
+	ret = arch_prepare_bpf_trampoline(image, image + PAGE_SIZE,
+		&list->func_model, 0, &prog, 1, NULL, 0, NULL);
+	if (ret < 0) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	hook = kzalloc(sizeof(struct bpf_lsm_hook), GFP_KERNEL);
+	if (!hook) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	hook->image = image;
+	hook->prog = prog;
+	bpf_prog_inc(prog);
+	return hook;
+error:
+	bpf_jit_free_exec(image);
+	return ERR_PTR(ret);
+}
+
+static void bpf_lsm_hook_free(struct bpf_lsm_hook *tr)
+{
+	if (!tr)
+		return;
+
+	if (tr->prog)
+		bpf_prog_put(tr->prog);
+
+	bpf_jit_free_exec(tr->image);
+	kfree(tr);
+}
+
+int bpf_lsm_attach(struct bpf_prog *prog)
+{
+	struct bpf_lsm_hook *hook;
+	struct bpf_lsm_list *list;
+	void **addr;
+	int ret = 0;
+
+	/* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
+	 */
+	if (!capable(CAP_MAC_ADMIN))
+		return -EPERM;
+
+	if (!bpf_lsm_info.initialized)
+		return -EBUSY;
+
+	list = bpf_lsm_list_lookup(prog);
+	if (IS_ERR(list))
+		return PTR_ERR(list);
+
+	hook = bpf_lsm_hook_alloc(list, prog);
+	if (IS_ERR(hook))
+		return PTR_ERR(hook);
+
+	hook->sec_hook.head = list->security_list_head;
+	addr = bpf_lsm_get_func_addr(&hook->sec_hook,
+				     prog->aux->attach_func_name);
+	if (IS_ERR(addr)) {
+		ret = PTR_ERR(addr);
+		goto error;
+	}
+
+	*addr = hook->image;
+
+	mutex_lock(&list->mutex);
+	hlist_add_tail_rcu(&hook->sec_hook.list, hook->sec_hook.head);
+	mutex_unlock(&list->mutex);
+	return 0;
+
+error:
+	bpf_lsm_hook_free(hook);
+	return ret;
+}
+
+int bpf_lsm_detach(struct bpf_prog *prog)
+{
+	struct security_hook_list *sec_hook;
+	struct bpf_lsm_hook *hook = NULL;
+	struct bpf_lsm_list *list;
+	struct hlist_node *n;
+
+	/* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
+	 */
+	if (!capable(CAP_MAC_ADMIN))
+		return -EPERM;
+
+	if (!bpf_lsm_info.initialized)
+		return -EBUSY;
+
+	list = &bpf_lsm_info.hook_lists[prog->aux->lsm_hook_index - 1];
+
+	mutex_lock(&list->mutex);
+	hlist_for_each_entry_safe(sec_hook, n, list->security_list_head, list) {
+		hook = container_of(sec_hook, struct bpf_lsm_hook, sec_hook);
+		if (hook->prog == prog) {
+			hlist_del_rcu(&hook->sec_hook.list);
+			break;
+		}
+	}
+	mutex_unlock(&list->mutex);
+	/* call_rcu is not used directly as module_memfree cannot run from an
+	 * softirq context. The best way would be to schedule this on a work
+	 * queue.
+	 */
+	synchronize_srcu(&security_hook_srcu);
+	bpf_lsm_hook_free(hook);
+	return 0;
+}
diff --git a/security/bpf/include/bpf_lsm.h b/security/bpf/include/bpf_lsm.h
new file mode 100644
index 000000000000..849f7f64d7f2
--- /dev/null
+++ b/security/bpf/include/bpf_lsm.h
@@ -0,0 +1,75 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _BPF_LSM_H
+#define _BPF_LSM_H
+
+#include <linux/filter.h>
+#include <linux/lsm_hooks.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
+
+
+struct bpf_lsm_hook {
+	/* The security_hook_list is initialized dynamically. These are
+	 * initialized in static LSMs by LSM_HOOK_INIT.
+	 */
+	struct security_hook_list sec_hook;
+	/* The BPF program for which this hook was allocated, this is used upon
+	 * detachment to find the hook corresponding to a program.
+	 */
+	struct bpf_prog *prog;
+	/* The address of the allocated function */
+	void *image;
+};
+
+/* The list represents the list of hooks attached to a particular
+ * security_list_head and contains information required for attaching and
+ * detaching BPF Programs.
+ */
+struct bpf_lsm_list {
+	/* Used on the first attached BPF program to populate the remaining
+	 * information
+	 */
+	bool initialized;
+	/* This mutex is used to serialize accesses to all the fields in
+	 * this structure.
+	 */
+	struct mutex mutex;
+	/* The BTF type for this hook.
+	 */
+	const struct btf_type *attach_type;
+	/* func_model for the setup of the callback.
+	 */
+	struct btf_func_model func_model;
+	/* The list of functions currently associated with the LSM hook.
+	 */
+	struct list_head callback_list;
+	/* The head to which the allocated hooks must be attached to.
+	 */
+	struct hlist_head *security_list_head;
+};
+
+struct bpf_lsm_info {
+	/* Dynamic Hooks can only be attached after the LSM is initialized.
+	 */
+	bool initialized;
+	/* The number of hooks is calculated at runtime using the BTF
+	 * information of the struct security_hook_heads.
+	 */
+	size_t num_hooks;
+	/* The hook_lists is allocated during __init and mutexes for each
+	 * allocated on __init, the remaining initialization happens when a
+	 * BPF program is attached to the list.
+	 */
+	struct bpf_lsm_list *hook_lists;
+	/* BTF type for security_hook_heads populated at init.
+	 */
+	const struct btf_type *btf_hook_heads;
+	/* BTF type for security_list_options populated at init.
+	 */
+	const struct btf_type *btf_hook_types;
+};
+
+extern struct bpf_lsm_info bpf_lsm_info;
+
+#endif /* _BPF_LSM_H */
diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
index d4ea6aa9ddb8..3bab187e7574 100644
--- a/security/bpf/lsm.c
+++ b/security/bpf/lsm.c
@@ -7,6 +7,8 @@
 #include <linux/bpf_lsm.h>
 #include <linux/lsm_hooks.h>
 
+#include "bpf_lsm.h"
+
 /* This is only for internal hooks, always statically shipped as part of the
  * BPF LSM. Statically defined hooks are appended to the security_hook_heads
  * which is common for LSMs and R/O after init.
@@ -19,6 +21,64 @@ static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
  */
 struct security_hook_heads bpf_lsm_hook_heads;
 
+/* Security hooks registered dynamically by the BPF LSM and must be accessed
+ * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock.
+ */
+struct bpf_lsm_info bpf_lsm_info;
+
+static __init int init_lsm_info(void)
+{
+	const struct btf_type *t;
+	size_t num_hooks;
+	int i;
+
+	if (!btf_vmlinux)
+		/* No need to grab any locks because we are still in init */
+		btf_vmlinux = btf_parse_vmlinux();
+
+	if (IS_ERR(btf_vmlinux)) {
+		pr_err("btf_vmlinux is malformed\n");
+		return PTR_ERR(btf_vmlinux);
+	}
+
+	t = btf_type_by_name_kind(btf_vmlinux, "security_hook_heads",
+				  BTF_KIND_STRUCT);
+	if (WARN_ON(IS_ERR(t)))
+		return PTR_ERR(t);
+
+	num_hooks = btf_type_vlen(t);
+	if (num_hooks <= 0)
+		return -EINVAL;
+
+	bpf_lsm_info.num_hooks = num_hooks;
+	bpf_lsm_info.btf_hook_heads = t;
+
+	t = btf_type_by_name_kind(btf_vmlinux, "security_list_options",
+				  BTF_KIND_UNION);
+	if (WARN_ON(IS_ERR(t)))
+		return PTR_ERR(t);
+
+	bpf_lsm_info.btf_hook_types = t;
+
+	bpf_lsm_info.hook_lists = kcalloc(num_hooks,
+		sizeof(struct bpf_lsm_list), GFP_KERNEL);
+	if (!bpf_lsm_info.hook_lists)
+		return -ENOMEM;
+
+	/* The mutex needs to be initialized at init as it must be held
+	 * when mutating the list. The rest of the information in the list
+	 * is populated lazily when the first LSM hook callback is appeneded
+	 * to the list.
+	 */
+	for (i = 0; i < num_hooks; i++)
+		mutex_init(&bpf_lsm_info.hook_lists[i].mutex);
+
+	bpf_lsm_info.initialized = true;
+	return 0;
+}
+
+late_initcall(init_lsm_info);
+
 static int __init lsm_init(void)
 {
 	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
diff --git a/security/bpf/ops.c b/security/bpf/ops.c
index 81c2bd9c0495..9a26633ac6f3 100644
--- a/security/bpf/ops.c
+++ b/security/bpf/ops.c
@@ -6,6 +6,7 @@
 
 #include <linux/filter.h>
 #include <linux/bpf.h>
+#include <linux/btf.h>
 
 const struct bpf_prog_ops lsm_prog_ops = {
 };
@@ -25,4 +26,5 @@ static const struct bpf_func_proto *get_bpf_func_proto(
 
 const struct bpf_verifier_ops lsm_verifier_ops = {
 	.get_func_proto = get_bpf_func_proto,
+	.is_valid_access = btf_ctx_access,
 };
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index b6a725a8a21d..fa5513cb3332 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -448,6 +448,10 @@ union bpf_attr {
 		__u32		line_info_cnt;	/* number of bpf_line_info records */
 		__u32		attach_btf_id;	/* in-kernel BTF type id to attach to */
 		__u32		attach_prog_fd; /* 0 to attach to vmlinux */
+		/* Index (one-based) of the hlist_head in security_hook_heads to
+		 * which the program must be attached.
+		 */
+		__u32		lsm_hook_index;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
-- 
2.20.1

[PATCH bpf-next v2 07/10] bpf: lsm: Make the allocated callback RO+X

[KP Singh]

From: KP Singh <kpsingh@google.com>

This patch is not needed after arch_bpf_prepare_trampoline
moves to using text_poke.

The two IPI TLB flushes can be further optimized if a new API to handle
W^X in the kernel emerges as an outcome of:

  https://lore.kernel.org/bpf/20200103234725.22846-1-kpsingh@chromium.org/

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

diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 4e71da0e8e9e..30f68341f5ef 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -222,6 +222,15 @@ static struct bpf_lsm_hook *bpf_lsm_hook_alloc(
 		goto error;
 	}
 
+	/* First make the page read-only, and only then make it executable to
+	 * prevent it from being W+X in between.
+	 */
+	set_memory_ro((unsigned long)image, 1);
+	/* More checks can be done here to ensure that nothing was changed
+	 * between arch_prepare_bpf_trampoline and set_memory_ro.
+	 */
+	set_memory_x((unsigned long)image, 1);
+
 	hook = kzalloc(sizeof(struct bpf_lsm_hook), GFP_KERNEL);
 	if (!hook) {
 		ret = -ENOMEM;
-- 
2.20.1

[PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[KP Singh]

From: KP Singh <kpsingh@google.com>

* Add functionality in libbpf to attach eBPF program to LSM hooks
* Lookup the index of the LSM hook in security_hook_heads and pass it in
  attr->lsm_hook_index

Signed-off-by: KP Singh <kpsingh@google.com>
---
 tools/lib/bpf/bpf.c      |   6 +-
 tools/lib/bpf/bpf.h      |   1 +
 tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
 tools/lib/bpf/libbpf.h   |   4 ++
 tools/lib/bpf/libbpf.map |   3 +
 5 files changed, 138 insertions(+), 19 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 500afe478e94..b138d98ff862 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
 	memset(&attr, 0, sizeof(attr));
 	attr.prog_type = load_attr->prog_type;
 	attr.expected_attach_type = load_attr->expected_attach_type;
-	if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
+
+	if (attr.prog_type == BPF_PROG_TYPE_LSM) {
+		attr.lsm_hook_index = load_attr->lsm_hook_index;
+	} else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
 		attr.attach_btf_id = load_attr->attach_btf_id;
 	} else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
 		attr.attach_btf_id = load_attr->attach_btf_id;
@@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
 		attr.prog_ifindex = load_attr->prog_ifindex;
 		attr.kern_version = load_attr->kern_version;
 	}
+
 	attr.insn_cnt = (__u32)load_attr->insns_cnt;
 	attr.insns = ptr_to_u64(load_attr->insns);
 	attr.license = ptr_to_u64(load_attr->license);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 56341d117e5b..54458a102939 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -86,6 +86,7 @@ struct bpf_load_program_attr {
 		__u32 prog_ifindex;
 		__u32 attach_btf_id;
 	};
+	__u32 lsm_hook_index;
 	__u32 prog_btf_fd;
 	__u32 func_info_rec_size;
 	const void *func_info;
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 0c229f00a67e..60737559a9a6 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -229,6 +229,7 @@ struct bpf_program {
 	enum bpf_attach_type expected_attach_type;
 	__u32 attach_btf_id;
 	__u32 attach_prog_fd;
+	__u32 lsm_hook_index;
 	void *func_info;
 	__u32 func_info_rec_size;
 	__u32 func_info_cnt;
@@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	load_attr.insns = insns;
 	load_attr.insns_cnt = insns_cnt;
 	load_attr.license = license;
-	if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
+
+	if (prog->type == BPF_PROG_TYPE_LSM) {
+		load_attr.lsm_hook_index = prog->lsm_hook_index;
+	} else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
 		load_attr.attach_btf_id = prog->attach_btf_id;
 	} else if (prog->type == BPF_PROG_TYPE_TRACING) {
 		load_attr.attach_prog_fd = prog->attach_prog_fd;
@@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 		load_attr.kern_version = kern_version;
 		load_attr.prog_ifindex = prog->prog_ifindex;
 	}
+
 	/* if .BTF.ext was loaded, kernel supports associated BTF for prog */
 	if (prog->obj->btf_ext)
 		btf_fd = bpf_object__btf_fd(prog->obj);
@@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
 				     enum bpf_attach_type attach_type,
 				     __u32 attach_prog_fd);
 
+static __s32 btf__find_lsm_hook_index(const char *name);
+
 int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 {
-	int err = 0, fd, i, btf_id;
+	int err = 0, fd, i, btf_id, index;
 
 	if (prog->type == BPF_PROG_TYPE_TRACING) {
 		btf_id = libbpf_find_attach_btf_id(prog->section_name,
@@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
 		prog->attach_btf_id = btf_id;
 	}
 
+	if (prog->type == BPF_PROG_TYPE_LSM) {
+		index = btf__find_lsm_hook_index(prog->section_name);
+		if (index < 0)
+			return index;
+		prog->lsm_hook_index = index;
+	}
+
 	if (prog->instances.nr < 0 || !prog->instances.fds) {
 		if (prog->preprocessor) {
 			pr_warn("Internal error: can't load program '%s'\n",
@@ -6207,6 +6221,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(lsm, BPF_PROG_TYPE_LSM);
 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);
@@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
 				      struct bpf_program *prog);
 static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
 				     struct bpf_program *prog);
+static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
+				   struct bpf_program *prog);
 
 struct bpf_sec_def {
 	const char *sec;
@@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
 		.expected_attach_type = BPF_TRACE_FEXIT,
 		.is_attach_btf = true,
 		.attach_fn = attach_trace),
+	SEC_DEF("lsm/", LSM,
+		.expected_attach_type = BPF_LSM_MAC,
+		.attach_fn = attach_lsm),
 	BPF_PROG_SEC("xdp",			BPF_PROG_TYPE_XDP),
 	BPF_PROG_SEC("perf_event",		BPF_PROG_TYPE_PERF_EVENT),
 	BPF_PROG_SEC("lwt_in",			BPF_PROG_TYPE_LWT_IN),
 	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_PROG_BTF("lsm/",			BPF_PROG_TYPE_LSM,
+						BPF_LSM_MAC),
 	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,
@@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
 	return -EINVAL;
 }
 
-#define BTF_PREFIX "btf_trace_"
+#define BTF_TRACE_PREFIX "btf_trace_"
+
+static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
+					   const char *prefix, __u32 kind)
+{
+	char btf_type_name[128];
+
+	snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
+	return btf__find_by_name_kind(btf, btf_type_name, kind);
+}
+
+static __s32 btf__find_lsm_hook_index(const char *name)
+{
+	struct btf *btf = bpf_find_kernel_btf();
+	const struct bpf_sec_def *sec_def;
+	const struct btf_type *hl_type;
+	struct btf_member *m;
+	__u16 vlen;
+	__s32 hl_id;
+	int j;
+
+	sec_def = find_sec_def(name);
+	if (!sec_def)
+		return -ESRCH;
+
+	name += sec_def->len;
+
+	hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
+				       BTF_KIND_STRUCT);
+	if (hl_id < 0) {
+		pr_debug("security_hook_heads cannot be found in BTF\n");
+		return hl_id;
+	}
+
+	hl_type = btf__type_by_id(btf, hl_id);
+	if (!hl_type) {
+		pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
+		return -EINVAL;
+	}
+
+	m = btf_members(hl_type);
+	vlen = btf_vlen(hl_type);
+
+	for (j = 0; j < vlen; j++) {
+		if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
+			return j + 1;
+		m++;
+	}
+
+	pr_warn("Cannot find offset for %s in security_hook_heads\n", name);
+	return -ENOENT;
+}
+
 int libbpf_find_vmlinux_btf_id(const char *name,
 			       enum bpf_attach_type attach_type)
 {
 	struct btf *btf = bpf_find_kernel_btf();
-	char raw_tp_btf[128] = BTF_PREFIX;
-	char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
-	const char *btf_name;
 	int err = -EINVAL;
-	__u32 kind;
 
 	if (IS_ERR(btf)) {
 		pr_warn("vmlinux BTF is not found\n");
 		return -EINVAL;
 	}
 
-	if (attach_type == BPF_TRACE_RAW_TP) {
-		/* prepend "btf_trace_" prefix per kernel convention */
-		strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
-		btf_name = raw_tp_btf;
-		kind = BTF_KIND_TYPEDEF;
-	} else {
-		btf_name = name;
-		kind = BTF_KIND_FUNC;
-	}
-	err = btf__find_by_name_kind(btf, btf_name, kind);
+	if (attach_type == BPF_TRACE_RAW_TP)
+		err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
+					       BTF_KIND_TYPEDEF);
+	else
+		err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
+
+	/* err = 0 means void / UNKNOWN which is treated as an error */
+	if (err == 0)
+		err = -EINVAL;
+
 	btf__free(btf);
 	return err;
 }
@@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 	}
 	err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
 	btf__free(btf);
-	if (err <= 0) {
+	if (err < 0) {
 		pr_warn("%s is not found in prog's BTF\n", name);
 		goto out;
 	}
@@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
 	return bpf_program__attach_trace(prog);
 }
 
+struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
+{
+	char errmsg[STRERR_BUFSIZE];
+	struct bpf_link_fd *link;
+	int prog_fd, pfd;
+
+	prog_fd = bpf_program__fd(prog);
+	if (prog_fd < 0) {
+		pr_warn("program '%s': can't attach before loaded\n",
+			bpf_program__title(prog, false));
+		return ERR_PTR(-EINVAL);
+	}
+
+	link = calloc(1, sizeof(*link));
+	if (!link)
+		return ERR_PTR(-ENOMEM);
+	link->link.detach = &bpf_link__detach_fd;
+
+	pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
+			      BPF_F_ALLOW_OVERRIDE);
+	if (pfd < 0) {
+		pfd = -errno;
+		pr_warn("program '%s': failed to attach: %s\n",
+			bpf_program__title(prog, false),
+			libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
+		return ERR_PTR(pfd);
+	}
+	link->fd = pfd;
+	return (struct bpf_link *)link;
+}
+
+static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
+				   struct bpf_program *prog)
+{
+	return bpf_program__attach_lsm(prog);
+}
+
 struct bpf_link *bpf_program__attach(struct bpf_program *prog)
 {
 	const struct bpf_sec_def *sec_def;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 01639f9a1062..a97e709a29e6 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
 bpf_program__attach_trace(struct bpf_program *prog);
 struct bpf_map;
 LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_lsm(struct bpf_program *prog);
 struct bpf_insn;
 
 /*
@@ -318,6 +320,7 @@ 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_tracing(struct bpf_program *prog);
 LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
+LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
 
 LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
 LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
@@ -339,6 +342,7 @@ 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_tracing(const struct bpf_program *prog);
 LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
+LIBBPF_API bool bpf_program__is_lsm(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 a19f04e6e3d9..3da0452ce679 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
 		bpf_program__is_struct_ops;
 		bpf_program__set_struct_ops;
 		btf__align_of;
+		bpf_program__is_lsm;
+		bpf_program__set_lsm;
+		bpf_program__attach_lsm;
 } LIBBPF_0.0.6;
-- 
2.20.1

[PATCH bpf-next v2 09/10] bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM

[KP Singh]

From: KP Singh <kpsingh@google.com>

* Load a BPF program that audits mprotect calls
* Attach the program to the "file_mprotect" LSM hook
* Initialize the perf events buffer and poll for audit events
* Do an mprotect on some memory allocated on the heap
* Verify if the audit event was received

Signed-off-by: KP Singh <kpsingh@google.com>
---
 MAINTAINERS                                   |  2 +
 tools/testing/selftests/bpf/lsm_helpers.h     | 19 ++++++
 .../bpf/prog_tests/lsm_mprotect_audit.c       | 58 +++++++++++++++++++
 .../selftests/bpf/progs/lsm_mprotect_audit.c  | 48 +++++++++++++++
 4 files changed, 127 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/lsm_helpers.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
 create mode 100644 tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 02d7e05e9b75..5d553c2e7452 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3210,6 +3210,8 @@ L:	bpf@vger.kernel.org
 S:	Maintained
 F:	security/bpf/
 F:	include/linux/bpf_lsm.h
+F:	tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c
+F:	tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
 
 BROADCOM B44 10/100 ETHERNET DRIVER
 M:	Michael Chan <michael.chan@broadcom.com>
diff --git a/tools/testing/selftests/bpf/lsm_helpers.h b/tools/testing/selftests/bpf/lsm_helpers.h
new file mode 100644
index 000000000000..8bad08f77654
--- /dev/null
+++ b/tools/testing/selftests/bpf/lsm_helpers.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+#ifndef _LSM_HELPERS_H
+#define _LSM_HELPERS_H
+
+struct lsm_mprotect_audit_result {
+	/* This ensures that the LSM Hook only monitors the PID requested
+	 * by the loader
+	 */
+	__u32 monitored_pid;
+	/* The number of mprotect calls for the monitored PID.
+	 */
+	__u32 mprotect_count;
+};
+
+#endif /* _LSM_HELPERS_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c b/tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
new file mode 100644
index 000000000000..ff90b874eafc
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#include <test_progs.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <malloc.h>
+#include "lsm_helpers.h"
+#include "lsm_mprotect_audit.skel.h"
+
+int heap_mprotect(void)
+{
+	void *buf;
+	long sz;
+
+	sz = sysconf(_SC_PAGESIZE);
+	if (sz < 0)
+		return sz;
+
+	buf = memalign(sz, 2 * sz);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	return mprotect(buf, sz, PROT_READ | PROT_EXEC);
+}
+
+void test_lsm_mprotect_audit(void)
+{
+	struct lsm_mprotect_audit_result *result;
+	struct lsm_mprotect_audit *skel = NULL;
+	int err, duration = 0;
+
+	skel = lsm_mprotect_audit__open_and_load();
+	if (CHECK(!skel, "skel_load", "lsm_mprotect_audit skeleton failed\n"))
+		goto close_prog;
+
+	err = lsm_mprotect_audit__attach(skel);
+	if (CHECK(err, "attach", "lsm_mprotect_audit attach failed: %d\n", err))
+		goto close_prog;
+
+	result = &skel->bss->result;
+	result->monitored_pid = getpid();
+
+	err = heap_mprotect();
+	if (CHECK(err < 0, "heap_mprotect", "err %d errno %d\n", err, errno))
+		goto close_prog;
+
+	/* Make sure mprotect_audit program was triggered
+	 * and detected an mprotect on the heap.
+	 */
+	CHECK_FAIL(result->mprotect_count != 1);
+
+close_prog:
+	lsm_mprotect_audit__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c b/tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c
new file mode 100644
index 000000000000..f4569b418616
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2019 Google LLC.
+ */
+
+#include <linux/bpf.h>
+#include <stdbool.h>
+#include "bpf_helpers.h"
+#include "bpf_trace_helpers.h"
+#include "lsm_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct lsm_mprotect_audit_result result = {
+	.mprotect_count = 0,
+	.monitored_pid = 0,
+};
+
+/*
+ * Define some of the structs used in the BPF program.
+ * Only the field names and their sizes need to be the
+ * same as the kernel type, the order is irrelevant.
+ */
+struct mm_struct {
+	unsigned long start_brk, brk;
+} __attribute__((preserve_access_index));
+
+struct vm_area_struct {
+	unsigned long vm_start, vm_end;
+	struct mm_struct *vm_mm;
+} __attribute__((preserve_access_index));
+
+SEC("lsm/file_mprotect")
+int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
+	     unsigned long reqprot, unsigned long prot)
+{
+	__u32 pid = bpf_get_current_pid_tgid();
+	int is_heap = 0;
+
+	is_heap = (vma->vm_start >= vma->vm_mm->start_brk &&
+		   vma->vm_end <= vma->vm_mm->brk);
+
+	if (is_heap && result.monitored_pid == pid)
+		result.mprotect_count++;
+
+	return 0;
+}
-- 
2.20.1

[PATCH bpf-next v2 10/10] bpf: lsm: Add Documentation

[KP Singh]

From: KP Singh <kpsingh@google.com>

Document how eBPF programs (BPF_PROG_TYPE_LSM) can be loaded and
attached (BPF_LSM_MAC) to the LSM hooks.

Signed-off-by: KP Singh <kpsingh@google.com>
---
 Documentation/security/bpf.rst   | 150 +++++++++++++++++++++++++++++++
 Documentation/security/index.rst |   1 +
 MAINTAINERS                      |   1 +
 3 files changed, 152 insertions(+)
 create mode 100644 Documentation/security/bpf.rst

diff --git a/Documentation/security/bpf.rst b/Documentation/security/bpf.rst
new file mode 100644
index 000000000000..4d115c07c370
--- /dev/null
+++ b/Documentation/security/bpf.rst
@@ -0,0 +1,150 @@
+.. SPDX-License-Identifier: GPL-2.0+
+.. Copyright 2019 Google LLC.
+
+==========================
+eBPF Linux Security Module
+==========================
+
+This LSM allows runtime instrumentation of the LSM hooks by privileged users to
+implement system-wide MAC (Mandatory Access Control) and Audit policies using
+eBPF. The LSM is privileged and stackable and requires both ``CAP_SYS_ADMIN``
+and ``CAP_MAC_ADMIN`` for the loading of BPF programs and modification of MAC
+policies respectively.
+
+eBPF Programs
+==============
+
+`eBPF (extended BPF) <https://cilium.readthedocs.io/en/latest/bpf>`_ is a
+virtual machine-like construct in the Linux Kernel allowing the execution of
+verifiable, just-in-time compiled byte code at various points in the Kernel.
+
+The eBPF LSM adds a new type, ``BPF_PROG_TYPE_LSM``, of eBPF programs which
+have the following characteristics:
+
+	* Multiple eBPF programs can be attached to the same LSM hook
+	* The programs are always run after the static hooks (i.e. the ones
+	  registered by SELinux, AppArmor, Smack etc.)
+	* LSM hooks can return an ``-EPERM`` to indicate the decision of the
+	  MAC policy being enforced or simply be used for auditing
+	* If ``CONFIG_SECURITY_BPF_ENFORCE`` is enabled and a non-zero error
+	  code is returned from the BPF program, no further BPF programs for the hook are executed
+	* Allowing the eBPF programs to be attached to all the LSM hooks by
+	  making :doc:`/bpf/btf` type information available for all LSM hooks
+	  and allowing the BPF verifier to perform runtime relocations and
+	  validation on the programs
+
+Structure
+---------
+
+The example shows an eBPF program that can be attached to the ``file_mprotect``
+LSM hook:
+
+.. c:function:: int file_mprotect(struct vm_area_struct *vma, unsigned long reqprot, unsigned long prot);
+
+eBPF programs that use :doc:`/bpf/btf` do not need to include kernel headers
+for accessing information from the attached eBPF program's context. They can
+simply declare the structures in the eBPF program and only specify the fields
+that need to be accessed.
+
+.. code-block:: c
+
+	struct mm_struct {
+		unsigned long start_brk, brk, start_stack;
+	} __attribute__((preserve_access_index));
+
+	struct vm_area_struct {
+		unsigned long start_brk, brk, start_stack;
+		unsigned long vm_start, vm_end;
+		struct mm_struct *vm_mm;
+	} __attribute__((preserve_access_index));
+
+
+.. note:: Only the size and the names of the fields must match the type in the
+	  kernel and the order of the fields is irrelevant.
+
+The eBPF programs can be declared using macros similar to the ``BPF_TRACE_<N>``
+macros defined in `tools/testing/selftests/bpf/bpf_trace_helpers.h`_. In this
+example:
+
+	* The LSM hook takes 3 args so we use ``BPF_TRACE_3``
+	* ``"lsm/file_mprotect"`` indicates the LSM hook that the program must
+	  be attached to
+	* ``mprotect_audit`` is the name of the eBPF program
+
+.. code-block:: c
+
+        SEC("lsm/file_mprotect")
+        int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
+                     unsigned long reqprot, unsigned long prot)
+	{
+		int is_heap;
+
+		is_heap = (vma->vm_start >= vma->vm_mm->start_brk &&
+			   vma->vm_end <= vma->vm_mm->brk);
+
+		/*
+		 * Return an -EPERM or write information to the perf events buffer
+		 * for auditing
+		 */
+	}
+
+The ``__attribute__((preserve_access_index))`` is a clang feature that allows
+the BPF verifier to update the offsets for the access at runtime using the
+:doc:`/bpf/btf` information. Since the BPF verifier is aware of the types, it
+also validates all the accesses made to the various types in the eBPF program.
+
+Loading
+-------
+
+eBPP programs can be loaded with the :manpage:`bpf(2)` syscall's
+``BPF_PROG_LOAD`` operation or more simply by using the the libbpf helper
+``bpf_prog_load_xattr``:
+
+
+.. code-block:: c
+
+	struct bpf_prog_load_attr attr = {
+		.file = "./prog.o",
+	};
+	struct bpf_object *prog_obj;
+	struct bpf_program *prog;
+	int prog_fd;
+
+	bpf_prog_load_xattr(&attr, &prog_obj, &prog_fd);
+
+Attachment to LSM Hooks
+-----------------------
+
+The LSM allows attachment of eBPF programs as LSM hooks using :manpage:`bpf(2)`
+syscall's ``BPF_PROG_ATTACH`` operation or more simply by
+using the libbpf helper ``bpf_program__attach_lsm``. In the code shown below
+``prog`` is the eBPF program loaded using ``BPF_PROG_LOAD``:
+
+.. code-block:: c
+
+	struct bpf_link *link;
+
+	link = bpf_program__attach_lsm(prog);
+
+The program can be detached from the LSM hook by *destroying* the ``link``
+link returned by ``bpf_program__attach_lsm``:
+
+.. code-block:: c
+
+	link->destroy();
+
+Examples
+--------
+
+An example eBPF program can be found in
+`tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c`_ and the corresponding
+userspace code in
+`tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c`_
+
+.. Links
+.. _tools/testing/selftests/bpf/bpf_trace_helpers.h:
+   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/tools/testing/selftests/selftests/bpf/bpf_trace_helpers.h
+.. _tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c:
+   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c
+.. _tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c:
+   https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
diff --git a/Documentation/security/index.rst b/Documentation/security/index.rst
index fc503dd689a7..844463df4547 100644
--- a/Documentation/security/index.rst
+++ b/Documentation/security/index.rst
@@ -5,6 +5,7 @@ Security Documentation
 .. toctree::
    :maxdepth: 1
 
+   bpf
    credentials
    IMA-templates
    keys/index
diff --git a/MAINTAINERS b/MAINTAINERS
index 5d553c2e7452..dd4c4ee151b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3212,6 +3212,7 @@ F:	security/bpf/
 F:	include/linux/bpf_lsm.h
 F:	tools/testing/selftests/bpf/progs/lsm_mprotect_audit.c
 F:	tools/testing/selftests/bpf/prog_tests/lsm_mprotect_audit.c
+F:	Documentation/security/bpf.rst
 
 BROADCOM B44 10/100 ETHERNET DRIVER
 M:	Michael Chan <michael.chan@broadcom.com>
-- 
2.20.1

Re: [PATCH bpf-next v2 06/10] bpf: lsm: Implement attach, detach and execution

[Greg Kroah-Hartman]

On Wed, Jan 15, 2020 at 06:13:29PM +0100, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> JITed BPF programs are used by the BPF LSM as dynamically allocated
> security hooks. arch_bpf_prepare_trampoline handles the
> arch_bpf_prepare_trampoline generates code to handle conversion of the
> signature of the hook to the BPF context and allows the BPF program to
> be called directly as a C function.
> 
> The following permissions are required to attach a program to a hook:
> 
> - CAP_SYS_ADMIN to load the program
> - CAP_MAC_ADMIN to attach it (i.e. to update the security policy)

You forgot to list "GPL-compatible license" here :)

Anyway, looks good to me:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[Stephen Smalley]

On 1/15/20 12:13 PM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> - The list of hooks registered by an LSM is currently immutable as they
>    are declared with __lsm_ro_after_init and they are attached to a
>    security_hook_heads struct.
> - For the BPF LSM we need to de/register the hooks at runtime. Making
>    the existing security_hook_heads mutable broadens an
>    attack vector, so a separate security_hook_heads is added for only
>    those that ~must~ be mutable.
> - These mutable hooks are run only after all the static hooks have
>    successfully executed.
> 
> This is based on the ideas discussed in:
> 
>    https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
> 
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
[...]
> diff --git a/security/security.c b/security/security.c
> index cd2d18d2d279..4a2eb4c089b2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -652,20 +653,21 @@ static void __init lsm_early_task(struct task_struct *task)
>   								\
>   		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
>   			P->hook.FUNC(__VA_ARGS__);		\
> +		CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__);	\
>   	} while (0)
>   
> -#define call_int_hook(FUNC, IRC, ...) ({			\
> -	int RC = IRC;						\
> -	do {							\
> -		struct security_hook_list *P;			\
> -								\
> +#define call_int_hook(FUNC, IRC, ...) ({				\
> +	int RC = IRC;							\
> +	do {								\
> +		struct security_hook_list *P;				\
>   		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> -			RC = P->hook.FUNC(__VA_ARGS__);		\
> -			if (RC != 0)				\
> -				break;				\
> -		}						\
> -	} while (0);						\
> -	RC;							\
> +			RC = P->hook.FUNC(__VA_ARGS__);			\
> +			if (RC != 0)					\
> +				break;					\
> +		}							\
> +		RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__);	\

Let's not clobber the return code from the other LSMs with the bpf one.

> +	} while (0);							\
> +	RC;								\
>   })
>   
>   /* Security operations */
> 

Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[Andrii Nakryiko]

On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> * Add functionality in libbpf to attach eBPF program to LSM hooks
> * Lookup the index of the LSM hook in security_hook_heads and pass it in
>   attr->lsm_hook_index
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  tools/lib/bpf/bpf.c      |   6 +-
>  tools/lib/bpf/bpf.h      |   1 +
>  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
>  tools/lib/bpf/libbpf.h   |   4 ++
>  tools/lib/bpf/libbpf.map |   3 +
>  5 files changed, 138 insertions(+), 19 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 500afe478e94..b138d98ff862 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
>         memset(&attr, 0, sizeof(attr));
>         attr.prog_type = load_attr->prog_type;
>         attr.expected_attach_type = load_attr->expected_attach_type;
> -       if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> +
> +       if (attr.prog_type == BPF_PROG_TYPE_LSM) {
> +               attr.lsm_hook_index = load_attr->lsm_hook_index;
> +       } else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
>                 attr.attach_btf_id = load_attr->attach_btf_id;
>         } else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
>                 attr.attach_btf_id = load_attr->attach_btf_id;
> @@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
>                 attr.prog_ifindex = load_attr->prog_ifindex;
>                 attr.kern_version = load_attr->kern_version;
>         }
> +
>         attr.insn_cnt = (__u32)load_attr->insns_cnt;
>         attr.insns = ptr_to_u64(load_attr->insns);
>         attr.license = ptr_to_u64(load_attr->license);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 56341d117e5b..54458a102939 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -86,6 +86,7 @@ struct bpf_load_program_attr {
>                 __u32 prog_ifindex;
>                 __u32 attach_btf_id;
>         };
> +       __u32 lsm_hook_index;


this is changing memory layout of struct bpf_load_program_attr, which
is part of public API, so breaking backward compatibility. But I think
you intended to put it inside union along the attach_btf_id?

also, we use idx for index pretty consistently (apart from ifindex),
so maybe lsm_hook_idx?

>         __u32 prog_btf_fd;
>         __u32 func_info_rec_size;
>         const void *func_info;
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 0c229f00a67e..60737559a9a6 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -229,6 +229,7 @@ struct bpf_program {
>         enum bpf_attach_type expected_attach_type;
>         __u32 attach_btf_id;
>         __u32 attach_prog_fd;
> +       __u32 lsm_hook_index
>         void *func_info;
>         __u32 func_info_rec_size;
>         __u32 func_info_cnt;
> @@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>         load_attr.insns = insns;
>         load_attr.insns_cnt = insns_cnt;
>         load_attr.license = license;
> -       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> +
> +       if (prog->type == BPF_PROG_TYPE_LSM) {
> +               load_attr.lsm_hook_index = prog->lsm_hook_index;
> +       } else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
>                 load_attr.attach_btf_id = prog->attach_btf_id;
>         } else if (prog->type == BPF_PROG_TYPE_TRACING) {
>                 load_attr.attach_prog_fd = prog->attach_prog_fd;
> @@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>                 load_attr.kern_version = kern_version;
>                 load_attr.prog_ifindex = prog->prog_ifindex;
>         }
> +
>         /* if .BTF.ext was loaded, kernel supports associated BTF for prog */
>         if (prog->obj->btf_ext)
>                 btf_fd = bpf_object__btf_fd(prog->obj);
> @@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
>                                      enum bpf_attach_type attach_type,
>                                      __u32 attach_prog_fd);
>
> +static __s32 btf__find_lsm_hook_index(const char *name);
> +
>  int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
>  {
> -       int err = 0, fd, i, btf_id;
> +       int err = 0, fd, i, btf_id, index;
>
>         if (prog->type == BPF_PROG_TYPE_TRACING) {
>                 btf_id = libbpf_find_attach_btf_id(prog->section_name,
> @@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
>                 prog->attach_btf_id = btf_id;
>         }
>
> +       if (prog->type == BPF_PROG_TYPE_LSM) {
> +               index = btf__find_lsm_hook_index(prog->section_name);
> +               if (index < 0)
> +                       return index;
> +               prog->lsm_hook_index = index;
> +       }
> +
>         if (prog->instances.nr < 0 || !prog->instances.fds) {
>                 if (prog->preprocessor) {
>                         pr_warn("Internal error: can't load program '%s'\n",
> @@ -6207,6 +6221,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(lsm, BPF_PROG_TYPE_LSM);
>  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);
> @@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
>                                       struct bpf_program *prog);
>  static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
>                                      struct bpf_program *prog);
> +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> +                                  struct bpf_program *prog);
>
>  struct bpf_sec_def {
>         const char *sec;
> @@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
>                 .expected_attach_type = BPF_TRACE_FEXIT,
>                 .is_attach_btf = true,
>                 .attach_fn = attach_trace),
> +       SEC_DEF("lsm/", LSM,
> +               .expected_attach_type = BPF_LSM_MAC,
> +               .attach_fn = attach_lsm),
>         BPF_PROG_SEC("xdp",                     BPF_PROG_TYPE_XDP),
>         BPF_PROG_SEC("perf_event",              BPF_PROG_TYPE_PERF_EVENT),
>         BPF_PROG_SEC("lwt_in",                  BPF_PROG_TYPE_LWT_IN),
>         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_PROG_BTF("lsm/",                    BPF_PROG_TYPE_LSM,
> +                                               BPF_LSM_MAC),

This is just a duplicate of SEC_DEF above, remove?

>         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,
> @@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
>         return -EINVAL;
>  }
>
> -#define BTF_PREFIX "btf_trace_"
> +#define BTF_TRACE_PREFIX "btf_trace_"
> +
> +static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
> +                                          const char *prefix, __u32 kind)

this is internal helper, not really BTF API, let's call it
find_btf_by_prefix_kind? Also const char *prefix more logically should
go before name argument?

> +{
> +       char btf_type_name[128];
> +
> +       snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);

check overflow?

> +       return btf__find_by_name_kind(btf, btf_type_name, kind);
> +}
> +
> +static __s32 btf__find_lsm_hook_index(const char *name)

this name is violating libbpf naming guidelines. Just
`find_lsm_hook_idx` for now?

> +{
> +       struct btf *btf = bpf_find_kernel_btf();

ok, it's probably time to do this right. Let's ensure we load kernel
BTF just once, keep it inside bpf_object while we need it and then
release it after successful load. We are at the point where all the
new types of program is loading/releasing kernel BTF for every section
and it starts to feel very wasteful.

> +       const struct bpf_sec_def *sec_def;
> +       const struct btf_type *hl_type;
> +       struct btf_member *m;
> +       __u16 vlen;
> +       __s32 hl_id;
> +       int j;

j without having i used anywhere?...

> +
> +       sec_def = find_sec_def(name);
> +       if (!sec_def)
> +               return -ESRCH;
> +
> +       name += sec_def->len;
> +
> +       hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
> +                                      BTF_KIND_STRUCT);
> +       if (hl_id < 0) {
> +               pr_debug("security_hook_heads cannot be found in BTF\n");

"in vmlinux BTF" ?

and it should be pr_warn(), we don't really expect this, right?

and it should be (hl_id <= 0) with current btf__find_by_name_kind(),
and then return hl_id ? : -ESRCH, which further proves we need to
change btf__find_by_name_kind() as I suggested below.

> +               return hl_id;
> +       }
> +
> +       hl_type = btf__type_by_id(btf, hl_id);
> +       if (!hl_type) {
> +               pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
> +               return -EINVAL;

-ESRCH?

> +       }
> +
> +       m = btf_members(hl_type);
> +       vlen = btf_vlen(hl_type);
> +
> +       for (j = 0; j < vlen; j++) {

can add succinct `, m++` here instead

> +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> +                       return j + 1;

I looked briefly through kernel-side patch introducing lsm_hook_index,
but it didn't seem to explain why this index needs to be (unnaturally)
1-based. So asking here first as I'm looking through libbpf changes?

> +               m++;
> +       }
> +
> +       pr_warn("Cannot find offset for %s in security_hook_heads\n", name);

it's not offset, rather member index?

> +       return -ENOENT;

not entirely clear about distinction between ENOENT and ESRCH? So far
we typically used ESRCH, does ENOENT have more specific semantics?

> +}
> +
>  int libbpf_find_vmlinux_btf_id(const char *name,
>                                enum bpf_attach_type attach_type)
>  {
>         struct btf *btf = bpf_find_kernel_btf();
> -       char raw_tp_btf[128] = BTF_PREFIX;
> -       char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> -       const char *btf_name;
>         int err = -EINVAL;
> -       __u32 kind;
>
>         if (IS_ERR(btf)) {
>                 pr_warn("vmlinux BTF is not found\n");
>                 return -EINVAL;
>         }
>
> -       if (attach_type == BPF_TRACE_RAW_TP) {
> -               /* prepend "btf_trace_" prefix per kernel convention */
> -               strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> -               btf_name = raw_tp_btf;
> -               kind = BTF_KIND_TYPEDEF;
> -       } else {
> -               btf_name = name;
> -               kind = BTF_KIND_FUNC;
> -       }
> -       err = btf__find_by_name_kind(btf, btf_name, kind);
> +       if (attach_type == BPF_TRACE_RAW_TP)
> +               err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
> +                                              BTF_KIND_TYPEDEF);
> +       else
> +               err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> +
> +       /* err = 0 means void / UNKNOWN which is treated as an error */
> +       if (err == 0)
> +               err = -EINVAL;

I think it's actually less error-prone to make btf__find_by_name_kind
and btf__find_by_prefix_kind to return -ESRCH when type is not found,
instead of a valid type_id 0. I just checked, and struct_ops code
already is mishandling it, only checking for <0. Could you make this
change and just do a natural <0 check everywhere?


> +
>         btf__free(btf);
>         return err;
>  }
> @@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
>         }
>         err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
>         btf__free(btf);
> -       if (err <= 0) {
> +       if (err < 0) {
>                 pr_warn("%s is not found in prog's BTF\n", name);
>                 goto out;
>         }
> @@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
>         return bpf_program__attach_trace(prog);
>  }
>
> +struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
> +{
> +       char errmsg[STRERR_BUFSIZE];
> +       struct bpf_link_fd *link;
> +       int prog_fd, pfd;
> +
> +       prog_fd = bpf_program__fd(prog);
> +       if (prog_fd < 0) {
> +               pr_warn("program '%s': can't attach before loaded\n",
> +                       bpf_program__title(prog, false));
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       link = calloc(1, sizeof(*link));
> +       if (!link)
> +               return ERR_PTR(-ENOMEM);
> +       link->link.detach = &bpf_link__detach_fd;
> +
> +       pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
> +                             BPF_F_ALLOW_OVERRIDE);

do we want to always specify ALLOW_OVERRIDE? Or should it be an option?

> +       if (pfd < 0) {
> +               pfd = -errno;
> +               pr_warn("program '%s': failed to attach: %s\n",
> +                       bpf_program__title(prog, false),
> +                       libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> +               return ERR_PTR(pfd);

leaking link here

> +       }
> +       link->fd = pfd;
> +       return (struct bpf_link *)link;
> +}
> +
> +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> +                                  struct bpf_program *prog)
> +{
> +       return bpf_program__attach_lsm(prog);
> +}
> +
>  struct bpf_link *bpf_program__attach(struct bpf_program *prog)
>  {
>         const struct bpf_sec_def *sec_def;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 01639f9a1062..a97e709a29e6 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
>  bpf_program__attach_trace(struct bpf_program *prog);
>  struct bpf_map;
>  LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_lsm(struct bpf_program *prog);

nit: put it after attach_trace, so that program attaches and map
attaches are grouped together, not intermixed

>  struct bpf_insn;
>
>  /*
> @@ -318,6 +320,7 @@ 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_tracing(struct bpf_program *prog);
>  LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
> +LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
>
>  LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
>  LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> @@ -339,6 +342,7 @@ 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_tracing(const struct bpf_program *prog);
>  LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
> +LIBBPF_API bool bpf_program__is_lsm(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 a19f04e6e3d9..3da0452ce679 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
>                 bpf_program__is_struct_ops;
>                 bpf_program__set_struct_ops;
>                 btf__align_of;
> +               bpf_program__is_lsm;
> +               bpf_program__set_lsm;
> +               bpf_program__attach_lsm;

preserve alphabetical order, please

>  } LIBBPF_0.0.6;

> --
> 2.20.1
>

Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[Andrii Nakryiko]

On Wed, Jan 15, 2020 at 1:19 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> >   attr->lsm_hook_index
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  tools/lib/bpf/bpf.c      |   6 +-
> >  tools/lib/bpf/bpf.h      |   1 +
> >  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
> >  tools/lib/bpf/libbpf.h   |   4 ++
> >  tools/lib/bpf/libbpf.map |   3 +
> >  5 files changed, 138 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 500afe478e94..b138d98ff862 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> >         memset(&attr, 0, sizeof(attr));
> >         attr.prog_type = load_attr->prog_type;
> >         attr.expected_attach_type = load_attr->expected_attach_type;
> > -       if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +
> > +       if (attr.prog_type == BPF_PROG_TYPE_LSM) {
> > +               attr.lsm_hook_index = load_attr->lsm_hook_index;
> > +       } else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> >                 attr.attach_btf_id = load_attr->attach_btf_id;
> >         } else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
> >                 attr.attach_btf_id = load_attr->attach_btf_id;
> > @@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> >                 attr.prog_ifindex = load_attr->prog_ifindex;
> >                 attr.kern_version = load_attr->kern_version;
> >         }
> > +
> >         attr.insn_cnt = (__u32)load_attr->insns_cnt;
> >         attr.insns = ptr_to_u64(load_attr->insns);
> >         attr.license = ptr_to_u64(load_attr->license);
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 56341d117e5b..54458a102939 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -86,6 +86,7 @@ struct bpf_load_program_attr {
> >                 __u32 prog_ifindex;
> >                 __u32 attach_btf_id;
> >         };
> > +       __u32 lsm_hook_index;
>
>
> this is changing memory layout of struct bpf_load_program_attr, which
> is part of public API, so breaking backward compatibility. But I think
> you intended to put it inside union along the attach_btf_id?
>
> also, we use idx for index pretty consistently (apart from ifindex),
> so maybe lsm_hook_idx?
>
> >         __u32 prog_btf_fd;
> >         __u32 func_info_rec_size;
> >         const void *func_info;
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 0c229f00a67e..60737559a9a6 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -229,6 +229,7 @@ struct bpf_program {
> >         enum bpf_attach_type expected_attach_type;
> >         __u32 attach_btf_id;
> >         __u32 attach_prog_fd;
> > +       __u32 lsm_hook_index
> >         void *func_info;
> >         __u32 func_info_rec_size;
> >         __u32 func_info_cnt;
> > @@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >         load_attr.insns = insns;
> >         load_attr.insns_cnt = insns_cnt;
> >         load_attr.license = license;
> > -       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +
> > +       if (prog->type == BPF_PROG_TYPE_LSM) {
> > +               load_attr.lsm_hook_index = prog->lsm_hook_index;
> > +       } else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> >                 load_attr.attach_btf_id = prog->attach_btf_id;
> >         } else if (prog->type == BPF_PROG_TYPE_TRACING) {
> >                 load_attr.attach_prog_fd = prog->attach_prog_fd;
> > @@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >                 load_attr.kern_version = kern_version;
> >                 load_attr.prog_ifindex = prog->prog_ifindex;
> >         }
> > +
> >         /* if .BTF.ext was loaded, kernel supports associated BTF for prog */
> >         if (prog->obj->btf_ext)
> >                 btf_fd = bpf_object__btf_fd(prog->obj);
> > @@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
> >                                      enum bpf_attach_type attach_type,
> >                                      __u32 attach_prog_fd);
> >
> > +static __s32 btf__find_lsm_hook_index(const char *name);
> > +
> >  int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> >  {
> > -       int err = 0, fd, i, btf_id;
> > +       int err = 0, fd, i, btf_id, index;
> >
> >         if (prog->type == BPF_PROG_TYPE_TRACING) {
> >                 btf_id = libbpf_find_attach_btf_id(prog->section_name,
> > @@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> >                 prog->attach_btf_id = btf_id;
> >         }
> >
> > +       if (prog->type == BPF_PROG_TYPE_LSM) {
> > +               index = btf__find_lsm_hook_index(prog->section_name);
> > +               if (index < 0)
> > +                       return index;
> > +               prog->lsm_hook_index = index;
> > +       }
> > +
> >         if (prog->instances.nr < 0 || !prog->instances.fds) {
> >                 if (prog->preprocessor) {
> >                         pr_warn("Internal error: can't load program '%s'\n",
> > @@ -6207,6 +6221,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(lsm, BPF_PROG_TYPE_LSM);
> >  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);
> > @@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
> >                                       struct bpf_program *prog);
> >  static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> >                                      struct bpf_program *prog);
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > +                                  struct bpf_program *prog);
> >
> >  struct bpf_sec_def {
> >         const char *sec;
> > @@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
> >                 .expected_attach_type = BPF_TRACE_FEXIT,
> >                 .is_attach_btf = true,
> >                 .attach_fn = attach_trace),
> > +       SEC_DEF("lsm/", LSM,
> > +               .expected_attach_type = BPF_LSM_MAC,
> > +               .attach_fn = attach_lsm),
> >         BPF_PROG_SEC("xdp",                     BPF_PROG_TYPE_XDP),
> >         BPF_PROG_SEC("perf_event",              BPF_PROG_TYPE_PERF_EVENT),
> >         BPF_PROG_SEC("lwt_in",                  BPF_PROG_TYPE_LWT_IN),
> >         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_PROG_BTF("lsm/",                    BPF_PROG_TYPE_LSM,
> > +                                               BPF_LSM_MAC),
>
> This is just a duplicate of SEC_DEF above, remove?
>
> >         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,
> > @@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
> >         return -EINVAL;
> >  }
> >
> > -#define BTF_PREFIX "btf_trace_"
> > +#define BTF_TRACE_PREFIX "btf_trace_"
> > +
> > +static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
> > +                                          const char *prefix, __u32 kind)
>
> this is internal helper, not really BTF API, let's call it
> find_btf_by_prefix_kind? Also const char *prefix more logically should
> go before name argument?
>
> > +{
> > +       char btf_type_name[128];
> > +
> > +       snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
>
> check overflow?
>
> > +       return btf__find_by_name_kind(btf, btf_type_name, kind);
> > +}
> > +
> > +static __s32 btf__find_lsm_hook_index(const char *name)
>
> this name is violating libbpf naming guidelines. Just
> `find_lsm_hook_idx` for now?
>
> > +{
> > +       struct btf *btf = bpf_find_kernel_btf();
>
> ok, it's probably time to do this right. Let's ensure we load kernel
> BTF just once, keep it inside bpf_object while we need it and then
> release it after successful load. We are at the point where all the
> new types of program is loading/releasing kernel BTF for every section
> and it starts to feel very wasteful.
>
> > +       const struct bpf_sec_def *sec_def;
> > +       const struct btf_type *hl_type;
> > +       struct btf_member *m;
> > +       __u16 vlen;
> > +       __s32 hl_id;
> > +       int j;
>
> j without having i used anywhere?...
>
> > +
> > +       sec_def = find_sec_def(name);
> > +       if (!sec_def)
> > +               return -ESRCH;
> > +
> > +       name += sec_def->len;
> > +
> > +       hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
> > +                                      BTF_KIND_STRUCT);
> > +       if (hl_id < 0) {
> > +               pr_debug("security_hook_heads cannot be found in BTF\n");
>
> "in vmlinux BTF" ?
>
> and it should be pr_warn(), we don't really expect this, right?
>
> and it should be (hl_id <= 0) with current btf__find_by_name_kind(),
> and then return hl_id ? : -ESRCH, which further proves we need to
> change btf__find_by_name_kind() as I suggested below.
>
> > +               return hl_id;
> > +       }
> > +
> > +       hl_type = btf__type_by_id(btf, hl_id);
> > +       if (!hl_type) {
> > +               pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
> > +               return -EINVAL;
>
> -ESRCH?
>
> > +       }
> > +
> > +       m = btf_members(hl_type);
> > +       vlen = btf_vlen(hl_type);
> > +
> > +       for (j = 0; j < vlen; j++) {
>
> can add succinct `, m++` here instead
>
> > +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > +                       return j + 1;
>
> I looked briefly through kernel-side patch introducing lsm_hook_index,
> but it didn't seem to explain why this index needs to be (unnaturally)
> 1-based. So asking here first as I'm looking through libbpf changes?
>
> > +               m++;
> > +       }
> > +
> > +       pr_warn("Cannot find offset for %s in security_hook_heads\n", name);
>
> it's not offset, rather member index?
>
> > +       return -ENOENT;
>
> not entirely clear about distinction between ENOENT and ESRCH? So far
> we typically used ESRCH, does ENOENT have more specific semantics?
>
> > +}
> > +
> >  int libbpf_find_vmlinux_btf_id(const char *name,
> >                                enum bpf_attach_type attach_type)
> >  {
> >         struct btf *btf = bpf_find_kernel_btf();
> > -       char raw_tp_btf[128] = BTF_PREFIX;
> > -       char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> > -       const char *btf_name;
> >         int err = -EINVAL;
> > -       __u32 kind;
> >
> >         if (IS_ERR(btf)) {
> >                 pr_warn("vmlinux BTF is not found\n");
> >                 return -EINVAL;
> >         }
> >
> > -       if (attach_type == BPF_TRACE_RAW_TP) {
> > -               /* prepend "btf_trace_" prefix per kernel convention */
> > -               strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> > -               btf_name = raw_tp_btf;
> > -               kind = BTF_KIND_TYPEDEF;
> > -       } else {
> > -               btf_name = name;
> > -               kind = BTF_KIND_FUNC;
> > -       }
> > -       err = btf__find_by_name_kind(btf, btf_name, kind);
> > +       if (attach_type == BPF_TRACE_RAW_TP)
> > +               err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
> > +                                              BTF_KIND_TYPEDEF);
> > +       else
> > +               err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > +
> > +       /* err = 0 means void / UNKNOWN which is treated as an error */
> > +       if (err == 0)
> > +               err = -EINVAL;
>
> I think it's actually less error-prone to make btf__find_by_name_kind
> and btf__find_by_prefix_kind to return -ESRCH when type is not found,
> instead of a valid type_id 0. I just checked, and struct_ops code
> already is mishandling it, only checking for <0. Could you make this
> change and just do a natural <0 check everywhere?

ok, re-read btf__find_by_name_kind() source code. It will return
-ENOENT when type is not handled (unless type name is "void", that
part is not clear and should be fixed separately), so this (err == 0)
check shouldn't be here. Disregard my comments about changing return
results of btf__find_by_name_kind(), please.

>
>
> > +
> >         btf__free(btf);
> >         return err;
> >  }
> > @@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> >         }
> >         err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> >         btf__free(btf);
> > -       if (err <= 0) {
> > +       if (err < 0) {

This change is good, thanks!

> >                 pr_warn("%s is not found in prog's BTF\n", name);
> >                 goto out;
> >         }
> > @@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> >         return bpf_program__attach_trace(prog);
> >  }
> >
> > +struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
> > +{
> > +       char errmsg[STRERR_BUFSIZE];
> > +       struct bpf_link_fd *link;
> > +       int prog_fd, pfd;
> > +
> > +       prog_fd = bpf_program__fd(prog);
> > +       if (prog_fd < 0) {
> > +               pr_warn("program '%s': can't attach before loaded\n",
> > +                       bpf_program__title(prog, false));
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       link = calloc(1, sizeof(*link));
> > +       if (!link)
> > +               return ERR_PTR(-ENOMEM);
> > +       link->link.detach = &bpf_link__detach_fd;
> > +
> > +       pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
> > +                             BPF_F_ALLOW_OVERRIDE);
>
> do we want to always specify ALLOW_OVERRIDE? Or should it be an option?
>
> > +       if (pfd < 0) {
> > +               pfd = -errno;
> > +               pr_warn("program '%s': failed to attach: %s\n",
> > +                       bpf_program__title(prog, false),
> > +                       libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > +               return ERR_PTR(pfd);
>
> leaking link here
>
> > +       }
> > +       link->fd = pfd;
> > +       return (struct bpf_link *)link;
> > +}
> > +
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > +                                  struct bpf_program *prog)
> > +{
> > +       return bpf_program__attach_lsm(prog);
> > +}
> > +
> >  struct bpf_link *bpf_program__attach(struct bpf_program *prog)
> >  {
> >         const struct bpf_sec_def *sec_def;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 01639f9a1062..a97e709a29e6 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
> >  bpf_program__attach_trace(struct bpf_program *prog);
> >  struct bpf_map;
> >  LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_lsm(struct bpf_program *prog);
>
> nit: put it after attach_trace, so that program attaches and map
> attaches are grouped together, not intermixed
>
> >  struct bpf_insn;
> >
> >  /*
> > @@ -318,6 +320,7 @@ 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_tracing(struct bpf_program *prog);
> >  LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
> > +LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
> >
> >  LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
> >  LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> > @@ -339,6 +342,7 @@ 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_tracing(const struct bpf_program *prog);
> >  LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
> > +LIBBPF_API bool bpf_program__is_lsm(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 a19f04e6e3d9..3da0452ce679 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
> >                 bpf_program__is_struct_ops;
> >                 bpf_program__set_struct_ops;
> >                 btf__align_of;
> > +               bpf_program__is_lsm;
> > +               bpf_program__set_lsm;
> > +               bpf_program__attach_lsm;
>
> preserve alphabetical order, please
>
> >  } LIBBPF_0.0.6;
>
> > --
> > 2.20.1
> >

Re: [PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI)

[Andrii Nakryiko]

On Wed, Jan 15, 2020 at 9:15 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> # Changes since v1 (https://lore.kernel.org/bpf/20191220154208.15895-1-kpsingh@chromium.org/):
>
> * Eliminate the requirement to maintain LSM hooks separately in
>   security/bpf/hooks.h Use BPF trampolines to dynamically allocate
>   security hooks
> * Drop the use of securityfs as bpftool provides the required
>   introspection capabilities.  Update the tests to use the bpf_skeleton
>   and global variables
> * Use O_CLOEXEC anonymous fds to represent BPF attachment in line with
>   the other BPF programs with the possibility to use bpf program pinning
>   in the future to provide "permanent attachment".
> * Drop the logic based on prog names for handling re-attachment.
> * Drop bpf_lsm_event_output from this series and send it as a separate
>   patch.
>
> # Motivation
>
> Google does analysis of rich runtime security data to detect and thwart
> threats in real-time. Currently, this is done in custom kernel modules
> but we would like to replace this with something that's upstream and
> useful to others.
>
> The current kernel infrastructure for providing telemetry (Audit, Perf
> etc.) is disjoint from access enforcement (i.e. LSMs).  Augmenting the
> 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 telemetry data requires changes to
> various LSMs and their respective policy languages.
>
> This patchset proposes a new stackable and privileged LSM which allows
> the LSM hooks to be implemented using eBPF. This facilitates a unified
> and dynamic (not requiring re-compilation of the kernel) audit and MAC
> policy.
>
> # Why an LSM?
>
> Linux Security Modules target security behaviours rather than the
> kernel's API. For example, it's easy to miss out a newly added system
> call for executing processes (eg. execve, execveat etc.) but the LSM
> framework ensures that all process executions trigger the relevant hooks
> irrespective of how the process was executed.
>
> Allowing users to implement LSM hooks at runtime also benefits the LSM
> eco-system by enabling a quick feedback loop from the security community
> about the kind of behaviours that the LSM Framework should be targeting.
>
> # How does it work?
>
> The LSM introduces a new eBPF (https://docs.cilium.io/en/v1.6/bpf/)
> program type BPF_PROG_TYPE_LSM which can only be attached to LSM hooks.
> Attachment requires CAP_SYS_ADMIN for loading eBPF programs and
> CAP_MAC_ADMIN for modifying MAC policies.
>
> The eBPF programs are attached to a separate security_hook_heads
> maintained by the BPF LSM for mutable hooks and executed after all the
> statically defined hooks (i.e. the ones declared by SELinux, AppArmor,
> Smack etc). This also ensures that statically defined LSM hooks retain
> the behaviour of "being read-only after init", i.e. __lsm_ro_after_init.
>
> Upon attachment, a security hook is dynamically allocated with
> arch_bpf_prepare_trampoline which generates code to handle the
> conversion from the signature of the hook to the BPF context and allows
> the JIT'ed BPF program to be called as a C function with the same
> arguments as the LSM hooks. If any of the attached eBPF programs returns
> an error (like ENOPERM), the behaviour represented by the hook is
> denied.
>
> Audit logs can be written using a format chosen by the eBPF program to
> the perf events buffer or to global eBPF variables or maps and can be
> further processed in user-space.
>
> # BTF Based Design
>
> The current design uses BTF
> (https://facebookmicrosites.github.io/bpf/blog/2018/11/14/btf-enhancement.html,
> https://lwn.net/Articles/803258/) which allows verifiable read-only
> structure accesses by field names rather than fixed offsets. This allows
> accessing the hook parameters using a dynamically created context which
> provides a certain degree of ABI stability:
>
>
> // Only declare the structure and fields intended to be used
> // in the program
> struct vm_area_struct {
>         unsigned long vm_start;
> } __attribute__((preserve_access_index));
>

It would be nice to also mention that you don't even have to
"re-define" these structs if you use vmlinux.h generated with `bpftool
btf dump file <path-to-vm-linux-or-/sys/kernel/btf/vmlinux> format c`.
Its output will contain all types of the kernel, including internal
ones not exposed through any public headers. And it will also
automatically have __attribute__((preserve_access_index)) applied to
all structs/unions. It can be pre-generated and checked in somewhere
along the application or generated on the fly, if environment and use
case allows.

> // Declare the eBPF program mprotect_audit which attaches to
> // to the file_mprotect LSM hook and accepts three arguments.
> SEC("lsm/file_mprotect")
> int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
>              unsigned long reqprot, unsigned long prot)
> {
>         unsigned long vm_start = vma->vm_start;
>
>         return 0;
> }
>
> By relocating field offsets, BTF makes a large portion of kernel data
> structures readily accessible across kernel versions without requiring a
> large corpus of BPF helper functions and requiring recompilation with
> every kernel version. The BTF type information is also used by the BPF
> verifier to validate memory accesses within the BPF program and also
> prevents arbitrary writes to the kernel memory.
>
> The limitations of BTF compatibility are described in BPF Co-Re
> (http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf, i.e. field
> renames, #defines and changes to the signature of LSM hooks).
>
> This design imposes that the MAC policy (eBPF programs) be updated when
> the inspected kernel structures change outside of BTF compatibility
> guarantees. In practice, this is only required when a structure field
> used by a current policy is removed (or renamed) or when the used LSM
> hooks change. We expect the maintenance cost of these changes to be
> acceptable as compared to the previous design
> (https://lore.kernel.org/bpf/20190910115527.5235-1-kpsingh@chromium.org/).
>

[...]

Re: [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[Casey Schaufler]

On 1/15/2020 9:13 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> - The list of hooks registered by an LSM is currently immutable as they
>   are declared with __lsm_ro_after_init and they are attached to a
>   security_hook_heads struct.
> - For the BPF LSM we need to de/register the hooks at runtime. Making
>   the existing security_hook_heads mutable broadens an
>   attack vector, so a separate security_hook_heads is added for only
>   those that ~must~ be mutable.
> - These mutable hooks are run only after all the static hooks have
>   successfully executed.
>
> This is based on the ideas discussed in:
>
>   https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  MAINTAINERS             |  1 +
>  include/linux/bpf_lsm.h | 71 +++++++++++++++++++++++++++++++++++++++++
>  security/bpf/Kconfig    |  1 +
>  security/bpf/Makefile   |  2 +-
>  security/bpf/hooks.c    | 20 ++++++++++++
>  security/bpf/lsm.c      |  9 +++++-
>  security/security.c     | 24 +++++++-------
>  7 files changed, 115 insertions(+), 13 deletions(-)
>  create mode 100644 include/linux/bpf_lsm.h
>  create mode 100644 security/bpf/hooks.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0941f478cfa5..02d7e05e9b75 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3209,6 +3209,7 @@ L:	linux-security-module@vger.kernel.org
>  L:	bpf@vger.kernel.org
>  S:	Maintained
>  F:	security/bpf/
> +F:	include/linux/bpf_lsm.h
>  
>  BROADCOM B44 10/100 ETHERNET DRIVER
>  M:	Michael Chan <michael.chan@broadcom.com>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> new file mode 100644
> index 000000000000..9883cf25241c
> --- /dev/null
> +++ b/include/linux/bpf_lsm.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright 2019 Google LLC.
> + */
> +
> +#ifndef _LINUX_BPF_LSM_H
> +#define _LINUX_BPF_LSM_H
> +
> +#include <linux/bpf.h>
> +#include <linux/lsm_hooks.h>
> +
> +#ifdef CONFIG_SECURITY_BPF
> +
> +/* Mutable hooks defined at runtime and executed after all the statically
> + * define LSM hooks.
> + */
> +extern struct security_hook_heads bpf_lsm_hook_heads;
> +
> +int bpf_lsm_srcu_read_lock(void);
> +void bpf_lsm_srcu_read_unlock(int idx);
> +
> +#define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)			\
> +	do {							\
> +		struct security_hook_list *P;			\
> +		int _idx;					\
> +								\
> +		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
> +			break;					\
> +								\
> +		_idx = bpf_lsm_srcu_read_lock();		\
> +		hlist_for_each_entry(P, &bpf_lsm_hook_heads.FUNC, list) \
> +			P->hook.FUNC(__VA_ARGS__);		\
> +		bpf_lsm_srcu_read_unlock(_idx);			\
> +	} while (0)
> +
> +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) ({		\
> +	do {							\
> +		struct security_hook_list *P;			\
> +		int _idx;					\
> +								\
> +		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
> +			break;					\
> +								\
> +		_idx = bpf_lsm_srcu_read_lock();		\
> +								\
> +		hlist_for_each_entry(P,				\
> +			&bpf_lsm_hook_heads.FUNC, list) {	\
> +			RC = P->hook.FUNC(__VA_ARGS__);		\
> +			if (RC && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> +				break;				\
> +		}						\
> +		bpf_lsm_srcu_read_unlock(_idx);			\
> +	} while (0);						\
> +	IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? RC : 0;	\
> +})
> +
> +#else /* !CONFIG_SECURITY_BPF */
> +
> +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) (RC)
> +#define CALL_BPF_LSM_VOID_HOOKS(...)
> +
> +static inline int bpf_lsm_srcu_read_lock(void)
> +{
> +	return 0;
> +}
> +static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> +
> +#endif /* CONFIG_SECURITY_BPF */
> +
> +#endif /* _LINUX_BPF_LSM_H */
> diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> index a5f6c67ae526..595e4ad597ae 100644
> --- a/security/bpf/Kconfig
> +++ b/security/bpf/Kconfig
> @@ -6,6 +6,7 @@ config SECURITY_BPF
>  	bool "BPF-based MAC and audit policy"
>  	depends on SECURITY
>  	depends on BPF_SYSCALL
> +	depends on SRCU
>  	help
>  	  This enables instrumentation of the security hooks with
>  	  eBPF programs.
> diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> index c78a8a056e7e..c526927c337d 100644
> --- a/security/bpf/Makefile
> +++ b/security/bpf/Makefile
> @@ -2,4 +2,4 @@
>  #
>  # Copyright 2019 Google LLC.
>  
> -obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o
> +obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> new file mode 100644
> index 000000000000..b123d9cb4cd4
> --- /dev/null
> +++ b/security/bpf/hooks.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2019 Google LLC.
> + */
> +
> +#include <linux/bpf_lsm.h>
> +#include <linux/srcu.h>
> +
> +DEFINE_STATIC_SRCU(security_hook_srcu);
> +
> +int bpf_lsm_srcu_read_lock(void)
> +{
> +	return srcu_read_lock(&security_hook_srcu);
> +}
> +
> +void bpf_lsm_srcu_read_unlock(int idx)
> +{
> +	return srcu_read_unlock(&security_hook_srcu, idx);
> +}
> diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> index 5c5c14f990ce..d4ea6aa9ddb8 100644
> --- a/security/bpf/lsm.c
> +++ b/security/bpf/lsm.c
> @@ -4,14 +4,21 @@
>   * Copyright 2019 Google LLC.
>   */
>  
> +#include <linux/bpf_lsm.h>
>  #include <linux/lsm_hooks.h>
>  
>  /* This is only for internal hooks, always statically shipped as part of the
> - * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> + * BPF LSM. Statically defined hooks are appended to the security_hook_heads
>   * which is common for LSMs and R/O after init.
>   */
>  static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
>  
> +/* Security hooks registered dynamically by the BPF LSM and must be accessed
> + * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock. The mutable
> + * hooks dynamically allocated by the BPF LSM are appeneded here.
> + */
> +struct security_hook_heads bpf_lsm_hook_heads;
> +
>  static int __init lsm_init(void)
>  {
>  	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> diff --git a/security/security.c b/security/security.c
> index cd2d18d2d279..4a2eb4c089b2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -27,6 +27,7 @@
>  #include <linux/backing-dev.h>
>  #include <linux/string.h>
>  #include <linux/msg.h>
> +#include <linux/bpf_lsm.h>
>  #include <net/flow.h>
>  
>  #define MAX_LSM_EVM_XATTR	2
> @@ -652,20 +653,21 @@ static void __init lsm_early_task(struct task_struct *task)
>  								\
>  		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
>  			P->hook.FUNC(__VA_ARGS__);		\
> +		CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__);	\
>  	} while (0)
>  
> -#define call_int_hook(FUNC, IRC, ...) ({			\
> -	int RC = IRC;						\
> -	do {							\
> -		struct security_hook_list *P;			\
> -								\
> +#define call_int_hook(FUNC, IRC, ...) ({				\
> +	int RC = IRC;							\
> +	do {								\
> +		struct security_hook_list *P;				\
>  		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> -			RC = P->hook.FUNC(__VA_ARGS__);		\
> -			if (RC != 0)				\
> -				break;				\
> -		}						\
> -	} while (0);						\
> -	RC;							\
> +			RC = P->hook.FUNC(__VA_ARGS__);			\
> +			if (RC != 0)					\
> +				break;					\
> +		}							\
> +		RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__);	\

Do not do this. Add LSM_ORDER_LAST for the lsm_order field of lsm_info
and use that to identify your module as one to be put on the list last.
Update the LSM registration code to do this. It will be much like the code
that uses LSM_ORDER_FIRST to put the capabilities at the head of the lists.

What you have here to way to much like the way Yama was invoked before
stacking.

> +	} while (0);							\
> +	RC;								\
>  })
>  
>  /* Security operations */

Re: [PATCH bpf-next v2 02/10] bpf: lsm: Add a skeleton and config options

[Casey Schaufler]

On 1/15/2020 9:13 AM, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> The LSM can be enabled by CONFIG_SECURITY_BPF.
> Without CONFIG_SECURITY_BPF_ENFORCE, the LSM will run the
> attached eBPF programs but not enforce MAC policy based
> on the return value of the attached programs.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  MAINTAINERS           |  7 +++++++
>  security/Kconfig      | 11 ++++++-----
>  security/Makefile     |  2 ++
>  security/bpf/Kconfig  | 22 ++++++++++++++++++++++
>  security/bpf/Makefile |  5 +++++
>  security/bpf/lsm.c    | 25 +++++++++++++++++++++++++
>  6 files changed, 67 insertions(+), 5 deletions(-)
>  create mode 100644 security/bpf/Kconfig
>  create mode 100644 security/bpf/Makefile
>  create mode 100644 security/bpf/lsm.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 66a2e5e07117..0941f478cfa5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3203,6 +3203,13 @@ S:	Supported
>  F:	arch/x86/net/
>  X:	arch/x86/net/bpf_jit_comp32.c
>  
> +BPF SECURITY MODULE
> +M:	KP Singh <kpsingh@chromium.org>
> +L:	linux-security-module@vger.kernel.org
> +L:	bpf@vger.kernel.org
> +S:	Maintained
> +F:	security/bpf/
> +
>  BROADCOM B44 10/100 ETHERNET DRIVER
>  M:	Michael Chan <michael.chan@broadcom.com>
>  L:	netdev@vger.kernel.org
> diff --git a/security/Kconfig b/security/Kconfig
> index 2a1a2d396228..6f1aab195e7d 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/bpf/Kconfig"
>  source "security/safesetid/Kconfig"
>  source "security/lockdown/Kconfig"
>  
> @@ -277,11 +278,11 @@ endchoice
>  
>  config LSM
>  	string "Ordered list of enabled LSMs"
> -	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
> -	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
> -	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> -	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
> -	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> +	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> +	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> +	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> +	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
> +	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
>  	help
>  	  A comma-separated list of LSMs, in initialization order.
>  	  Any LSMs left off this list will be ignored. This can be
> diff --git a/security/Makefile b/security/Makefile
> index be1dd9d2cb2f..50e6821dd7b7 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA)		+= yama
>  subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
>  subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
>  subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
> +subdir-$(CONFIG_SECURITY_BPF)		+= bpf
>  
>  # always enable default capabilities
>  obj-y					+= commoncap.o
> @@ -29,6 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA)		+= yama/
>  obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
>  obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
>  obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
> +obj-$(CONFIG_SECURITY_BPF)		+= bpf/
>  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
>  
>  # Object integrity file lists
> diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> new file mode 100644
> index 000000000000..a5f6c67ae526
> --- /dev/null
> +++ b/security/bpf/Kconfig
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright 2019 Google LLC.
> +
> +config SECURITY_BPF
> +	bool "BPF-based MAC and audit policy"
> +	depends on SECURITY
> +	depends on BPF_SYSCALL
> +	help
> +	  This enables instrumentation of the security hooks with
> +	  eBPF programs.
> +
> +	  If you are unsure how to answer this question, answer N.
> +
> +config SECURITY_BPF_ENFORCE
> +	bool "Deny operations based on the evaluation of the attached programs"
> +	depends on SECURITY_BPF
> +	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 or deny the operation
> +	  guarded by the security hook.
> diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> new file mode 100644
> index 000000000000..26a0ab6f99b7
> --- /dev/null
> +++ b/security/bpf/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright 2019 Google LLC.
> +
> +obj-$(CONFIG_SECURITY_BPF) := lsm.o
> diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> new file mode 100644
> index 000000000000..5c5c14f990ce
> --- /dev/null
> +++ b/security/bpf/lsm.c
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2019 Google LLC.
> + */
> +
> +#include <linux/lsm_hooks.h>
> +
> +/* This is only for internal hooks, always statically shipped as part of the
> + * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> + * which is common for LSMs and R/O after init.
> + */
> +static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};

s/lsm_hooks/bpf_hooks/

The lsm prefix is for the infrastructure. The way you have it is massively confusing.

> +
> +static int __init lsm_init(void)

s/lsm_init/bpf_init/

Same reason. When I'm looking at several security modules at once I
need to be able to tell them apart.

> +{
> +	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> +	pr_info("eBPF and LSM are friends now.\n");

Cute message, but not very informative if you haven't read the code.
"LSM support for eBPF active\n" is more likely to be comprehensible.

> +	return 0;
> +}
> +
> +DEFINE_LSM(bpf) = {
> +	.name = "bpf",
> +	.init = lsm_init,
> +};

Re: [PATCH bpf-next v2 06/10] bpf: lsm: Implement attach, detach and execution

[KP Singh]

On 15-Jan 18:24, Greg Kroah-Hartman wrote:
> On Wed, Jan 15, 2020 at 06:13:29PM +0100, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> > 
> > JITed BPF programs are used by the BPF LSM as dynamically allocated
> > security hooks. arch_bpf_prepare_trampoline handles the
> > arch_bpf_prepare_trampoline generates code to handle conversion of the
> > signature of the hook to the BPF context and allows the BPF program to
> > be called directly as a C function.
> > 
> > The following permissions are required to attach a program to a hook:
> > 
> > - CAP_SYS_ADMIN to load the program
> > - CAP_MAC_ADMIN to attach it (i.e. to update the security policy)
> 
> You forgot to list "GPL-compatible license" here :)

Added it to the commit log for v3.

> 
> Anyway, looks good to me:

Thanks! :)

> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[KP Singh]

On 15-Jan 12:30, Stephen Smalley wrote:
> On 1/15/20 12:13 PM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> > 
> > - The list of hooks registered by an LSM is currently immutable as they
> >    are declared with __lsm_ro_after_init and they are attached to a
> >    security_hook_heads struct.
> > - For the BPF LSM we need to de/register the hooks at runtime. Making
> >    the existing security_hook_heads mutable broadens an
> >    attack vector, so a separate security_hook_heads is added for only
> >    those that ~must~ be mutable.
> > - These mutable hooks are run only after all the static hooks have
> >    successfully executed.
> > 
> > This is based on the ideas discussed in:
> > 
> >    https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
> > 
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> [...]
> > diff --git a/security/security.c b/security/security.c
> > index cd2d18d2d279..4a2eb4c089b2 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -652,20 +653,21 @@ static void __init lsm_early_task(struct task_struct *task)
> >   								\
> >   		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> >   			P->hook.FUNC(__VA_ARGS__);		\
> > +		CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__);	\
> >   	} while (0)
> > -#define call_int_hook(FUNC, IRC, ...) ({			\
> > -	int RC = IRC;						\
> > -	do {							\
> > -		struct security_hook_list *P;			\
> > -								\
> > +#define call_int_hook(FUNC, IRC, ...) ({				\
> > +	int RC = IRC;							\
> > +	do {								\
> > +		struct security_hook_list *P;				\
> >   		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > -			RC = P->hook.FUNC(__VA_ARGS__);		\
> > -			if (RC != 0)				\
> > -				break;				\
> > -		}						\
> > -	} while (0);						\
> > -	RC;							\
> > +			RC = P->hook.FUNC(__VA_ARGS__);			\
> > +			if (RC != 0)					\
> > +				break;					\
> > +		}							\
> > +		RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__);	\
> 
> Let's not clobber the return code from the other LSMs with the bpf one.

Good catch and thanks for pointing it out. Should be fixed in v3.

- KP


> 
> > +	} while (0);							\
> > +	RC;								\
> >   })
> >   /* Security operations */
> > 
> 

Re: [PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI)

[Brendan Jackman]

On Wed, Jan 15 2020 at 18:13, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
[...]
> KP Singh (10):
>   bpf: btf: Make some of the API visible outside BTF
>   bpf: lsm: Add a skeleton and config options
>   bpf: lsm: Introduce types for eBPF based LSM
>   bpf: lsm: Add mutable hooks list for the BPF LSM
>   bpf: lsm: BTF API for LSM hooks
>   bpf: lsm: Implement attach, detach and execution
>   bpf: lsm: Make the allocated callback RO+X
>   tools/libbpf: Add support for BPF_PROG_TYPE_LSM
>   bpf: lsm: Add selftests for BPF_PROG_TYPE_LSM
>   bpf: lsm: Add Documentation

Just to note internal review cycles at Google; For the whole set:

Reviewed-by: Brendan Jackman <jackmanb@chromium.org>

Re: [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[KP Singh]

On 15-Jan 22:33, Casey Schaufler wrote:
> On 1/15/2020 9:13 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > - The list of hooks registered by an LSM is currently immutable as they
> >   are declared with __lsm_ro_after_init and they are attached to a
> >   security_hook_heads struct.
> > - For the BPF LSM we need to de/register the hooks at runtime. Making
> >   the existing security_hook_heads mutable broadens an
> >   attack vector, so a separate security_hook_heads is added for only
> >   those that ~must~ be mutable.
> > - These mutable hooks are run only after all the static hooks have
> >   successfully executed.
> >
> > This is based on the ideas discussed in:
> >
> >   https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  MAINTAINERS             |  1 +
> >  include/linux/bpf_lsm.h | 71 +++++++++++++++++++++++++++++++++++++++++
> >  security/bpf/Kconfig    |  1 +
> >  security/bpf/Makefile   |  2 +-
> >  security/bpf/hooks.c    | 20 ++++++++++++
> >  security/bpf/lsm.c      |  9 +++++-
> >  security/security.c     | 24 +++++++-------
> >  7 files changed, 115 insertions(+), 13 deletions(-)
> >  create mode 100644 include/linux/bpf_lsm.h
> >  create mode 100644 security/bpf/hooks.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0941f478cfa5..02d7e05e9b75 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3209,6 +3209,7 @@ L:	linux-security-module@vger.kernel.org
> >  L:	bpf@vger.kernel.org
> >  S:	Maintained
> >  F:	security/bpf/
> > +F:	include/linux/bpf_lsm.h
> >  
> >  BROADCOM B44 10/100 ETHERNET DRIVER
> >  M:	Michael Chan <michael.chan@broadcom.com>
> > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > new file mode 100644
> > index 000000000000..9883cf25241c
> > --- /dev/null
> > +++ b/include/linux/bpf_lsm.h
> > @@ -0,0 +1,71 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * Copyright 2019 Google LLC.
> > + */
> > +
> > +#ifndef _LINUX_BPF_LSM_H
> > +#define _LINUX_BPF_LSM_H
> > +
> > +#include <linux/bpf.h>
> > +#include <linux/lsm_hooks.h>
> > +
> > +#ifdef CONFIG_SECURITY_BPF
> > +
> > +/* Mutable hooks defined at runtime and executed after all the statically
> > + * define LSM hooks.
> > + */
> > +extern struct security_hook_heads bpf_lsm_hook_heads;
> > +
> > +int bpf_lsm_srcu_read_lock(void);
> > +void bpf_lsm_srcu_read_unlock(int idx);
> > +
> > +#define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)			\
> > +	do {							\
> > +		struct security_hook_list *P;			\
> > +		int _idx;					\
> > +								\
> > +		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
> > +			break;					\
> > +								\
> > +		_idx = bpf_lsm_srcu_read_lock();		\
> > +		hlist_for_each_entry(P, &bpf_lsm_hook_heads.FUNC, list) \
> > +			P->hook.FUNC(__VA_ARGS__);		\
> > +		bpf_lsm_srcu_read_unlock(_idx);			\
> > +	} while (0)
> > +
> > +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) ({		\
> > +	do {							\
> > +		struct security_hook_list *P;			\
> > +		int _idx;					\
> > +								\
> > +		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
> > +			break;					\
> > +								\
> > +		_idx = bpf_lsm_srcu_read_lock();		\
> > +								\
> > +		hlist_for_each_entry(P,				\
> > +			&bpf_lsm_hook_heads.FUNC, list) {	\
> > +			RC = P->hook.FUNC(__VA_ARGS__);		\
> > +			if (RC && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> > +				break;				\
> > +		}						\
> > +		bpf_lsm_srcu_read_unlock(_idx);			\
> > +	} while (0);						\
> > +	IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? RC : 0;	\
> > +})
> > +
> > +#else /* !CONFIG_SECURITY_BPF */
> > +
> > +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) (RC)
> > +#define CALL_BPF_LSM_VOID_HOOKS(...)
> > +
> > +static inline int bpf_lsm_srcu_read_lock(void)
> > +{
> > +	return 0;
> > +}
> > +static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> > +
> > +#endif /* CONFIG_SECURITY_BPF */
> > +
> > +#endif /* _LINUX_BPF_LSM_H */
> > diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> > index a5f6c67ae526..595e4ad597ae 100644
> > --- a/security/bpf/Kconfig
> > +++ b/security/bpf/Kconfig
> > @@ -6,6 +6,7 @@ config SECURITY_BPF
> >  	bool "BPF-based MAC and audit policy"
> >  	depends on SECURITY
> >  	depends on BPF_SYSCALL
> > +	depends on SRCU
> >  	help
> >  	  This enables instrumentation of the security hooks with
> >  	  eBPF programs.
> > diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> > index c78a8a056e7e..c526927c337d 100644
> > --- a/security/bpf/Makefile
> > +++ b/security/bpf/Makefile
> > @@ -2,4 +2,4 @@
> >  #
> >  # Copyright 2019 Google LLC.
> >  
> > -obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o
> > +obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o
> > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> > new file mode 100644
> > index 000000000000..b123d9cb4cd4
> > --- /dev/null
> > +++ b/security/bpf/hooks.c
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright 2019 Google LLC.
> > + */
> > +
> > +#include <linux/bpf_lsm.h>
> > +#include <linux/srcu.h>
> > +
> > +DEFINE_STATIC_SRCU(security_hook_srcu);
> > +
> > +int bpf_lsm_srcu_read_lock(void)
> > +{
> > +	return srcu_read_lock(&security_hook_srcu);
> > +}
> > +
> > +void bpf_lsm_srcu_read_unlock(int idx)
> > +{
> > +	return srcu_read_unlock(&security_hook_srcu, idx);
> > +}
> > diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> > index 5c5c14f990ce..d4ea6aa9ddb8 100644
> > --- a/security/bpf/lsm.c
> > +++ b/security/bpf/lsm.c
> > @@ -4,14 +4,21 @@
> >   * Copyright 2019 Google LLC.
> >   */
> >  
> > +#include <linux/bpf_lsm.h>
> >  #include <linux/lsm_hooks.h>
> >  
> >  /* This is only for internal hooks, always statically shipped as part of the
> > - * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> > + * BPF LSM. Statically defined hooks are appended to the security_hook_heads
> >   * which is common for LSMs and R/O after init.
> >   */
> >  static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
> >  
> > +/* Security hooks registered dynamically by the BPF LSM and must be accessed
> > + * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock. The mutable
> > + * hooks dynamically allocated by the BPF LSM are appeneded here.
> > + */
> > +struct security_hook_heads bpf_lsm_hook_heads;
> > +
> >  static int __init lsm_init(void)
> >  {
> >  	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> > diff --git a/security/security.c b/security/security.c
> > index cd2d18d2d279..4a2eb4c089b2 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/backing-dev.h>
> >  #include <linux/string.h>
> >  #include <linux/msg.h>
> > +#include <linux/bpf_lsm.h>
> >  #include <net/flow.h>
> >  
> >  #define MAX_LSM_EVM_XATTR	2
> > @@ -652,20 +653,21 @@ static void __init lsm_early_task(struct task_struct *task)
> >  								\
> >  		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> >  			P->hook.FUNC(__VA_ARGS__);		\
> > +		CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__);	\
> >  	} while (0)
> >  
> > -#define call_int_hook(FUNC, IRC, ...) ({			\
> > -	int RC = IRC;						\
> > -	do {							\
> > -		struct security_hook_list *P;			\
> > -								\
> > +#define call_int_hook(FUNC, IRC, ...) ({				\
> > +	int RC = IRC;							\
> > +	do {								\
> > +		struct security_hook_list *P;				\
> >  		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > -			RC = P->hook.FUNC(__VA_ARGS__);		\
> > -			if (RC != 0)				\
> > -				break;				\
> > -		}						\
> > -	} while (0);						\
> > -	RC;							\
> > +			RC = P->hook.FUNC(__VA_ARGS__);			\
> > +			if (RC != 0)					\
> > +				break;					\
> > +		}							\
> > +		RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__);	\
> 
> Do not do this. Add LSM_ORDER_LAST for the lsm_order field of lsm_info
> and use that to identify your module as one to be put on the list last.
> Update the LSM registration code to do this. It will be much like the code
> that uses LSM_ORDER_FIRST to put the capabilities at the head of the lists.
> 
> What you have here to way to much like the way Yama was invoked before
> stacking.

Thanks for taking a look!

The BPF LSM has two kinds of hooks. The static hooks which are
appended to the security_hook_heads in security.c and mutable hooks
which are allocated at runtime and attached to a separate
security_hook_heads (bpf_lsm_hook_heads) defined in security/bpf/lsm.c
This macro runs these mutable hooks (dynamically allocated at runtime)
under SRCU protection.

Having a separate security_hook_heads allows:

- The security_hook_heads in security.c to be __lsm_ro_after_init
  and thus limits the attack surface does not change the existing
  behaviour
- The SRCU critical section be limited to the execution of dynamically
  allocated hooks

I agree that the static hooks should indeed be LSM_ORDER_LAST which
makes logical sense and represents correctly how the LSM will behave
in reality. (i.e. its hooks are executed last irrespective of the
position it is mentioned in the list of LSMs.)

I will update this for v3.

- KP

> 
> > +	} while (0);							\
> > +	RC;								\
> >  })
> >  
> >  /* Security operations */
> 

Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[KP Singh]

Thanks for the review Andrii!

I will incorporate the fixes in the next revision.

On 15-Jan 13:19, Andrii Nakryiko wrote:
> On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> >   attr->lsm_hook_index
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  tools/lib/bpf/bpf.c      |   6 +-
> >  tools/lib/bpf/bpf.h      |   1 +
> >  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
> >  tools/lib/bpf/libbpf.h   |   4 ++
> >  tools/lib/bpf/libbpf.map |   3 +
> >  5 files changed, 138 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 500afe478e94..b138d98ff862 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> >         memset(&attr, 0, sizeof(attr));
> >         attr.prog_type = load_attr->prog_type;
> >         attr.expected_attach_type = load_attr->expected_attach_type;
> > -       if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +
> > +       if (attr.prog_type == BPF_PROG_TYPE_LSM) {
> > +               attr.lsm_hook_index = load_attr->lsm_hook_index;
> > +       } else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> >                 attr.attach_btf_id = load_attr->attach_btf_id;
> >         } else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
> >                 attr.attach_btf_id = load_attr->attach_btf_id;
> > @@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> >                 attr.prog_ifindex = load_attr->prog_ifindex;
> >                 attr.kern_version = load_attr->kern_version;
> >         }
> > +
> >         attr.insn_cnt = (__u32)load_attr->insns_cnt;
> >         attr.insns = ptr_to_u64(load_attr->insns);
> >         attr.license = ptr_to_u64(load_attr->license);
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 56341d117e5b..54458a102939 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -86,6 +86,7 @@ struct bpf_load_program_attr {
> >                 __u32 prog_ifindex;
> >                 __u32 attach_btf_id;
> >         };
> > +       __u32 lsm_hook_index;
> 
> 
> this is changing memory layout of struct bpf_load_program_attr, which
> is part of public API, so breaking backward compatibility. But I think
> you intended to put it inside union along the attach_btf_id?
> 

Correct, I moved it to the union.

> also, we use idx for index pretty consistently (apart from ifindex),
> so maybe lsm_hook_idx?

Renamed all usages of index -> idx.

> 
> >         __u32 prog_btf_fd;
> >         __u32 func_info_rec_size;
> >         const void *func_info;
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 0c229f00a67e..60737559a9a6 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -229,6 +229,7 @@ struct bpf_program {
> >         enum bpf_attach_type expected_attach_type;
> >         __u32 attach_btf_id;
> >         __u32 attach_prog_fd;
> > +       __u32 lsm_hook_index
> >         void *func_info;
> >         __u32 func_info_rec_size;
> >         __u32 func_info_cnt;
> > @@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >         load_attr.insns = insns;
> >         load_attr.insns_cnt = insns_cnt;
> >         load_attr.license = license;
> > -       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +
> > +       if (prog->type == BPF_PROG_TYPE_LSM) {
> > +               load_attr.lsm_hook_index = prog->lsm_hook_index;
> > +       } else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> >                 load_attr.attach_btf_id = prog->attach_btf_id;
> >         } else if (prog->type == BPF_PROG_TYPE_TRACING) {
> >                 load_attr.attach_prog_fd = prog->attach_prog_fd;
> > @@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >                 load_attr.kern_version = kern_version;
> >                 load_attr.prog_ifindex = prog->prog_ifindex;
> >         }
> > +
> >         /* if .BTF.ext was loaded, kernel supports associated BTF for prog */
> >         if (prog->obj->btf_ext)
> >                 btf_fd = bpf_object__btf_fd(prog->obj);
> > @@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
> >                                      enum bpf_attach_type attach_type,
> >                                      __u32 attach_prog_fd);
> >
> > +static __s32 btf__find_lsm_hook_index(const char *name);
> > +
> >  int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> >  {
> > -       int err = 0, fd, i, btf_id;
> > +       int err = 0, fd, i, btf_id, index;
> >
> >         if (prog->type == BPF_PROG_TYPE_TRACING) {
> >                 btf_id = libbpf_find_attach_btf_id(prog->section_name,
> > @@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> >                 prog->attach_btf_id = btf_id;
> >         }
> >
> > +       if (prog->type == BPF_PROG_TYPE_LSM) {
> > +               index = btf__find_lsm_hook_index(prog->section_name);
> > +               if (index < 0)
> > +                       return index;
> > +               prog->lsm_hook_index = index;
> > +       }
> > +
> >         if (prog->instances.nr < 0 || !prog->instances.fds) {
> >                 if (prog->preprocessor) {
> >                         pr_warn("Internal error: can't load program '%s'\n",
> > @@ -6207,6 +6221,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(lsm, BPF_PROG_TYPE_LSM);
> >  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);
> > @@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
> >                                       struct bpf_program *prog);
> >  static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> >                                      struct bpf_program *prog);
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > +                                  struct bpf_program *prog);
> >
> >  struct bpf_sec_def {
> >         const char *sec;
> > @@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
> >                 .expected_attach_type = BPF_TRACE_FEXIT,
> >                 .is_attach_btf = true,
> >                 .attach_fn = attach_trace),
> > +       SEC_DEF("lsm/", LSM,
> > +               .expected_attach_type = BPF_LSM_MAC,
> > +               .attach_fn = attach_lsm),
> >         BPF_PROG_SEC("xdp",                     BPF_PROG_TYPE_XDP),
> >         BPF_PROG_SEC("perf_event",              BPF_PROG_TYPE_PERF_EVENT),
> >         BPF_PROG_SEC("lwt_in",                  BPF_PROG_TYPE_LWT_IN),
> >         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_PROG_BTF("lsm/",                    BPF_PROG_TYPE_LSM,
> > +                                               BPF_LSM_MAC),
> 
> This is just a duplicate of SEC_DEF above, remove?

Oops. Removed.

> 
> >         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,
> > @@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
> >         return -EINVAL;
> >  }
> >
> > -#define BTF_PREFIX "btf_trace_"
> > +#define BTF_TRACE_PREFIX "btf_trace_"
> > +
> > +static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
> > +                                          const char *prefix, __u32 kind)
> 
> this is internal helper, not really BTF API, let's call it
> find_btf_by_prefix_kind? Also const char *prefix more logically should
> go before name argument?

Done.

> 
> > +{
> > +       char btf_type_name[128];
> > +
> > +       snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
> 
> check overflow?

Done.

> 
> > +       return btf__find_by_name_kind(btf, btf_type_name, kind);
> > +}
> > +
> > +static __s32 btf__find_lsm_hook_index(const char *name)
> 
> this name is violating libbpf naming guidelines. Just
> `find_lsm_hook_idx` for now?

Cool. I think I finally hang of the naming convention now :). I mixed
up btf.c and libbpf.c here.

> 
> > +{
> > +       struct btf *btf = bpf_find_kernel_btf();
> 
> ok, it's probably time to do this right. Let's ensure we load kernel
> BTF just once, keep it inside bpf_object while we need it and then
> release it after successful load. We are at the point where all the
> new types of program is loading/releasing kernel BTF for every section
> and it starts to feel very wasteful.

Sure, will give it a shot in v3.

> 
> > +       const struct bpf_sec_def *sec_def;
> > +       const struct btf_type *hl_type;
> > +       struct btf_member *m;
> > +       __u16 vlen;
> > +       __s32 hl_id;
> > +       int j;
> 
> j without having i used anywhere?...

Fixed.

> 
> > +
> > +       sec_def = find_sec_def(name);
> > +       if (!sec_def)
> > +               return -ESRCH;
> > +
> > +       name += sec_def->len;
> > +
> > +       hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
> > +                                      BTF_KIND_STRUCT);
> > +       if (hl_id < 0) {
> > +               pr_debug("security_hook_heads cannot be found in BTF\n");
> 
> "in vmlinux BTF" ?
> 
> and it should be pr_warn(), we don't really expect this, right?

Fixed both.

> 
> and it should be (hl_id <= 0) with current btf__find_by_name_kind(),
> and then return hl_id ? : -ESRCH, which further proves we need to
> change btf__find_by_name_kind() as I suggested below.
> 
> > +               return hl_id;
> > +       }
> > +
> > +       hl_type = btf__type_by_id(btf, hl_id);
> > +       if (!hl_type) {
> > +               pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
> > +               return -EINVAL;
> 
> -ESRCH?

Done.

> 
> > +       }
> > +
> > +       m = btf_members(hl_type);
> > +       vlen = btf_vlen(hl_type);
> > +
> > +       for (j = 0; j < vlen; j++) {
> 
> can add succinct `, m++` here instead

Done.

> 
> > +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > +                       return j + 1;
> 
> I looked briefly through kernel-side patch introducing lsm_hook_index,
> but it didn't seem to explain why this index needs to be (unnaturally)
> 1-based. So asking here first as I'm looking through libbpf changes?

The lsm_hook_idx is one-based as it makes it easy to validate the
input. If we make it zero-based it's hard to check if the user
intended to attach to the LSM hook at index 0 or did not set it.

We are then up to the verifier to reject the loaded program which
may or may not match the signature of the hook at lsm_hook_idx = 0.

I will clarify this in the commit log as well.

> 
> > +               m++;
> > +       }
> > +
> > +       pr_warn("Cannot find offset for %s in security_hook_heads\n", name);
> 
> it's not offset, rather member index?

Correct, fixed.

> 
> > +       return -ENOENT;
> 
> not entirely clear about distinction between ENOENT and ESRCH? So far
> we typically used ESRCH, does ENOENT have more specific semantics?

Updated it to ESRCH, to be consistent with libbpf's convention.

> 
> > +}
> > +
> >  int libbpf_find_vmlinux_btf_id(const char *name,
> >                                enum bpf_attach_type attach_type)
> >  {
> >         struct btf *btf = bpf_find_kernel_btf();
> > -       char raw_tp_btf[128] = BTF_PREFIX;
> > -       char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> > -       const char *btf_name;
> >         int err = -EINVAL;
> > -       __u32 kind;
> >
> >         if (IS_ERR(btf)) {
> >                 pr_warn("vmlinux BTF is not found\n");
> >                 return -EINVAL;
> >         }
> >
> > -       if (attach_type == BPF_TRACE_RAW_TP) {
> > -               /* prepend "btf_trace_" prefix per kernel convention */
> > -               strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> > -               btf_name = raw_tp_btf;
> > -               kind = BTF_KIND_TYPEDEF;
> > -       } else {
> > -               btf_name = name;
> > -               kind = BTF_KIND_FUNC;
> > -       }
> > -       err = btf__find_by_name_kind(btf, btf_name, kind);
> > +       if (attach_type == BPF_TRACE_RAW_TP)
> > +               err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
> > +                                              BTF_KIND_TYPEDEF);
> > +       else
> > +               err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > +
> > +       /* err = 0 means void / UNKNOWN which is treated as an error */
> > +       if (err == 0)
> > +               err = -EINVAL;
> 
> I think it's actually less error-prone to make btf__find_by_name_kind
> and btf__find_by_prefix_kind to return -ESRCH when type is not found,
> instead of a valid type_id 0. I just checked, and struct_ops code
> already is mishandling it, only checking for <0. Could you make this
> change and just do a natural <0 check everywhere?
> 
> 
> > +
> >         btf__free(btf);
> >         return err;
> >  }
> > @@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> >         }
> >         err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> >         btf__free(btf);
> > -       if (err <= 0) {
> > +       if (err < 0) {
> >                 pr_warn("%s is not found in prog's BTF\n", name);
> >                 goto out;
> >         }
> > @@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> >         return bpf_program__attach_trace(prog);
> >  }
> >
> > +struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
> > +{
> > +       char errmsg[STRERR_BUFSIZE];
> > +       struct bpf_link_fd *link;
> > +       int prog_fd, pfd;
> > +
> > +       prog_fd = bpf_program__fd(prog);
> > +       if (prog_fd < 0) {
> > +               pr_warn("program '%s': can't attach before loaded\n",
> > +                       bpf_program__title(prog, false));
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       link = calloc(1, sizeof(*link));
> > +       if (!link)
> > +               return ERR_PTR(-ENOMEM);
> > +       link->link.detach = &bpf_link__detach_fd;
> > +
> > +       pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
> > +                             BPF_F_ALLOW_OVERRIDE);
> 
> do we want to always specify ALLOW_OVERRIDE? Or should it be an option?

I think this is a relic from the duplicate attachment code. With the
anonymous-fd + link which can be destroyed. The way the OVERRIDE
should work is:

-  Destroy the link (detach)
-  And attach.

We don't use the BPF_F_ALLOW_OVERRIDE in the attach logic in the LSM.
So I will get rid of that.

> 
> > +       if (pfd < 0) {
> > +               pfd = -errno;
> > +               pr_warn("program '%s': failed to attach: %s\n",
> > +                       bpf_program__title(prog, false),
> > +                       libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > +               return ERR_PTR(pfd);
> 
> leaking link here

Fixed.

> 
> > +       }
> > +       link->fd = pfd;
> > +       return (struct bpf_link *)link;
> > +}
> > +
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > +                                  struct bpf_program *prog)
> > +{
> > +       return bpf_program__attach_lsm(prog);
> > +}
> > +
> >  struct bpf_link *bpf_program__attach(struct bpf_program *prog)
> >  {
> >         const struct bpf_sec_def *sec_def;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 01639f9a1062..a97e709a29e6 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
> >  bpf_program__attach_trace(struct bpf_program *prog);
> >  struct bpf_map;
> >  LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_lsm(struct bpf_program *prog);
> 
> nit: put it after attach_trace, so that program attaches and map
> attaches are grouped together, not intermixed

Done.

> 
> >  struct bpf_insn;
> >
> >  /*
> > @@ -318,6 +320,7 @@ 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_tracing(struct bpf_program *prog);
> >  LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
> > +LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
> >
> >  LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
> >  LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> > @@ -339,6 +342,7 @@ 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_tracing(const struct bpf_program *prog);
> >  LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
> > +LIBBPF_API bool bpf_program__is_lsm(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 a19f04e6e3d9..3da0452ce679 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
> >                 bpf_program__is_struct_ops;
> >                 bpf_program__set_struct_ops;
> >                 btf__align_of;
> > +               bpf_program__is_lsm;
> > +               bpf_program__set_lsm;
> > +               bpf_program__attach_lsm;
> 
> preserve alphabetical order, please

Sure.

> 
> >  } LIBBPF_0.0.6;
> 
> > --
> > 2.20.1
> >

Re: [PATCH bpf-next v2 02/10] bpf: lsm: Add a skeleton and config options

[KP Singh]

On 15-Jan 23:04, Casey Schaufler wrote:
> On 1/15/2020 9:13 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > The LSM can be enabled by CONFIG_SECURITY_BPF.
> > Without CONFIG_SECURITY_BPF_ENFORCE, the LSM will run the
> > attached eBPF programs but not enforce MAC policy based
> > on the return value of the attached programs.
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  MAINTAINERS           |  7 +++++++
> >  security/Kconfig      | 11 ++++++-----
> >  security/Makefile     |  2 ++
> >  security/bpf/Kconfig  | 22 ++++++++++++++++++++++
> >  security/bpf/Makefile |  5 +++++
> >  security/bpf/lsm.c    | 25 +++++++++++++++++++++++++
> >  6 files changed, 67 insertions(+), 5 deletions(-)
> >  create mode 100644 security/bpf/Kconfig
> >  create mode 100644 security/bpf/Makefile
> >  create mode 100644 security/bpf/lsm.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 66a2e5e07117..0941f478cfa5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3203,6 +3203,13 @@ S:	Supported
> >  F:	arch/x86/net/
> >  X:	arch/x86/net/bpf_jit_comp32.c
> >  
> > +BPF SECURITY MODULE
> > +M:	KP Singh <kpsingh@chromium.org>
> > +L:	linux-security-module@vger.kernel.org
> > +L:	bpf@vger.kernel.org
> > +S:	Maintained
> > +F:	security/bpf/
> > +
> >  BROADCOM B44 10/100 ETHERNET DRIVER
> >  M:	Michael Chan <michael.chan@broadcom.com>
> >  L:	netdev@vger.kernel.org
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 2a1a2d396228..6f1aab195e7d 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/bpf/Kconfig"
> >  source "security/safesetid/Kconfig"
> >  source "security/lockdown/Kconfig"
> >  
> > @@ -277,11 +278,11 @@ endchoice
> >  
> >  config LSM
> >  	string "Ordered list of enabled LSMs"
> > -	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
> > -	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
> > -	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> > -	default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
> > -	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> > +	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> > +	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> > +	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> > +	default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
> > +	default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
> >  	help
> >  	  A comma-separated list of LSMs, in initialization order.
> >  	  Any LSMs left off this list will be ignored. This can be
> > diff --git a/security/Makefile b/security/Makefile
> > index be1dd9d2cb2f..50e6821dd7b7 100644
> > --- a/security/Makefile
> > +++ b/security/Makefile
> > @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA)		+= yama
> >  subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
> >  subdir-$(CONFIG_SECURITY_SAFESETID)    += safesetid
> >  subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown
> > +subdir-$(CONFIG_SECURITY_BPF)		+= bpf
> >  
> >  # always enable default capabilities
> >  obj-y					+= commoncap.o
> > @@ -29,6 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA)		+= yama/
> >  obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
> >  obj-$(CONFIG_SECURITY_SAFESETID)       += safesetid/
> >  obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
> > +obj-$(CONFIG_SECURITY_BPF)		+= bpf/
> >  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
> >  
> >  # Object integrity file lists
> > diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> > new file mode 100644
> > index 000000000000..a5f6c67ae526
> > --- /dev/null
> > +++ b/security/bpf/Kconfig
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright 2019 Google LLC.
> > +
> > +config SECURITY_BPF
> > +	bool "BPF-based MAC and audit policy"
> > +	depends on SECURITY
> > +	depends on BPF_SYSCALL
> > +	help
> > +	  This enables instrumentation of the security hooks with
> > +	  eBPF programs.
> > +
> > +	  If you are unsure how to answer this question, answer N.
> > +
> > +config SECURITY_BPF_ENFORCE
> > +	bool "Deny operations based on the evaluation of the attached programs"
> > +	depends on SECURITY_BPF
> > +	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 or deny the operation
> > +	  guarded by the security hook.
> > diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> > new file mode 100644
> > index 000000000000..26a0ab6f99b7
> > --- /dev/null
> > +++ b/security/bpf/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright 2019 Google LLC.
> > +
> > +obj-$(CONFIG_SECURITY_BPF) := lsm.o
> > diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> > new file mode 100644
> > index 000000000000..5c5c14f990ce
> > --- /dev/null
> > +++ b/security/bpf/lsm.c
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright 2019 Google LLC.
> > + */
> > +
> > +#include <linux/lsm_hooks.h>
> > +
> > +/* This is only for internal hooks, always statically shipped as part of the
> > + * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> > + * which is common for LSMs and R/O after init.
> > + */
> > +static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
> 
> s/lsm_hooks/bpf_hooks/
> 
> The lsm prefix is for the infrastructure. The way you have it is massively confusing.

Good point, I changed this to bpf_lsm_hooks as we prefix most types
with bpf_lsm_

> 
> > +
> > +static int __init lsm_init(void)
> 
> s/lsm_init/bpf_init/
> 
> Same reason. When I'm looking at several security modules at once I
> need to be able to tell them apart.

Changed to bpf_lsm_init.

> 
> > +{
> > +	security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> > +	pr_info("eBPF and LSM are friends now.\n");
> 
> Cute message, but not very informative if you haven't read the code.
> "LSM support for eBPF active\n" is more likely to be comprehensible.

Agreed, Updated :)

- KP

> 
> > +	return 0;
> > +}
> > +
> > +DEFINE_LSM(bpf) = {
> > +	.name = "bpf",
> > +	.init = lsm_init,
> > +};
> 

Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[KP Singh]

On 16-Jan 13:49, KP Singh wrote:
> Thanks for the review Andrii!
> 
> I will incorporate the fixes in the next revision.
> 
> On 15-Jan 13:19, Andrii Nakryiko wrote:
> > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > >   attr->lsm_hook_index
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > ---
> > >  tools/lib/bpf/bpf.c      |   6 +-
> > >  tools/lib/bpf/bpf.h      |   1 +
> > >  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
> > >  tools/lib/bpf/libbpf.h   |   4 ++
> > >  tools/lib/bpf/libbpf.map |   3 +
> > >  5 files changed, 138 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > index 500afe478e94..b138d98ff862 100644
> > > --- a/tools/lib/bpf/bpf.c
> > > +++ b/tools/lib/bpf/bpf.c
> > > @@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > >         memset(&attr, 0, sizeof(attr));
> > >         attr.prog_type = load_attr->prog_type;
> > >         attr.expected_attach_type = load_attr->expected_attach_type;
> > > -       if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > > +
> > > +       if (attr.prog_type == BPF_PROG_TYPE_LSM) {
> > > +               attr.lsm_hook_index = load_attr->lsm_hook_index;
> > > +       } else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > >                 attr.attach_btf_id = load_attr->attach_btf_id;
> > >         } else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
> > >                 attr.attach_btf_id = load_attr->attach_btf_id;
> > > @@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > >                 attr.prog_ifindex = load_attr->prog_ifindex;
> > >                 attr.kern_version = load_attr->kern_version;
> > >         }
> > > +
> > >         attr.insn_cnt = (__u32)load_attr->insns_cnt;
> > >         attr.insns = ptr_to_u64(load_attr->insns);
> > >         attr.license = ptr_to_u64(load_attr->license);
> > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > > index 56341d117e5b..54458a102939 100644
> > > --- a/tools/lib/bpf/bpf.h
> > > +++ b/tools/lib/bpf/bpf.h
> > > @@ -86,6 +86,7 @@ struct bpf_load_program_attr {
> > >                 __u32 prog_ifindex;
> > >                 __u32 attach_btf_id;
> > >         };
> > > +       __u32 lsm_hook_index;
> > 
> > 
> > this is changing memory layout of struct bpf_load_program_attr, which
> > is part of public API, so breaking backward compatibility. But I think
> > you intended to put it inside union along the attach_btf_id?
> > 
> 
> Correct, I moved it to the union.
> 
> > also, we use idx for index pretty consistently (apart from ifindex),
> > so maybe lsm_hook_idx?
> 
> Renamed all usages of index -> idx.
> 
> > 
> > >         __u32 prog_btf_fd;
> > >         __u32 func_info_rec_size;
> > >         const void *func_info;
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 0c229f00a67e..60737559a9a6 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -229,6 +229,7 @@ struct bpf_program {
> > >         enum bpf_attach_type expected_attach_type;
> > >         __u32 attach_btf_id;
> > >         __u32 attach_prog_fd;
> > > +       __u32 lsm_hook_index
> > >         void *func_info;
> > >         __u32 func_info_rec_size;
> > >         __u32 func_info_cnt;
> > > @@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > >         load_attr.insns = insns;
> > >         load_attr.insns_cnt = insns_cnt;
> > >         load_attr.license = license;
> > > -       if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > > +
> > > +       if (prog->type == BPF_PROG_TYPE_LSM) {
> > > +               load_attr.lsm_hook_index = prog->lsm_hook_index;
> > > +       } else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > >                 load_attr.attach_btf_id = prog->attach_btf_id;
> > >         } else if (prog->type == BPF_PROG_TYPE_TRACING) {
> > >                 load_attr.attach_prog_fd = prog->attach_prog_fd;
> > > @@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > >                 load_attr.kern_version = kern_version;
> > >                 load_attr.prog_ifindex = prog->prog_ifindex;
> > >         }
> > > +
> > >         /* if .BTF.ext was loaded, kernel supports associated BTF for prog */
> > >         if (prog->obj->btf_ext)
> > >                 btf_fd = bpf_object__btf_fd(prog->obj);
> > > @@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
> > >                                      enum bpf_attach_type attach_type,
> > >                                      __u32 attach_prog_fd);
> > >
> > > +static __s32 btf__find_lsm_hook_index(const char *name);
> > > +
> > >  int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> > >  {
> > > -       int err = 0, fd, i, btf_id;
> > > +       int err = 0, fd, i, btf_id, index;
> > >
> > >         if (prog->type == BPF_PROG_TYPE_TRACING) {
> > >                 btf_id = libbpf_find_attach_btf_id(prog->section_name,
> > > @@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> > >                 prog->attach_btf_id = btf_id;
> > >         }
> > >
> > > +       if (prog->type == BPF_PROG_TYPE_LSM) {
> > > +               index = btf__find_lsm_hook_index(prog->section_name);
> > > +               if (index < 0)
> > > +                       return index;
> > > +               prog->lsm_hook_index = index;
> > > +       }
> > > +
> > >         if (prog->instances.nr < 0 || !prog->instances.fds) {
> > >                 if (prog->preprocessor) {
> > >                         pr_warn("Internal error: can't load program '%s'\n",
> > > @@ -6207,6 +6221,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(lsm, BPF_PROG_TYPE_LSM);
> > >  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);
> > > @@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
> > >                                       struct bpf_program *prog);
> > >  static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > >                                      struct bpf_program *prog);
> > > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > > +                                  struct bpf_program *prog);
> > >
> > >  struct bpf_sec_def {
> > >         const char *sec;
> > > @@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
> > >                 .expected_attach_type = BPF_TRACE_FEXIT,
> > >                 .is_attach_btf = true,
> > >                 .attach_fn = attach_trace),
> > > +       SEC_DEF("lsm/", LSM,
> > > +               .expected_attach_type = BPF_LSM_MAC,
> > > +               .attach_fn = attach_lsm),
> > >         BPF_PROG_SEC("xdp",                     BPF_PROG_TYPE_XDP),
> > >         BPF_PROG_SEC("perf_event",              BPF_PROG_TYPE_PERF_EVENT),
> > >         BPF_PROG_SEC("lwt_in",                  BPF_PROG_TYPE_LWT_IN),
> > >         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_PROG_BTF("lsm/",                    BPF_PROG_TYPE_LSM,
> > > +                                               BPF_LSM_MAC),
> > 
> > This is just a duplicate of SEC_DEF above, remove?
> 
> Oops. Removed.
> 
> > 
> > >         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,
> > > @@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
> > >         return -EINVAL;
> > >  }
> > >
> > > -#define BTF_PREFIX "btf_trace_"
> > > +#define BTF_TRACE_PREFIX "btf_trace_"
> > > +
> > > +static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
> > > +                                          const char *prefix, __u32 kind)
> > 
> > this is internal helper, not really BTF API, let's call it
> > find_btf_by_prefix_kind? Also const char *prefix more logically should
> > go before name argument?
> 
> Done.
> 
> > 
> > > +{
> > > +       char btf_type_name[128];
> > > +
> > > +       snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
> > 
> > check overflow?
> 
> Done.
> 
> > 
> > > +       return btf__find_by_name_kind(btf, btf_type_name, kind);
> > > +}
> > > +
> > > +static __s32 btf__find_lsm_hook_index(const char *name)
> > 
> > this name is violating libbpf naming guidelines. Just
> > `find_lsm_hook_idx` for now?
> 
> Cool. I think I finally hang of the naming convention now :). I mixed
> up btf.c and libbpf.c here.
> 
> > 
> > > +{
> > > +       struct btf *btf = bpf_find_kernel_btf();
> > 
> > ok, it's probably time to do this right. Let's ensure we load kernel
> > BTF just once, keep it inside bpf_object while we need it and then
> > release it after successful load. We are at the point where all the
> > new types of program is loading/releasing kernel BTF for every section
> > and it starts to feel very wasteful.
> 
> Sure, will give it a shot in v3.

Since this will be useful for other programs as well, I will send it
as a separate patch sooner than the v3 of the LSM patch-set.

- KP

> 
> > 
> > > +       const struct bpf_sec_def *sec_def;
> > > +       const struct btf_type *hl_type;
> > > +       struct btf_member *m;
> > > +       __u16 vlen;
> > > +       __s32 hl_id;
> > > +       int j;
> > 
> > j without having i used anywhere?...
> 
> Fixed.
> 
> > 
> > > +
> > > +       sec_def = find_sec_def(name);
> > > +       if (!sec_def)
> > > +               return -ESRCH;
> > > +
> > > +       name += sec_def->len;
> > > +
> > > +       hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
> > > +                                      BTF_KIND_STRUCT);
> > > +       if (hl_id < 0) {
> > > +               pr_debug("security_hook_heads cannot be found in BTF\n");
> > 
> > "in vmlinux BTF" ?
> > 
> > and it should be pr_warn(), we don't really expect this, right?
> 
> Fixed both.
> 
> > 
> > and it should be (hl_id <= 0) with current btf__find_by_name_kind(),
> > and then return hl_id ? : -ESRCH, which further proves we need to
> > change btf__find_by_name_kind() as I suggested below.
> > 
> > > +               return hl_id;
> > > +       }
> > > +
> > > +       hl_type = btf__type_by_id(btf, hl_id);
> > > +       if (!hl_type) {
> > > +               pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
> > > +               return -EINVAL;
> > 
> > -ESRCH?
> 
> Done.
> 
> > 
> > > +       }
> > > +
> > > +       m = btf_members(hl_type);
> > > +       vlen = btf_vlen(hl_type);
> > > +
> > > +       for (j = 0; j < vlen; j++) {
> > 
> > can add succinct `, m++` here instead
> 
> Done.
> 
> > 
> > > +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > +                       return j + 1;
> > 
> > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > but it didn't seem to explain why this index needs to be (unnaturally)
> > 1-based. So asking here first as I'm looking through libbpf changes?
> 
> The lsm_hook_idx is one-based as it makes it easy to validate the
> input. If we make it zero-based it's hard to check if the user
> intended to attach to the LSM hook at index 0 or did not set it.
> 
> We are then up to the verifier to reject the loaded program which
> may or may not match the signature of the hook at lsm_hook_idx = 0.
> 
> I will clarify this in the commit log as well.
> 
> > 
> > > +               m++;
> > > +       }
> > > +
> > > +       pr_warn("Cannot find offset for %s in security_hook_heads\n", name);
> > 
> > it's not offset, rather member index?
> 
> Correct, fixed.
> 
> > 
> > > +       return -ENOENT;
> > 
> > not entirely clear about distinction between ENOENT and ESRCH? So far
> > we typically used ESRCH, does ENOENT have more specific semantics?
> 
> Updated it to ESRCH, to be consistent with libbpf's convention.
> 
> > 
> > > +}
> > > +
> > >  int libbpf_find_vmlinux_btf_id(const char *name,
> > >                                enum bpf_attach_type attach_type)
> > >  {
> > >         struct btf *btf = bpf_find_kernel_btf();
> > > -       char raw_tp_btf[128] = BTF_PREFIX;
> > > -       char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> > > -       const char *btf_name;
> > >         int err = -EINVAL;
> > > -       __u32 kind;
> > >
> > >         if (IS_ERR(btf)) {
> > >                 pr_warn("vmlinux BTF is not found\n");
> > >                 return -EINVAL;
> > >         }
> > >
> > > -       if (attach_type == BPF_TRACE_RAW_TP) {
> > > -               /* prepend "btf_trace_" prefix per kernel convention */
> > > -               strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> > > -               btf_name = raw_tp_btf;
> > > -               kind = BTF_KIND_TYPEDEF;
> > > -       } else {
> > > -               btf_name = name;
> > > -               kind = BTF_KIND_FUNC;
> > > -       }
> > > -       err = btf__find_by_name_kind(btf, btf_name, kind);
> > > +       if (attach_type == BPF_TRACE_RAW_TP)
> > > +               err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
> > > +                                              BTF_KIND_TYPEDEF);
> > > +       else
> > > +               err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > > +
> > > +       /* err = 0 means void / UNKNOWN which is treated as an error */
> > > +       if (err == 0)
> > > +               err = -EINVAL;
> > 
> > I think it's actually less error-prone to make btf__find_by_name_kind
> > and btf__find_by_prefix_kind to return -ESRCH when type is not found,
> > instead of a valid type_id 0. I just checked, and struct_ops code
> > already is mishandling it, only checking for <0. Could you make this
> > change and just do a natural <0 check everywhere?
> > 
> > 
> > > +
> > >         btf__free(btf);
> > >         return err;
> > >  }
> > > @@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> > >         }
> > >         err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > >         btf__free(btf);
> > > -       if (err <= 0) {
> > > +       if (err < 0) {
> > >                 pr_warn("%s is not found in prog's BTF\n", name);
> > >                 goto out;
> > >         }
> > > @@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > >         return bpf_program__attach_trace(prog);
> > >  }
> > >
> > > +struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
> > > +{
> > > +       char errmsg[STRERR_BUFSIZE];
> > > +       struct bpf_link_fd *link;
> > > +       int prog_fd, pfd;
> > > +
> > > +       prog_fd = bpf_program__fd(prog);
> > > +       if (prog_fd < 0) {
> > > +               pr_warn("program '%s': can't attach before loaded\n",
> > > +                       bpf_program__title(prog, false));
> > > +               return ERR_PTR(-EINVAL);
> > > +       }
> > > +
> > > +       link = calloc(1, sizeof(*link));
> > > +       if (!link)
> > > +               return ERR_PTR(-ENOMEM);
> > > +       link->link.detach = &bpf_link__detach_fd;
> > > +
> > > +       pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
> > > +                             BPF_F_ALLOW_OVERRIDE);
> > 
> > do we want to always specify ALLOW_OVERRIDE? Or should it be an option?
> 
> I think this is a relic from the duplicate attachment code. With the
> anonymous-fd + link which can be destroyed. The way the OVERRIDE
> should work is:
> 
> -  Destroy the link (detach)
> -  And attach.
> 
> We don't use the BPF_F_ALLOW_OVERRIDE in the attach logic in the LSM.
> So I will get rid of that.
> 
> > 
> > > +       if (pfd < 0) {
> > > +               pfd = -errno;
> > > +               pr_warn("program '%s': failed to attach: %s\n",
> > > +                       bpf_program__title(prog, false),
> > > +                       libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > > +               return ERR_PTR(pfd);
> > 
> > leaking link here
> 
> Fixed.
> 
> > 
> > > +       }
> > > +       link->fd = pfd;
> > > +       return (struct bpf_link *)link;
> > > +}
> > > +
> > > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > > +                                  struct bpf_program *prog)
> > > +{
> > > +       return bpf_program__attach_lsm(prog);
> > > +}
> > > +
> > >  struct bpf_link *bpf_program__attach(struct bpf_program *prog)
> > >  {
> > >         const struct bpf_sec_def *sec_def;
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 01639f9a1062..a97e709a29e6 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
> > >  bpf_program__attach_trace(struct bpf_program *prog);
> > >  struct bpf_map;
> > >  LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
> > > +LIBBPF_API struct bpf_link *
> > > +bpf_program__attach_lsm(struct bpf_program *prog);
> > 
> > nit: put it after attach_trace, so that program attaches and map
> > attaches are grouped together, not intermixed
> 
> Done.
> 
> > 
> > >  struct bpf_insn;
> > >
> > >  /*
> > > @@ -318,6 +320,7 @@ 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_tracing(struct bpf_program *prog);
> > >  LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
> > > +LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
> > >
> > >  LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
> > >  LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> > > @@ -339,6 +342,7 @@ 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_tracing(const struct bpf_program *prog);
> > >  LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
> > > +LIBBPF_API bool bpf_program__is_lsm(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 a19f04e6e3d9..3da0452ce679 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
> > >                 bpf_program__is_struct_ops;
> > >                 bpf_program__set_struct_ops;
> > >                 btf__align_of;
> > > +               bpf_program__is_lsm;
> > > +               bpf_program__set_lsm;
> > > +               bpf_program__attach_lsm;
> > 
> > preserve alphabetical order, please
> 
> Sure.
> 
> > 
> > >  } LIBBPF_0.0.6;
> > 
> > > --
> > > 2.20.1
> > >

Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[Andrii Nakryiko]

On Thu, Jan 16, 2020 at 4:49 AM KP Singh <kpsingh@chromium.org> wrote:
>
> Thanks for the review Andrii!
>
> I will incorporate the fixes in the next revision.
>
> On 15-Jan 13:19, Andrii Nakryiko wrote:
> > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > >   attr->lsm_hook_index
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > ---
> > >  tools/lib/bpf/bpf.c      |   6 +-
> > >  tools/lib/bpf/bpf.h      |   1 +
> > >  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
> > >  tools/lib/bpf/libbpf.h   |   4 ++
> > >  tools/lib/bpf/libbpf.map |   3 +
> > >  5 files changed, 138 insertions(+), 19 deletions(-)
> > >

[...]

> >
> > > +{
> > > +       struct btf *btf = bpf_find_kernel_btf();
> >
> > ok, it's probably time to do this right. Let's ensure we load kernel
> > BTF just once, keep it inside bpf_object while we need it and then
> > release it after successful load. We are at the point where all the
> > new types of program is loading/releasing kernel BTF for every section
> > and it starts to feel very wasteful.
>
> Sure, will give it a shot in v3.

thanks!

[...]

> >
> > > +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > +                       return j + 1;
> >
> > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > but it didn't seem to explain why this index needs to be (unnaturally)
> > 1-based. So asking here first as I'm looking through libbpf changes?
>
> The lsm_hook_idx is one-based as it makes it easy to validate the
> input. If we make it zero-based it's hard to check if the user
> intended to attach to the LSM hook at index 0 or did not set it.

Think about providing FDs. 0 is a valid, though rarely
intended/correct value. Yet we don't make all FD arguments
artificially 1-based, right? This extra +1/-1 translation just makes
for more confusing interface, IMO. If user "accidentally" guessed type
signature of very first hook, well, so be it... If not, BPF verifier
will politely refuse. Seems like enough protection.

>
> We are then up to the verifier to reject the loaded program which
> may or may not match the signature of the hook at lsm_hook_idx = 0.
>
> I will clarify this in the commit log as well.
>

[...]

Re: [PATCH bpf-next v2 05/10] bpf: lsm: BTF API for LSM hooks

[Andrii Nakryiko]

On Wed, Jan 15, 2020 at 9:14 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> The BTF API provides information required by the BPF verifier to
> attach eBPF programs to the LSM hooks by using the BTF information of
> two types:
>
> - struct security_hook_heads: This type provides the offset which
>   a new dynamically allocated security hook must be attached to.
> - union security_list_options: This provides the information about the
>   function prototype required by the hook.
>
> When the program is loaded:
>
> - The verifier receives the index of a member in struct
>   security_hook_heads to which a program must be attached as
>   prog->aux->lsm_hook_index. The index is one-based for better
>   verification.
> - bpf_lsm_type_by_index is used to determine the func_proto of
>   the LSM hook and updates prog->aux->attach_func_proto
> - bpf_lsm_head_by_index is used to determine the hlist_head to which
>   the BPF program must be attached.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  include/linux/bpf_lsm.h |  12 +++++
>  security/bpf/Kconfig    |   1 +
>  security/bpf/hooks.c    | 104 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 117 insertions(+)
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 9883cf25241c..a9b4f7b41c65 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -19,6 +19,8 @@ extern struct security_hook_heads bpf_lsm_hook_heads;
>
>  int bpf_lsm_srcu_read_lock(void);
>  void bpf_lsm_srcu_read_unlock(int idx);
> +const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 offset);
> +const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 id);
>
>  #define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)                     \
>         do {                                                    \
> @@ -65,6 +67,16 @@ static inline int bpf_lsm_srcu_read_lock(void)
>         return 0;
>  }
>  static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> +static inline const struct btf_type *bpf_lsm_type_by_index(
> +       struct btf *btf, u32 index)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline const struct btf_member *bpf_lsm_head_by_index(
> +       struct btf *btf, u32 id)
> +{
> +       return ERR_PTR(-EOPNOTSUPP);
> +}
>
>  #endif /* CONFIG_SECURITY_BPF */
>
> diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> index 595e4ad597ae..9438d899b618 100644
> --- a/security/bpf/Kconfig
> +++ b/security/bpf/Kconfig
> @@ -7,6 +7,7 @@ config SECURITY_BPF
>         depends on SECURITY
>         depends on BPF_SYSCALL
>         depends on SRCU
> +       depends on DEBUG_INFO_BTF
>         help
>           This enables instrumentation of the security hooks with
>           eBPF programs.
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> index b123d9cb4cd4..82725611693d 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -5,6 +5,8 @@
>   */
>
>  #include <linux/bpf_lsm.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
>  #include <linux/srcu.h>
>
>  DEFINE_STATIC_SRCU(security_hook_srcu);
> @@ -18,3 +20,105 @@ void bpf_lsm_srcu_read_unlock(int idx)
>  {
>         return srcu_read_unlock(&security_hook_srcu, idx);
>  }
> +
> +static inline int validate_hlist_head(struct btf *btf, u32 type_id)
> +{
> +       s32 hlist_id;
> +
> +       hlist_id = btf_find_by_name_kind(btf, "hlist_head", BTF_KIND_STRUCT);
> +       if (hlist_id < 0 || hlist_id != type_id)
> +               return -EINVAL;

This feels backwards and expensive. You already have type_id you want
to check. Do a quick look up, check type and other attributes, if you
want. There is no need to do linear search for struct named
"hlist_head".

But in reality, you should trust kernel BTF, you already know that you
found correct "security_hook_heads" struct, so its member has to be
hlist_head, no?

> +
> +       return 0;
> +}
> +
> +/* Find the BTF representation of the security_hook_heads member for a member
> + * with a given index in struct security_hook_heads.
> + */
> +const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 index)
> +{
> +       const struct btf_member *member;
> +       const struct btf_type *t;
> +       u32 off, i;
> +       int ret;
> +
> +       t = btf_type_by_name_kind(btf, "security_hook_heads", BTF_KIND_STRUCT);
> +       if (WARN_ON_ONCE(IS_ERR(t)))
> +               return ERR_CAST(t);
> +
> +       for_each_member(i, t, member) {
> +               /* We've found the id requested and need to check the
> +                * the following:
> +                *
> +                * - Is it at a valid alignment for struct hlist_head?
> +                *
> +                * - Is it a valid hlist_head struct?
> +                */
> +               if (index == i) {

Also not efficient. Check index to be < vlen(t), then member =
btf_type_member(t) + index;


> +                       off = btf_member_bit_offset(t, member);
> +                       if (off % 8)
> +                               /* valid c code cannot generate such btf */
> +                               return ERR_PTR(-EINVAL);
> +                       off /= 8;
> +
> +                       if (off % __alignof__(struct hlist_head))
> +                               return ERR_PTR(-EINVAL);
> +
> +                       ret = validate_hlist_head(btf, member->type);
> +                       if (ret < 0)
> +                               return ERR_PTR(ret);
> +
> +                       return member;

This feels a bit over-cautious to double-check this. If
security_hook_heads definition is controlled by kernel sources, then
we could just trust vmlinux BTF?

> +               }
> +       }
> +
> +       return ERR_PTR(-ENOENT);
> +}
> +
> +/* Given an index of a member in security_hook_heads return the
> + * corresponding type for the LSM hook. The members of the union
> + * security_list_options have the same name as the security_hook_heads which
> + * is ensured by the LSM_HOOK_INIT macro defined in include/linux/lsm_hooks.h
> + */
> +const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 index)
> +{
> +       const struct btf_member *member, *hook_head = NULL;
> +       const struct btf_type *t, *hook_type = NULL;
> +       u32 i;
> +
> +       hook_head = bpf_lsm_head_by_index(btf, index);
> +       if (IS_ERR(hook_head))
> +               return ERR_PTR(PTR_ERR(hook_head));
> +
> +       t = btf_type_by_name_kind(btf, "security_list_options", BTF_KIND_UNION);
> +       if (WARN_ON_ONCE(IS_ERR(t)))
> +               return ERR_CAST(t);

btf_type_by_name_kind() is a linear search (at least right now), so it
might be a good idea to cache found type_id's of security_list_options
and security_hook_heads?

> +
> +       for_each_member(i, t, member) {
> +               if (hook_head->name_off == member->name_off) {
> +                       /* There should be only one member with the same name
> +                        * as the LSM hook. This should never really happen
> +                        * and either indicates malformed BTF or someone trying
> +                        * trick the LSM.
> +                        */
> +                       if (WARN_ON(hook_type))
> +                               return ERR_PTR(-EINVAL);
> +
> +                       hook_type = btf_type_by_id(btf, member->type);
> +                       if (unlikely(!hook_type))
> +                               return ERR_PTR(-EINVAL);
> +
> +                       if (!btf_type_is_ptr(hook_type))
> +                               return ERR_PTR(-EINVAL);
> +               }
> +       }
> +
> +       if (!hook_type)
> +               return ERR_PTR(-ENOENT);
> +
> +       t = btf_type_by_id(btf, hook_type->type);
> +       if (unlikely(!t))
> +               return ERR_PTR(-EINVAL);

why not do this inside the loop when you find correct member and not
continue processing all the fields?

> +
> +       return t;
> +}
> --
> 2.20.1
>

Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM

[KP Singh]

On 16-Jan 11:10, Andrii Nakryiko wrote:
> On Thu, Jan 16, 2020 at 4:49 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > Thanks for the review Andrii!
> >
> > I will incorporate the fixes in the next revision.
> >
> > On 15-Jan 13:19, Andrii Nakryiko wrote:
> > > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > > >
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > > >   attr->lsm_hook_index
> > > >
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > > ---
> > > >  tools/lib/bpf/bpf.c      |   6 +-
> > > >  tools/lib/bpf/bpf.h      |   1 +
> > > >  tools/lib/bpf/libbpf.c   | 143 ++++++++++++++++++++++++++++++++++-----
> > > >  tools/lib/bpf/libbpf.h   |   4 ++
> > > >  tools/lib/bpf/libbpf.map |   3 +
> > > >  5 files changed, 138 insertions(+), 19 deletions(-)
> > > >
> 
> [...]
> 
> > >
> > > > +{
> > > > +       struct btf *btf = bpf_find_kernel_btf();
> > >
> > > ok, it's probably time to do this right. Let's ensure we load kernel
> > > BTF just once, keep it inside bpf_object while we need it and then
> > > release it after successful load. We are at the point where all the
> > > new types of program is loading/releasing kernel BTF for every section
> > > and it starts to feel very wasteful.
> >
> > Sure, will give it a shot in v3.
> 
> thanks!
> 
> [...]
> 
> > >
> > > > +               if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > > +                       return j + 1;
> > >
> > > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > > but it didn't seem to explain why this index needs to be (unnaturally)
> > > 1-based. So asking here first as I'm looking through libbpf changes?
> >
> > The lsm_hook_idx is one-based as it makes it easy to validate the
> > input. If we make it zero-based it's hard to check if the user
> > intended to attach to the LSM hook at index 0 or did not set it.
> 
> Think about providing FDs. 0 is a valid, though rarely
> intended/correct value. Yet we don't make all FD arguments
> artificially 1-based, right? This extra +1/-1 translation just makes
> for more confusing interface, IMO. If user "accidentally" guessed type
> signature of very first hook, well, so be it... If not, BPF verifier
> will politely refuse. Seems like enough protection.

Thanks! I see your point and will update to using the
more-conventional 0-based indexing for the next revision.

- KP

> 
> >
> > We are then up to the verifier to reject the loaded program which
> > may or may not match the signature of the hook at lsm_hook_idx = 0.
> >
> > I will clarify this in the commit log as well.
> >
> 
> [...]

Re: [PATCH bpf-next v2 01/10] bpf: btf: Make some of the API visible outside BTF

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]

Hi KP,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20200116]
[cannot apply to bpf-next/master bpf/master linus/master security/next-testing v5.5-rc6 v5.5-rc5 v5.5-rc4 v5.5-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/KP-Singh/MAC-and-Audit-policy-using-eBPF-KRSI/20200117-070342
base:    2747d5fdab78f43210256cd52fb2718e0b3cce74
config: nds32-defconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from kernel/bpf/core.c:27:
>> include/linux/btf.h:148:38: error: static declaration of 'btf_type_by_name_kind' follows non-static declaration
     148 | static inline const struct btf_type *btf_type_by_name_kind(
         |                                      ^~~~~~~~~~~~~~~~~~~~~
   include/linux/btf.h:70:24: note: previous declaration of 'btf_type_by_name_kind' was here
      70 | const struct btf_type *btf_type_by_name_kind(
         |                        ^~~~~~~~~~~~~~~~~~~~~

vim +/btf_type_by_name_kind +148 include/linux/btf.h

   136	
   137	#ifdef CONFIG_BPF_SYSCALL
   138	const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
   139	const char *btf_name_by_offset(const struct btf *btf, u32 offset);
   140	struct btf *btf_parse_vmlinux(void);
   141	struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
   142	#else
   143	static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
   144							    u32 type_id)
   145	{
   146		return NULL;
   147	}
 > 148	static inline const struct btf_type *btf_type_by_name_kind(
   149		struct btf *btf, const char *name, u8 kind)
   150	{
   151		return ERR_PTR(-EOPNOTSUPP);
   152	}
   153	static inline const char *btf_name_by_offset(const struct btf *btf,
   154						     u32 offset)
   155	{
   156		return NULL;
   157	}
   158	#endif
   159	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10867 bytes --]

Re: [PATCH bpf-next v2 01/10] bpf: btf: Make some of the API visible outside BTF

[KP Singh]

Thanks! I have fixed this in the v3 of the series. btf_find_by_name_kind
is independant of CONFIG_BPF_SYSCALL and btf_type_by_name_kind needs
to be as well.

The mistake was adding a static inline definition of the function
in the !CONFIG_BPF_SYSCALL section which is not needed in this case.

- KP

On 18-Jan 20:44, kbuild test robot wrote:
> Hi KP,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on next-20200116]
> [cannot apply to bpf-next/master bpf/master linus/master security/next-testing v5.5-rc6 v5.5-rc5 v5.5-rc4 v5.5-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> 
> url:    https://github.com/0day-ci/linux/commits/KP-Singh/MAC-and-Audit-policy-using-eBPF-KRSI/20200117-070342
> base:    2747d5fdab78f43210256cd52fb2718e0b3cce74
> config: nds32-defconfig (attached as .config)
> compiler: nds32le-linux-gcc (GCC) 9.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=9.2.0 make.cross ARCH=nds32 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from kernel/bpf/core.c:27:
> >> include/linux/btf.h:148:38: error: static declaration of 'btf_type_by_name_kind' follows non-static declaration
>      148 | static inline const struct btf_type *btf_type_by_name_kind(
>          |                                      ^~~~~~~~~~~~~~~~~~~~~
>    include/linux/btf.h:70:24: note: previous declaration of 'btf_type_by_name_kind' was here
>       70 | const struct btf_type *btf_type_by_name_kind(
>          |                        ^~~~~~~~~~~~~~~~~~~~~
> 
> vim +/btf_type_by_name_kind +148 include/linux/btf.h
> 
>    136	
>    137	#ifdef CONFIG_BPF_SYSCALL
>    138	const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>    139	const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>    140	struct btf *btf_parse_vmlinux(void);
>    141	struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>    142	#else
>    143	static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
>    144							    u32 type_id)
>    145	{
>    146		return NULL;
>    147	}
>  > 148	static inline const struct btf_type *btf_type_by_name_kind(
>    149		struct btf *btf, const char *name, u8 kind)
>    150	{
>    151		return ERR_PTR(-EOPNOTSUPP);
>    152	}
>    153	static inline const char *btf_name_by_offset(const struct btf *btf,
>    154						     u32 offset)
>    155	{
>    156		return NULL;
>    157	}
>    158	#endif
>    159	
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Re: [PATCH bpf-next v2 05/10] bpf: lsm: BTF API for LSM hooks

[KP Singh]

Thanks for reviewing!

On 16-Jan 16:28, Andrii Nakryiko wrote:
> On Wed, Jan 15, 2020 at 9:14 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > The BTF API provides information required by the BPF verifier to
> > attach eBPF programs to the LSM hooks by using the BTF information of
> > two types:
> >
> > - struct security_hook_heads: This type provides the offset which
> >   a new dynamically allocated security hook must be attached to.
> > - union security_list_options: This provides the information about the
> >   function prototype required by the hook.
> >
> > When the program is loaded:
> >
> > - The verifier receives the index of a member in struct
> >   security_hook_heads to which a program must be attached as
> >   prog->aux->lsm_hook_index. The index is one-based for better
> >   verification.
> > - bpf_lsm_type_by_index is used to determine the func_proto of
> >   the LSM hook and updates prog->aux->attach_func_proto
> > - bpf_lsm_head_by_index is used to determine the hlist_head to which
> >   the BPF program must be attached.
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  include/linux/bpf_lsm.h |  12 +++++
> >  security/bpf/Kconfig    |   1 +
> >  security/bpf/hooks.c    | 104 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 117 insertions(+)
> >
> > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > index 9883cf25241c..a9b4f7b41c65 100644
> > --- a/include/linux/bpf_lsm.h
> > +++ b/include/linux/bpf_lsm.h
> > @@ -19,6 +19,8 @@ extern struct security_hook_heads bpf_lsm_hook_heads;
> >
> >  int bpf_lsm_srcu_read_lock(void);
> >  void bpf_lsm_srcu_read_unlock(int idx);
> > +const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 offset);
> > +const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 id);
> >
> >  #define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...)                     \
> >         do {                                                    \
> > @@ -65,6 +67,16 @@ static inline int bpf_lsm_srcu_read_lock(void)
> >         return 0;
> >  }
> >  static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> > +static inline const struct btf_type *bpf_lsm_type_by_index(
> > +       struct btf *btf, u32 index)
> > +{
> > +       return ERR_PTR(-EOPNOTSUPP);
> > +}
> > +static inline const struct btf_member *bpf_lsm_head_by_index(
> > +       struct btf *btf, u32 id)
> > +{
> > +       return ERR_PTR(-EOPNOTSUPP);
> > +}
> >
> >  #endif /* CONFIG_SECURITY_BPF */
> >
> > diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> > index 595e4ad597ae..9438d899b618 100644
> > --- a/security/bpf/Kconfig
> > +++ b/security/bpf/Kconfig
> > @@ -7,6 +7,7 @@ config SECURITY_BPF
> >         depends on SECURITY
> >         depends on BPF_SYSCALL
> >         depends on SRCU
> > +       depends on DEBUG_INFO_BTF
> >         help
> >           This enables instrumentation of the security hooks with
> >           eBPF programs.
> > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> > index b123d9cb4cd4..82725611693d 100644
> > --- a/security/bpf/hooks.c
> > +++ b/security/bpf/hooks.c
> > @@ -5,6 +5,8 @@
> >   */
> >
> >  #include <linux/bpf_lsm.h>
> > +#include <linux/bpf.h>
> > +#include <linux/btf.h>
> >  #include <linux/srcu.h>
> >
> >  DEFINE_STATIC_SRCU(security_hook_srcu);
> > @@ -18,3 +20,105 @@ void bpf_lsm_srcu_read_unlock(int idx)
> >  {
> >         return srcu_read_unlock(&security_hook_srcu, idx);
> >  }
> > +
> > +static inline int validate_hlist_head(struct btf *btf, u32 type_id)
> > +{
> > +       s32 hlist_id;
> > +
> > +       hlist_id = btf_find_by_name_kind(btf, "hlist_head", BTF_KIND_STRUCT);
> > +       if (hlist_id < 0 || hlist_id != type_id)
> > +               return -EINVAL;
> 
> This feels backwards and expensive. You already have type_id you want
> to check. Do a quick look up, check type and other attributes, if you
> want. There is no need to do linear search for struct named
> "hlist_head".
> 
> But in reality, you should trust kernel BTF, you already know that you
> found correct "security_hook_heads" struct, so its member has to be
> hlist_head, no?

We had a discussion internally and also came the same conclusion (it's
okay to trust the BTF) and will remove sone of the "over-cautious"
checks in the next revision.

This one, however, in particular is to protect against the case when a
new member which is not a hlist_head is added to security_hook_heads
and the user-space tries to attach at that index.

I admit that the likelyhood of that happening is very little  but I
think it's worth checking. I do, like your idea and will update the
code to use the type_id and do a quick check rather than a linear
search to look for the type_id.

This is what remains of all the pedantic checks pertaining to
hlist_head:

	t = btf_type_by_id(btf, member->type);
	if (unlikely(!t))
		return -EINVAL;

	if (BTF_INFO_KIND(t->info) != BTF_KIND_STRUCT)
		return -EINVAL;

	if (t->size != sizeof(struct hlist_head))
		return -EINVAL;

> 
> > +
> > +       return 0;
> > +}
> > +
> > +/* Find the BTF representation of the security_hook_heads member for a member
> > + * with a given index in struct security_hook_heads.
> > + */
> > +const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 index)
> > +{
> > +       const struct btf_member *member;
> > +       const struct btf_type *t;
> > +       u32 off, i;
> > +       int ret;
> > +
> > +       t = btf_type_by_name_kind(btf, "security_hook_heads", BTF_KIND_STRUCT);
> > +       if (WARN_ON_ONCE(IS_ERR(t)))
> > +               return ERR_CAST(t);
> > +
> > +       for_each_member(i, t, member) {
> > +               /* We've found the id requested and need to check the

> > +                * the following:
> > +                *
> > +                * - Is it at a valid alignment for struct hlist_head?
> > +                *
> > +                * - Is it a valid hlist_head struct?
> > +                */
> > +               if (index == i) {
> 
> Also not efficient. Check index to be < vlen(t), then member =
> btf_type_member(t) + index;

Neat! Updated.

> 
> 
> > +                       off = btf_member_bit_offset(t, member);
> > +                       if (off % 8)
> > +                               /* valid c code cannot generate such btf */
> > +                               return ERR_PTR(-EINVAL);
> > +                       off /= 8;
> > +
> > +                       if (off % __alignof__(struct hlist_head))
> > +                               return ERR_PTR(-EINVAL);
> > +
> > +                       ret = validate_hlist_head(btf, member->type);
> > +                       if (ret < 0)
> > +                               return ERR_PTR(ret);
> > +
> > +                       return member;
> 
> This feels a bit over-cautious to double-check this. If
> security_hook_heads definition is controlled by kernel sources, then
> we could just trust vmlinux BTF?

Yep, makes sense. Removed some of these checks.

> 
> > +               }
> > +       }
> > +
> > +       return ERR_PTR(-ENOENT);
> > +}
> > +
> > +/* Given an index of a member in security_hook_heads return the
> > + * corresponding type for the LSM hook. The members of the union
> > + * security_list_options have the same name as the security_hook_heads which
> > + * is ensured by the LSM_HOOK_INIT macro defined in include/linux/lsm_hooks.h
> > + */
> > +const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 index)
> > +{
> > +       const struct btf_member *member, *hook_head = NULL;
> > +       const struct btf_type *t, *hook_type = NULL;
> > +       u32 i;
> > +
> > +       hook_head = bpf_lsm_head_by_index(btf, index);
> > +       if (IS_ERR(hook_head))
> > +               return ERR_PTR(PTR_ERR(hook_head));
> > +
> > +       t = btf_type_by_name_kind(btf, "security_list_options", BTF_KIND_UNION);
> > +       if (WARN_ON_ONCE(IS_ERR(t)))
> > +               return ERR_CAST(t);
> 
> btf_type_by_name_kind() is a linear search (at least right now), so it
> might be a good idea to cache found type_id's of security_list_options
> and security_hook_heads?

I am already caching these types in the next patch (struct
bpf_lsm_info) of the series which implements attachment. I moved it to
this patch so that it's clearer.

> 
> > +
> > +       for_each_member(i, t, member) {
> > +               if (hook_head->name_off == member->name_off) {
> > +                       /* There should be only one member with the same name
> > +                        * as the LSM hook. This should never really happen
> > +                        * and either indicates malformed BTF or someone trying
> > +                        * trick the LSM.
> > +                        */
> > +                       if (WARN_ON(hook_type))
> > +                               return ERR_PTR(-EINVAL);
> > +
> > +                       hook_type = btf_type_by_id(btf, member->type);
> > +                       if (unlikely(!hook_type))
> > +                               return ERR_PTR(-EINVAL);
> > +
> > +                       if (!btf_type_is_ptr(hook_type))
> > +                               return ERR_PTR(-EINVAL);
> > +               }
> > +       }
> > +
> > +       if (!hook_type)
> > +               return ERR_PTR(-ENOENT);
> > +
> > +       t = btf_type_by_id(btf, hook_type->type);
> > +       if (unlikely(!t))
> > +               return ERR_PTR(-EINVAL);
> 
> why not do this inside the loop when you find correct member and not
> continue processing all the fields?

Updated.

- KP

> 
> > +
> > +       return t;
> > +}
> > --
> > 2.20.1
> >

Re: [PATCH bpf-next v2 00/10] MAC and Audit policy using eBPF (KRSI)

[KP Singh]

On 15-Jan 14:12, Andrii Nakryiko wrote:
> On Wed, Jan 15, 2020 at 9:15 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > # Changes since v1 (https://lore.kernel.org/bpf/20191220154208.15895-1-kpsingh@chromium.org/):
> >
> > * Eliminate the requirement to maintain LSM hooks separately in
> >   security/bpf/hooks.h Use BPF trampolines to dynamically allocate
> >   security hooks
> > * Drop the use of securityfs as bpftool provides the required
> >   introspection capabilities.  Update the tests to use the bpf_skeleton
> >   and global variables
> > * Use O_CLOEXEC anonymous fds to represent BPF attachment in line with
> >   the other BPF programs with the possibility to use bpf program pinning
> >   in the future to provide "permanent attachment".
> > * Drop the logic based on prog names for handling re-attachment.
> > * Drop bpf_lsm_event_output from this series and send it as a separate
> >   patch.
> >
> > # Motivation
> >
> > Google does analysis of rich runtime security data to detect and thwart
> > threats in real-time. Currently, this is done in custom kernel modules
> > but we would like to replace this with something that's upstream and
> > useful to others.
> >
> > The current kernel infrastructure for providing telemetry (Audit, Perf
> > etc.) is disjoint from access enforcement (i.e. LSMs).  Augmenting the
> > 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 telemetry data requires changes to
> > various LSMs and their respective policy languages.
> >
> > This patchset proposes a new stackable and privileged LSM which allows
> > the LSM hooks to be implemented using eBPF. This facilitates a unified
> > and dynamic (not requiring re-compilation of the kernel) audit and MAC
> > policy.
> >
> > # Why an LSM?
> >
> > Linux Security Modules target security behaviours rather than the
> > kernel's API. For example, it's easy to miss out a newly added system
> > call for executing processes (eg. execve, execveat etc.) but the LSM
> > framework ensures that all process executions trigger the relevant hooks
> > irrespective of how the process was executed.
> >
> > Allowing users to implement LSM hooks at runtime also benefits the LSM
> > eco-system by enabling a quick feedback loop from the security community
> > about the kind of behaviours that the LSM Framework should be targeting.
> >
> > # How does it work?
> >
> > The LSM introduces a new eBPF (https://docs.cilium.io/en/v1.6/bpf/)
> > program type BPF_PROG_TYPE_LSM which can only be attached to LSM hooks.
> > Attachment requires CAP_SYS_ADMIN for loading eBPF programs and
> > CAP_MAC_ADMIN for modifying MAC policies.
> >
> > The eBPF programs are attached to a separate security_hook_heads
> > maintained by the BPF LSM for mutable hooks and executed after all the
> > statically defined hooks (i.e. the ones declared by SELinux, AppArmor,
> > Smack etc). This also ensures that statically defined LSM hooks retain
> > the behaviour of "being read-only after init", i.e. __lsm_ro_after_init.
> >
> > Upon attachment, a security hook is dynamically allocated with
> > arch_bpf_prepare_trampoline which generates code to handle the
> > conversion from the signature of the hook to the BPF context and allows
> > the JIT'ed BPF program to be called as a C function with the same
> > arguments as the LSM hooks. If any of the attached eBPF programs returns
> > an error (like ENOPERM), the behaviour represented by the hook is
> > denied.
> >
> > Audit logs can be written using a format chosen by the eBPF program to
> > the perf events buffer or to global eBPF variables or maps and can be
> > further processed in user-space.
> >
> > # BTF Based Design
> >
> > The current design uses BTF
> > (https://facebookmicrosites.github.io/bpf/blog/2018/11/14/btf-enhancement.html,
> > https://lwn.net/Articles/803258/) which allows verifiable read-only
> > structure accesses by field names rather than fixed offsets. This allows
> > accessing the hook parameters using a dynamically created context which
> > provides a certain degree of ABI stability:
> >
> >
> > // Only declare the structure and fields intended to be used
> > // in the program
> > struct vm_area_struct {
> >         unsigned long vm_start;
> > } __attribute__((preserve_access_index));
> >
> 
> It would be nice to also mention that you don't even have to
> "re-define" these structs if you use vmlinux.h generated with `bpftool
> btf dump file <path-to-vm-linux-or-/sys/kernel/btf/vmlinux> format c`.
> Its output will contain all types of the kernel, including internal
> ones not exposed through any public headers. And it will also
> automatically have __attribute__((preserve_access_index)) applied to
> all structs/unions. It can be pre-generated and checked in somewhere
> along the application or generated on the fly, if environment and use
> case allows.

Cool, I will update the documentation to mention this. Thanks!

- KP

> 
> > // Declare the eBPF program mprotect_audit which attaches to
> > // to the file_mprotect LSM hook and accepts three arguments.
> > SEC("lsm/file_mprotect")
> > int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
> >              unsigned long reqprot, unsigned long prot)
> > {
> >         unsigned long vm_start = vma->vm_start;
> >
> >         return 0;
> > }
> >
> > By relocating field offsets, BTF makes a large portion of kernel data
> > structures readily accessible across kernel versions without requiring a
> > large corpus of BPF helper functions and requiring recompilation with
> > every kernel version. The BTF type information is also used by the BPF
> > verifier to validate memory accesses within the BPF program and also
> > prevents arbitrary writes to the kernel memory.
> >
> > The limitations of BTF compatibility are described in BPF Co-Re
> > (http://vger.kernel.org/bpfconf2019_talks/bpf-core.pdf, i.e. field
> > renames, #defines and changes to the signature of LSM hooks).
> >
> > This design imposes that the MAC policy (eBPF programs) be updated when
> > the inspected kernel structures change outside of BTF compatibility
> > guarantees. In practice, this is only required when a structure field
> > used by a current policy is removed (or renamed) or when the used LSM
> > hooks change. We expect the maintenance cost of these changes to be
> > acceptable as compared to the previous design
> > (https://lore.kernel.org/bpf/20190910115527.5235-1-kpsingh@chromium.org/).
> >
> 
> [...]