* [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
@ 2023-01-19 23:10 KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 1/4] kernel: Add helper macros for loop unrolling KP Singh
` (6 more replies)
0 siblings, 7 replies; 27+ messages in thread
From: KP Singh @ 2023-01-19 23:10 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, casey, song, revest, keescook
# Background
LSM hooks (callbacks) are currently invoked as indirect function calls. These
callbacks are registered into a linked list at boot time as the order of the
LSMs can be configured on the kernel command line with the "lsm=" command line
parameter.
Indirect function calls have a high overhead due to retpoline mitigation for
various speculative execution attacks.
Retpolines remain relevant even with newer generation CPUs as recently
discovered speculative attacks, like Spectre BHB need Retpolines to mitigate
against branch history injection and still need to be used in combination with
newer mitigation features like eIBRS.
This overhead is especially significant for the "bpf" LSM which allows the user
to implement LSM functionality with eBPF program. In order to facilitate this
the "bpf" LSM provides a default callback for all LSM hooks. When enabled,
the "bpf" LSM incurs an unnecessary / avoidable indirect call. This is
especially bad in OS hot paths (e.g. in the networking stack).
This overhead prevents the adoption of bpf LSM on performance critical
systems, and also, in general, slows down all LSMs.
Since we know the address of the enabled LSM callbacks at compile time and only
the order is determined at boot time, the LSM framework can allocate static
calls for each of the possible LSM callbacks and these calls can be updated once
the order is determined at boot.
This series is a respin of the RFC proposed by Paul Renauld (renauld@google.com)
and Brendan Jackman (jackmanb@google.com) [1]
# Performance improvement
With this patch-set some syscalls with lots of LSM hooks in their path
benefitted at an average of ~3%. Here are the results of the relevant Unixbench
system benchmarks with BPF LSM and a major LSM (in this case apparmor) enabled
with and without the series.
Benchmark Delta(%): (+ is better)
===============================================================================
Execl Throughput +2.9015
File Write 1024 bufsize 2000 maxblocks +5.4196
Pipe Throughput +7.7434
Pipe-based Context Switching +3.5118
Process Creation +0.3552
Shell Scripts (1 concurrent) +1.7106
System Call Overhead +3.0067
System Benchmarks Index Score (Partial Only): +3.1809
In the best case, some syscalls like eventfd_create benefitted to about ~10%.
The full analysis can be viewed at https://kpsingh.ch/lsm-perf
[1] https://lore.kernel.org/linux-security-module/20200820164753.3256899-1-jackmanb@chromium.org/
KP Singh (4):
kernel: Add helper macros for loop unrolling
security: Generate a header with the count of enabled LSMs
security: Replace indirect LSM hook calls with static calls
bpf: Only enable BPF LSM hooks when an LSM program is attached
include/linux/bpf.h | 1 +
include/linux/bpf_lsm.h | 1 +
include/linux/lsm_hooks.h | 94 +++++++++++--
include/linux/unroll.h | 35 +++++
kernel/bpf/trampoline.c | 29 ++++-
scripts/Makefile | 1 +
scripts/security/.gitignore | 1 +
scripts/security/Makefile | 4 +
scripts/security/gen_lsm_count.c | 57 ++++++++
security/Makefile | 11 ++
security/bpf/hooks.c | 26 +++-
security/security.c | 217 ++++++++++++++++++++-----------
12 files changed, 386 insertions(+), 91 deletions(-)
create mode 100644 include/linux/unroll.h
create mode 100644 scripts/security/.gitignore
create mode 100644 scripts/security/Makefile
create mode 100644 scripts/security/gen_lsm_count.c
--
2.39.0.246.g2a6d74b583-goog
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next 1/4] kernel: Add helper macros for loop unrolling
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
@ 2023-01-19 23:10 ` KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs KP Singh
` (5 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: KP Singh @ 2023-01-19 23:10 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, casey, song, revest,
keescook, KP Singh
This helps in easily initializing blocks of code (e.g. static calls and
keys).
UNROLL(N, MACRO, __VA_ARGS__) calls MACRO N times with the first
argument as the index of the iteration. This allows string pasting to
create unique tokens for variable names, function calls etc.
As an example:
#include <linux/unroll.h>
#define MACRO(N, a, b) \
int add_##N(int a, int b) \
{ \
return a + b + N; \
}
UNROLL(2, MACRO, x, y)
expands to:
int add_0(int x, int y)
{
return x + y + 0;
}
int add_1(int x, int y)
{
return x + y + 1;
}
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/unroll.h | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
create mode 100644 include/linux/unroll.h
diff --git a/include/linux/unroll.h b/include/linux/unroll.h
new file mode 100644
index 000000000000..e19aef95b94b
--- /dev/null
+++ b/include/linux/unroll.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2020 Google LLC.
+ */
+
+#ifndef __UNROLL_H
+#define __UNROLL_H
+
+#define __UNROLL_CONCAT(a, b) a ## _ ## b
+#define UNROLL(N, MACRO, args...) __UNROLL_CONCAT(__UNROLL, N)(MACRO, args)
+
+#define __UNROLL_0(MACRO, args...)
+#define __UNROLL_1(MACRO, args...) __UNROLL_0(MACRO, args) MACRO(0, args)
+#define __UNROLL_2(MACRO, args...) __UNROLL_1(MACRO, args) MACRO(1, args)
+#define __UNROLL_3(MACRO, args...) __UNROLL_2(MACRO, args) MACRO(2, args)
+#define __UNROLL_4(MACRO, args...) __UNROLL_3(MACRO, args) MACRO(3, args)
+#define __UNROLL_5(MACRO, args...) __UNROLL_4(MACRO, args) MACRO(4, args)
+#define __UNROLL_6(MACRO, args...) __UNROLL_5(MACRO, args) MACRO(5, args)
+#define __UNROLL_7(MACRO, args...) __UNROLL_6(MACRO, args) MACRO(6, args)
+#define __UNROLL_8(MACRO, args...) __UNROLL_7(MACRO, args) MACRO(7, args)
+#define __UNROLL_9(MACRO, args...) __UNROLL_8(MACRO, args) MACRO(8, args)
+#define __UNROLL_10(MACRO, args...) __UNROLL_9(MACRO, args) MACRO(9, args)
+#define __UNROLL_11(MACRO, args...) __UNROLL_10(MACRO, args) MACRO(10, args)
+#define __UNROLL_12(MACRO, args...) __UNROLL_11(MACRO, args) MACRO(11, args)
+#define __UNROLL_13(MACRO, args...) __UNROLL_12(MACRO, args) MACRO(12, args)
+#define __UNROLL_14(MACRO, args...) __UNROLL_13(MACRO, args) MACRO(13, args)
+#define __UNROLL_15(MACRO, args...) __UNROLL_14(MACRO, args) MACRO(14, args)
+#define __UNROLL_16(MACRO, args...) __UNROLL_15(MACRO, args) MACRO(15, args)
+#define __UNROLL_17(MACRO, args...) __UNROLL_16(MACRO, args) MACRO(16, args)
+#define __UNROLL_18(MACRO, args...) __UNROLL_17(MACRO, args) MACRO(17, args)
+#define __UNROLL_19(MACRO, args...) __UNROLL_18(MACRO, args) MACRO(18, args)
+#define __UNROLL_20(MACRO, args...) __UNROLL_19(MACRO, args) MACRO(19, args)
+
+#endif /* __UNROLL_H */
--
2.39.0.246.g2a6d74b583-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 1/4] kernel: Add helper macros for loop unrolling KP Singh
@ 2023-01-19 23:10 ` KP Singh
2023-01-20 1:32 ` Casey Schaufler
2023-01-19 23:10 ` [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls KP Singh
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: KP Singh @ 2023-01-19 23:10 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, casey, song, revest,
keescook, KP Singh
The header defines a MAX_LSM_COUNT constant which is used in a
subsequent patch to generate the static calls for each LSM hook which
are named using preprocessor token pasting. Since token pasting does not
work with arithmetic expressions, generate a simple lsm_count.h header
which represents the subset of LSMs that can be enabled on a given
kernel based on the config.
While one can generate static calls for all the possible LSMs that the
kernel has, this is actually wasteful as most kernels only enable a
handful of LSMs.
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
scripts/Makefile | 1 +
scripts/security/.gitignore | 1 +
scripts/security/Makefile | 4 +++
scripts/security/gen_lsm_count.c | 57 ++++++++++++++++++++++++++++++++
security/Makefile | 11 ++++++
5 files changed, 74 insertions(+)
create mode 100644 scripts/security/.gitignore
create mode 100644 scripts/security/Makefile
create mode 100644 scripts/security/gen_lsm_count.c
diff --git a/scripts/Makefile b/scripts/Makefile
index 1575af84d557..9712249c0fb3 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -41,6 +41,7 @@ targets += module.lds
subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
subdir-$(CONFIG_MODVERSIONS) += genksyms
subdir-$(CONFIG_SECURITY_SELINUX) += selinux
+subdir-$(CONFIG_SECURITY) += security
# Let clean descend into subdirs
subdir- += basic dtc gdb kconfig mod
diff --git a/scripts/security/.gitignore b/scripts/security/.gitignore
new file mode 100644
index 000000000000..684af16735f1
--- /dev/null
+++ b/scripts/security/.gitignore
@@ -0,0 +1 @@
+gen_lsm_count
diff --git a/scripts/security/Makefile b/scripts/security/Makefile
new file mode 100644
index 000000000000..05f7e4109052
--- /dev/null
+++ b/scripts/security/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+hostprogs-always-y += gen_lsm_count
+HOST_EXTRACFLAGS += \
+ -I$(srctree)/include/uapi -I$(srctree)/include
diff --git a/scripts/security/gen_lsm_count.c b/scripts/security/gen_lsm_count.c
new file mode 100644
index 000000000000..a9a227724d84
--- /dev/null
+++ b/scripts/security/gen_lsm_count.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* NOTE: we really do want to use the kernel headers here */
+#define __EXPORTED_HEADERS__
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <ctype.h>
+
+#include <linux/kconfig.h>
+
+#define GEN_MAX_LSM_COUNT ( \
+ /* Capabilities */ \
+ IS_ENABLED(CONFIG_SECURITY) + \
+ IS_ENABLED(CONFIG_SECURITY_SELINUX) + \
+ IS_ENABLED(CONFIG_SECURITY_SMACK) + \
+ IS_ENABLED(CONFIG_SECURITY_TOMOYO) + \
+ IS_ENABLED(CONFIG_SECURITY_APPARMOR) + \
+ IS_ENABLED(CONFIG_SECURITY_YAMA) + \
+ IS_ENABLED(CONFIG_SECURITY_LOADPIN) + \
+ IS_ENABLED(CONFIG_SECURITY_SAFESETID) + \
+ IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) + \
+ IS_ENABLED(CONFIG_BPF_LSM) + \
+ IS_ENABLED(CONFIG_SECURITY_LANDLOCK))
+
+const char *progname;
+
+static void usage(void)
+{
+ printf("usage: %s lsm_count.h\n", progname);
+ exit(1);
+}
+
+int main(int argc, char *argv[])
+{
+ FILE *fout;
+
+ progname = argv[0];
+
+ if (argc < 2)
+ usage();
+
+ fout = fopen(argv[1], "w");
+ if (!fout) {
+ fprintf(stderr, "Could not open %s for writing: %s\n",
+ argv[1], strerror(errno));
+ exit(2);
+ }
+
+ fprintf(fout, "#ifndef _LSM_COUNT_H_\n#define _LSM_COUNT_H_\n\n");
+ fprintf(fout, "\n#define MAX_LSM_COUNT %d\n", GEN_MAX_LSM_COUNT);
+ fprintf(fout, "#endif /* _LSM_COUNT_H_ */\n");
+ exit(0);
+}
diff --git a/security/Makefile b/security/Makefile
index 18121f8f85cd..7a47174831f4 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -3,6 +3,7 @@
# Makefile for the kernel security code
#
+gen := include/generated
obj-$(CONFIG_KEYS) += keys/
# always enable default capabilities
@@ -27,3 +28,13 @@ obj-$(CONFIG_SECURITY_LANDLOCK) += landlock/
# Object integrity file lists
obj-$(CONFIG_INTEGRITY) += integrity/
+
+$(addprefix $(obj)/,$(obj-y)): $(gen)/lsm_count.h
+
+quiet_cmd_lsm_count = GEN ${gen}/lsm_count.h
+ cmd_lsm_count = scripts/security/gen_lsm_count ${gen}/lsm_count.h
+
+targets += lsm_count.h
+
+${gen}/lsm_count.h: FORCE
+ $(call if_changed,lsm_count)
--
2.39.0.246.g2a6d74b583-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 1/4] kernel: Add helper macros for loop unrolling KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs KP Singh
@ 2023-01-19 23:10 ` KP Singh
2023-01-20 1:43 ` Casey Schaufler
2023-01-19 23:10 ` [PATCH bpf-next 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: KP Singh @ 2023-01-19 23:10 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, casey, song, revest,
keescook, KP Singh
LSM hooks are currently invoked from a linked list as indirect calls
which are invoked using retpolines as a mitigation for speculative
attacks (Branch History / Target injection) and add extra overhead which
is especially bad in kernel hot paths:
security_file_ioctl:
0xffffffff814f0320 <+0>: endbr64
0xffffffff814f0324 <+4>: push %rbp
0xffffffff814f0325 <+5>: push %r15
0xffffffff814f0327 <+7>: push %r14
0xffffffff814f0329 <+9>: push %rbx
0xffffffff814f032a <+10>: mov %rdx,%rbx
0xffffffff814f032d <+13>: mov %esi,%ebp
0xffffffff814f032f <+15>: mov %rdi,%r14
0xffffffff814f0332 <+18>: mov $0xffffffff834a7030,%r15
0xffffffff814f0339 <+25>: mov (%r15),%r15
0xffffffff814f033c <+28>: test %r15,%r15
0xffffffff814f033f <+31>: je 0xffffffff814f0358 <security_file_ioctl+56>
0xffffffff814f0341 <+33>: mov 0x18(%r15),%r11
0xffffffff814f0345 <+37>: mov %r14,%rdi
0xffffffff814f0348 <+40>: mov %ebp,%esi
0xffffffff814f034a <+42>: mov %rbx,%rdx
>>> 0xffffffff814f034d <+45>: call 0xffffffff81f742e0 <__x86_indirect_thunk_array+352>
Indirect calls that use retpolines leading to overhead, not just due
to extra instruction but also branch misses.
0xffffffff814f0352 <+50>: test %eax,%eax
0xffffffff814f0354 <+52>: je 0xffffffff814f0339 <security_file_ioctl+25>
0xffffffff814f0356 <+54>: jmp 0xffffffff814f035a <security_file_ioctl+58>
0xffffffff814f0358 <+56>: xor %eax,%eax
0xffffffff814f035a <+58>: pop %rbx
0xffffffff814f035b <+59>: pop %r14
0xffffffff814f035d <+61>: pop %r15
0xffffffff814f035f <+63>: pop %rbp
0xffffffff814f0360 <+64>: jmp 0xffffffff81f747c4 <__x86_return_thunk>
The indirect calls are not really needed as one knows the addresses of
enabled LSM callbacks at boot time and only the order can possibly
change at boot time with the lsm= kernel command line parameter.
An array of static calls is defined per LSM hook and the static calls
are updated at boot time once the order has been determined.
A static key guards whether an LSM static call is enabled or not,
without this static key, for LSM hooks that return an int, the presence
of the hook that returns a default value can create side-effects which
has resulted in bugs [1].
With the hook now exposed as a static call, one can see that the
retpolines are no longer there and the LSM callbacks are invoked
directly:
security_file_ioctl:
0xffffffff814f0dd0 <+0>: endbr64
0xffffffff814f0dd4 <+4>: push %rbp
0xffffffff814f0dd5 <+5>: push %r14
0xffffffff814f0dd7 <+7>: push %rbx
0xffffffff814f0dd8 <+8>: mov %rdx,%rbx
0xffffffff814f0ddb <+11>: mov %esi,%ebp
0xffffffff814f0ddd <+13>: mov %rdi,%r14
>>> 0xffffffff814f0de0 <+16>: jmp 0xffffffff814f0df1 <security_file_ioctl+33>
Static key enabled for selinux_file_ioctl
>>> 0xffffffff814f0de2 <+18>: jmp 0xffffffff814f0e08 <security_file_ioctl+56>
Static key enabled for bpf_lsm_file_ioctl. This is something that is
changed to default to false to avoid the existing side effect issues
of BPF LSM [1]
0xffffffff814f0de4 <+20>: xor %eax,%eax
0xffffffff814f0de6 <+22>: xchg %ax,%ax
0xffffffff814f0de8 <+24>: pop %rbx
0xffffffff814f0de9 <+25>: pop %r14
0xffffffff814f0deb <+27>: pop %rbp
0xffffffff814f0dec <+28>: jmp 0xffffffff81f767c4 <__x86_return_thunk>
0xffffffff814f0df1 <+33>: endbr64
0xffffffff814f0df5 <+37>: mov %r14,%rdi
0xffffffff814f0df8 <+40>: mov %ebp,%esi
0xffffffff814f0dfa <+42>: mov %rbx,%rdx
>>> 0xffffffff814f0dfd <+45>: call 0xffffffff814fe820 <selinux_file_ioctl>
Direct call to SELinux.
0xffffffff814f0e02 <+50>: test %eax,%eax
0xffffffff814f0e04 <+52>: jne 0xffffffff814f0de8 <security_file_ioctl+24>
0xffffffff814f0e06 <+54>: jmp 0xffffffff814f0de2 <security_file_ioctl+18>
0xffffffff814f0e08 <+56>: endbr64
0xffffffff814f0e0c <+60>: mov %r14,%rdi
0xffffffff814f0e0f <+63>: mov %ebp,%esi
0xffffffff814f0e11 <+65>: mov %rbx,%rdx
>>> 0xffffffff814f0e14 <+68>: call 0xffffffff8123b7d0 <bpf_lsm_file_ioctl>
Direct call to bpf_lsm_file_ioctl
0xffffffff814f0e19 <+73>: test %eax,%eax
0xffffffff814f0e1b <+75>: jne 0xffffffff814f0de8 <security_file_ioctl+24>
0xffffffff814f0e1d <+77>: jmp 0xffffffff814f0de4 <security_file_ioctl+20>
0xffffffff814f0e1f <+79>: endbr64
0xffffffff814f0e23 <+83>: mov %r14,%rdi
0xffffffff814f0e26 <+86>: mov %ebp,%esi
0xffffffff814f0e28 <+88>: mov %rbx,%rdx
0xffffffff814f0e2b <+91>: pop %rbx
0xffffffff814f0e2c <+92>: pop %r14
0xffffffff814f0e2e <+94>: pop %rbp
0xffffffff814f0e2f <+95>: ret
0xffffffff814f0e30 <+96>: int3
0xffffffff814f0e31 <+97>: int3
0xffffffff814f0e32 <+98>: int3
0xffffffff814f0e33 <+99>: int3
There are some hooks that don't use the call_int_hook and
call_void_hook. These hooks are updated to use a new macro called
security_for_each_hook where the lsm_callback is directly invoked as an
indirect call. Currently, there are no performance sensitive hooks that
use the security_for_each_hook macro. However, if, some performance
sensitive hooks are discovered, these can be updated to use
static calls with loop unrolling as well using a custom macro.
[1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/lsm_hooks.h | 83 +++++++++++++--
security/security.c | 216 ++++++++++++++++++++++++--------------
2 files changed, 211 insertions(+), 88 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 0a5ba81f7367..c82d15a4ef50 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -28,6 +28,26 @@
#include <linux/security.h>
#include <linux/init.h>
#include <linux/rculist.h>
+#include <linux/static_call.h>
+#include <linux/unroll.h>
+#include <linux/jump_label.h>
+
+/* Include the generated MAX_LSM_COUNT */
+#include <generated/lsm_count.h>
+
+#define SECURITY_HOOK_ENABLED_KEY(HOOK, IDX) security_enabled_key_##HOOK##_##IDX
+
+/*
+ * Identifier for the LSM static calls.
+ * HOOK is an LSM hook as defined in linux/lsm_hookdefs.h
+ * IDX is the index of the static call. 0 <= NUM < MAX_LSM_COUNT
+ */
+#define LSM_STATIC_CALL(HOOK, IDX) lsm_static_call_##HOOK##_##IDX
+
+/*
+ * Call the macro M for each LSM hook MAX_LSM_COUNT times.
+ */
+#define LSM_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__)
/**
* union security_list_options - Linux Security Module hook function list
@@ -1657,21 +1677,48 @@ union security_list_options {
#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
#include "lsm_hook_defs.h"
#undef LSM_HOOK
+ void *lsm_callback;
};
-struct security_hook_heads {
- #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME;
- #include "lsm_hook_defs.h"
+/*
+ * @key: static call key as defined by STATIC_CALL_KEY
+ * @trampoline: static call trampoline as defined by STATIC_CALL_TRAMP
+ * @hl: The security_hook_list as initialized by the owning LSM.
+ * @enabled_key: Enabled when the static call has an LSM hook associated.
+ */
+struct lsm_static_call {
+ struct static_call_key *key;
+ void *trampoline;
+ struct security_hook_list *hl;
+ struct static_key *enabled_key;
+};
+
+/*
+ * Table of the static calls for each LSM hook.
+ * Once the LSMs are initialized, their callbacks will be copied to these
+ * tables such that the calls are filled backwards (from last to first).
+ * This way, we can jump directly to the first used static call, and execute
+ * all of them after. This essentially makes the entry point
+ * dynamic to adapt the number of static calls to the number of callbacks.
+ */
+struct lsm_static_calls_table {
+ #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+ struct lsm_static_call NAME[MAX_LSM_COUNT];
+ #include <linux/lsm_hook_defs.h>
#undef LSM_HOOK
} __randomize_layout;
/*
* Security module hook list structure.
* For use with generic list macros for common operations.
+ *
+ * struct security_hook_list - Contents of a cacheable, mappable object.
+ * @scalls: The beginning of the array of static calls assigned to this hook.
+ * @hook: The callback for the hook.
+ * @lsm: The name of the lsm that owns this hook.
*/
struct security_hook_list {
- struct hlist_node list;
- struct hlist_head *head;
+ struct lsm_static_call *scalls;
union security_list_options hook;
const char *lsm;
} __randomize_layout;
@@ -1701,10 +1748,12 @@ struct lsm_blob_sizes {
* care of the common case and reduces the amount of
* text involved.
*/
-#define LSM_HOOK_INIT(HEAD, HOOK) \
- { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
+#define LSM_HOOK_INIT(NAME, CALLBACK) \
+ { \
+ .scalls = static_calls_table.NAME, \
+ .hook = { .NAME = CALLBACK } \
+ }
-extern struct security_hook_heads security_hook_heads;
extern char *lsm_names;
extern void security_add_hooks(struct security_hook_list *hooks, int count,
@@ -1756,10 +1805,21 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
static inline void security_delete_hooks(struct security_hook_list *hooks,
int count)
{
- int i;
+ struct lsm_static_call *scalls;
+ int i, j;
+
+ for (i = 0; i < count; i++) {
+ scalls = hooks[i].scalls;
+ for (j = 0; j < MAX_LSM_COUNT; j++) {
+ if (scalls[j].hl != &hooks[i])
+ continue;
- for (i = 0; i < count; i++)
- hlist_del_rcu(&hooks[i].list);
+ static_key_disable(scalls[j].enabled_key);
+ __static_call_update(scalls[j].key,
+ scalls[j].trampoline, NULL);
+ scalls[j].hl = NULL;
+ }
+ }
}
#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
@@ -1771,5 +1831,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
extern int lsm_inode_alloc(struct inode *inode);
+extern struct lsm_static_calls_table static_calls_table __ro_after_init;
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/security.c b/security/security.c
index d1571900a8c7..e54d5ba187d1 100644
--- a/security/security.c
+++ b/security/security.c
@@ -29,6 +29,8 @@
#include <linux/string.h>
#include <linux/msg.h>
#include <net/flow.h>
+#include <linux/static_call.h>
+#include <linux/jump_label.h>
#define MAX_LSM_EVM_XATTR 2
@@ -74,7 +76,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
[LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
};
-struct security_hook_heads security_hook_heads __lsm_ro_after_init;
static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
static struct kmem_cache *lsm_file_cache;
@@ -93,6 +94,43 @@ static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
static __initdata struct lsm_info **ordered_lsms;
static __initdata struct lsm_info *exclusive;
+/*
+ * Define static calls and static keys for each LSM hook.
+ */
+
+#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...) \
+ DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM), \
+ *((RET(*)(__VA_ARGS__))NULL)); \
+ DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ENABLED_KEY(NAME, NUM));
+
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+ LSM_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__)
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+#undef DEFINE_LSM_STATIC_CALL
+
+/*
+ * Initialise a table of static calls for each LSM hook.
+ * DEFINE_STATIC_CALL_NULL invocation above generates a key (STATIC_CALL_KEY)
+ * and a trampoline (STATIC_CALL_TRAMP) which are used to call
+ * __static_call_update when updating the static call.
+ */
+struct lsm_static_calls_table static_calls_table __lsm_ro_after_init = {
+#define INIT_LSM_STATIC_CALL(NUM, NAME) \
+ (struct lsm_static_call) { \
+ .key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)), \
+ .trampoline = &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM)),\
+ .enabled_key = &SECURITY_HOOK_ENABLED_KEY(NAME, NUM).key,\
+ },
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+ .NAME = { \
+ LSM_UNROLL(INIT_LSM_STATIC_CALL, NAME) \
+ },
+#include <linux/lsm_hook_defs.h>
+#undef LSM_HOOK
+#undef INIT_LSM_STATIC_CALL
+};
+
static __initdata bool debug;
#define init_debug(...) \
do { \
@@ -153,7 +191,7 @@ static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from)
if (exists_ordered_lsm(lsm))
return;
- if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM slots!?\n", from))
+ if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM static calls!?\n", from))
return;
/* Enable this LSM, if it is not already set. */
@@ -318,6 +356,24 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
kfree(sep);
}
+static void __init lsm_static_call_init(struct security_hook_list *hl)
+{
+ struct lsm_static_call *scall = hl->scalls;
+ int i;
+
+ for (i = 0; i < MAX_LSM_COUNT; i++) {
+ /* Update the first static call that is not used yet */
+ if (!scall->hl) {
+ __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_callback);
+ scall->hl = hl;
+ static_key_enable(scall->enabled_key);
+ return;
+ }
+ scall++;
+ }
+ panic("%s - No static call remaining to add LSM hook.\n", __func__);
+}
+
static void __init lsm_early_cred(struct cred *cred);
static void __init lsm_early_task(struct task_struct *task);
@@ -395,11 +451,6 @@ int __init early_security_init(void)
{
struct lsm_info *lsm;
-#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
- INIT_HLIST_HEAD(&security_hook_heads.NAME);
-#include "linux/lsm_hook_defs.h"
-#undef LSM_HOOK
-
for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) {
if (!lsm->enabled)
lsm->enabled = &lsm_enabled_true;
@@ -515,7 +566,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
for (i = 0; i < count; i++) {
hooks[i].lsm = lsm;
- hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+ lsm_static_call_init(&hooks[i]);
}
/*
@@ -753,28 +804,42 @@ static int lsm_superblock_alloc(struct super_block *sb)
* call_int_hook:
* This is a hook that returns a value.
*/
+#define __CALL_STATIC_VOID(NUM, HOOK, ...) \
+ if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \
+ static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
+ }
-#define call_void_hook(FUNC, ...) \
- do { \
- struct security_hook_list *P; \
- \
- hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
- P->hook.FUNC(__VA_ARGS__); \
+#define call_void_hook(FUNC, ...) \
+ do { \
+ LSM_UNROLL(__CALL_STATIC_VOID, FUNC, __VA_ARGS__) \
} while (0)
-#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; \
-})
+#define __CALL_STATIC_INT(NUM, R, HOOK, ...) \
+ if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \
+ R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
+ if (R != 0) \
+ goto out; \
+ }
+
+#define call_int_hook(FUNC, IRC, ...) \
+ ({ \
+ __label__ out; \
+ int RC = IRC; \
+ do { \
+ LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__) \
+ \
+ } while (0); \
+out: \
+ RC; \
+ })
+
+#define security_for_each_hook(scall, NAME, expression) \
+ for (scall = static_calls_table.NAME; \
+ scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) { \
+ if (!static_key_enabled(scall->enabled_key)) \
+ continue; \
+ (expression); \
+ }
/* Security operations */
@@ -859,7 +924,7 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz)
int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int cap_sys_admin = 1;
int rc;
@@ -870,13 +935,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
* agree that it should be set it will. If any module
* thinks it should not be set it won't.
*/
- hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
- rc = hp->hook.vm_enough_memory(mm, pages);
+ security_for_each_hook(scall, vm_enough_memory, ({
+ rc = scall->hl->hook.vm_enough_memory(mm, pages);
if (rc <= 0) {
cap_sys_admin = 0;
break;
}
- }
+ }));
return __vm_enough_memory(mm, pages, cap_sys_admin);
}
@@ -918,18 +983,17 @@ int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
int security_fs_context_parse_param(struct fs_context *fc,
struct fs_parameter *param)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int trc;
int rc = -ENOPARAM;
- hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param,
- list) {
- trc = hp->hook.fs_context_parse_param(fc, param);
+ security_for_each_hook(scall, fs_context_parse_param, ({
+ trc = scall->hl->hook.fs_context_parse_param(fc, param);
if (trc == 0)
rc = 0;
else if (trc != -ENOPARAM)
return trc;
- }
+ }));
return rc;
}
@@ -1092,18 +1156,19 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
const char **xattr_name, void **ctx,
u32 *ctxlen)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc;
/*
* Only one module will provide a security context.
*/
- hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
- rc = hp->hook.dentry_init_security(dentry, mode, name,
+ security_for_each_hook(scall, dentry_init_security, ({
+ rc = scall->hl->hook.dentry_init_security(dentry, mode, name,
xattr_name, ctx, ctxlen);
if (rc != LSM_RET_DEFAULT(dentry_init_security))
return rc;
- }
+ }));
+
return LSM_RET_DEFAULT(dentry_init_security);
}
EXPORT_SYMBOL(security_dentry_init_security);
@@ -1502,7 +1567,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
struct inode *inode, const char *name,
void **buffer, bool alloc)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc;
if (unlikely(IS_PRIVATE(inode)))
@@ -1510,17 +1575,17 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
/*
* Only one module will provide an attribute with a given name.
*/
- hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
- rc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
+ security_for_each_hook(scall, inode_getsecurity, ({
+ rc = scall->hl->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
if (rc != LSM_RET_DEFAULT(inode_getsecurity))
return rc;
- }
+ }));
return LSM_RET_DEFAULT(inode_getsecurity);
}
int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc;
if (unlikely(IS_PRIVATE(inode)))
@@ -1528,12 +1593,11 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
/*
* Only one module will provide an attribute with a given name.
*/
- hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
- rc = hp->hook.inode_setsecurity(inode, name, value, size,
- flags);
+ security_for_each_hook(scall, inode_setsecurity, ({
+ rc = scall->hl->hook.inode_setsecurity(inode, name, value, size, flags);
if (rc != LSM_RET_DEFAULT(inode_setsecurity))
return rc;
- }
+ }));
return LSM_RET_DEFAULT(inode_setsecurity);
}
@@ -1558,7 +1622,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
int security_inode_copy_up_xattr(const char *name)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc;
/*
@@ -1566,12 +1630,11 @@ int security_inode_copy_up_xattr(const char *name)
* xattr), -EOPNOTSUPP if it does not know anything about the xattr or
* any other error code incase of an error.
*/
- hlist_for_each_entry(hp,
- &security_hook_heads.inode_copy_up_xattr, list) {
- rc = hp->hook.inode_copy_up_xattr(name);
+ security_for_each_hook(scall, inode_copy_up_xattr, ({
+ rc = scall->hl->hook.inode_copy_up_xattr(name);
if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
return rc;
- }
+ }));
return LSM_RET_DEFAULT(inode_copy_up_xattr);
}
@@ -1968,16 +2031,16 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
{
int thisrc;
int rc = LSM_RET_DEFAULT(task_prctl);
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
- hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
- thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
+ security_for_each_hook(scall, task_prctl, ({
+ thisrc = scall->hl->hook.task_prctl(option, arg2, arg3, arg4, arg5);
if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
rc = thisrc;
if (thisrc != 0)
break;
}
- }
+ }));
return rc;
}
@@ -2142,26 +2205,26 @@ EXPORT_SYMBOL(security_d_instantiate);
int security_getprocattr(struct task_struct *p, const char *lsm,
const char *name, char **value)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
- hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
- if (lsm != NULL && strcmp(lsm, hp->lsm))
+ security_for_each_hook(scall, getprocattr, ({
+ if (lsm != NULL && strcmp(lsm, scall->hl->lsm))
continue;
- return hp->hook.getprocattr(p, name, value);
- }
+ return scall->hl->hook.getprocattr(p, name, value);
+ }));
return LSM_RET_DEFAULT(getprocattr);
}
int security_setprocattr(const char *lsm, const char *name, void *value,
size_t size)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
- hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
- if (lsm != NULL && strcmp(lsm, hp->lsm))
+ security_for_each_hook(scall, setprocattr, ({
+ if (lsm != NULL && strcmp(lsm, scall->hl->lsm))
continue;
- return hp->hook.setprocattr(name, value, size);
- }
+ return scall->hl->hook.setprocattr(name, value, size);
+ }));
return LSM_RET_DEFAULT(setprocattr);
}
@@ -2178,18 +2241,18 @@ EXPORT_SYMBOL(security_ismaclabel);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc;
/*
* Currently, only one LSM can implement secid_to_secctx (i.e this
* LSM hook is not "stackable").
*/
- hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
- rc = hp->hook.secid_to_secctx(secid, secdata, seclen);
+ security_for_each_hook(scall, secid_to_secctx, ({
+ rc = scall->hl->hook.secid_to_secctx(secid, secdata, seclen);
if (rc != LSM_RET_DEFAULT(secid_to_secctx))
return rc;
- }
+ }));
return LSM_RET_DEFAULT(secid_to_secctx);
}
@@ -2582,7 +2645,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
struct xfrm_policy *xp,
const struct flowi_common *flic)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
/*
@@ -2594,11 +2657,10 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
* For speed optimization, we explicitly break the loop rather than
* using the macro
*/
- hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
- list) {
- rc = hp->hook.xfrm_state_pol_flow_match(x, xp, flic);
+ security_for_each_hook(scall, xfrm_state_pol_flow_match, ({
+ rc = scall->hl->hook.xfrm_state_pol_flow_match(x, xp, flic);
break;
- }
+ }));
return rc;
}
--
2.39.0.246.g2a6d74b583-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH bpf-next 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
` (2 preceding siblings ...)
2023-01-19 23:10 ` [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls KP Singh
@ 2023-01-19 23:10 ` KP Singh
2023-01-20 1:13 ` [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls Casey Schaufler
` (2 subsequent siblings)
6 siblings, 0 replies; 27+ messages in thread
From: KP Singh @ 2023-01-19 23:10 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, casey, song, revest,
keescook, KP Singh
BPF LSM hooks have side-effects (even when a default value is returned),
as some hooks end up behaving differently due to the very presence of
the hook.
The static keys guarding the BPF LSM hooks are disabled by default and
enabled only when a BPF program is attached implementing the hook
logic. This avoids the issue of the side-effects and also the minor
overhead associated with the empty callback.
security_file_ioctl:
0xffffffff814f0e90 <+0>: endbr64
0xffffffff814f0e94 <+4>: push %rbp
0xffffffff814f0e95 <+5>: push %r14
0xffffffff814f0e97 <+7>: push %rbx
0xffffffff814f0e98 <+8>: mov %rdx,%rbx
0xffffffff814f0e9b <+11>: mov %esi,%ebp
0xffffffff814f0e9d <+13>: mov %rdi,%r14
>>> 0xffffffff814f0ea0 <+16>: jmp 0xffffffff814f0eb1 <security_file_ioctl+33>
Static key enabled for SELinux
0xffffffff814f0ea2 <+18>: xchg %ax,%ax
Static key disabled for BPF. This gets patched to a jmp when a BPF
LSM program is attached.
0xffffffff814f0ea4 <+20>: xor %eax,%eax
0xffffffff814f0ea6 <+22>: xchg %ax,%ax
0xffffffff814f0ea8 <+24>: pop %rbx
0xffffffff814f0ea9 <+25>: pop %r14
0xffffffff814f0eab <+27>: pop %rbp
0xffffffff814f0eac <+28>: jmp 0xffffffff81f767c4 <__x86_return_thunk>
0xffffffff814f0eb1 <+33>: endbr64
0xffffffff814f0eb5 <+37>: mov %r14,%rdi
0xffffffff814f0eb8 <+40>: mov %ebp,%esi
0xffffffff814f0eba <+42>: mov %rbx,%rdx
0xffffffff814f0ebd <+45>: call 0xffffffff814fe8e0 <selinux_file_ioctl>
0xffffffff814f0ec2 <+50>: test %eax,%eax
0xffffffff814f0ec4 <+52>: jne 0xffffffff814f0ea8 <security_file_ioctl+24>
0xffffffff814f0ec6 <+54>: jmp 0xffffffff814f0ea2 <security_file_ioctl+18>
0xffffffff814f0ec8 <+56>: endbr64
0xffffffff814f0ecc <+60>: mov %r14,%rdi
0xffffffff814f0ecf <+63>: mov %ebp,%esi
0xffffffff814f0ed1 <+65>: mov %rbx,%rdx
0xffffffff814f0ed4 <+68>: call 0xffffffff8123b890 <bpf_lsm_file_ioctl>
0xffffffff814f0ed9 <+73>: test %eax,%eax
0xffffffff814f0edb <+75>: jne 0xffffffff814f0ea8 <security_file_ioctl+24>
0xffffffff814f0edd <+77>: jmp 0xffffffff814f0ea4 <security_file_ioctl+20>
0xffffffff814f0edf <+79>: endbr64
0xffffffff814f0ee3 <+83>: mov %r14,%rdi
0xffffffff814f0ee6 <+86>: mov %ebp,%esi
0xffffffff814f0ee8 <+88>: mov %rbx,%rdx
0xffffffff814f0eeb <+91>: pop %rbx
0xffffffff814f0eec <+92>: pop %r14
0xffffffff814f0eee <+94>: pop %rbp
0xffffffff814f0eef <+95>: ret
0xffffffff814f0ef0 <+96>: int3
0xffffffff814f0ef1 <+97>: int3
0xffffffff814f0ef2 <+98>: int3
0xffffffff814f0ef3 <+99>: int3
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/bpf.h | 1 +
include/linux/bpf_lsm.h | 1 +
include/linux/lsm_hooks.h | 13 ++++++++++++-
kernel/bpf/trampoline.c | 29 +++++++++++++++++++++++++++--
security/bpf/hooks.c | 26 +++++++++++++++++++++++++-
security/security.c | 3 ++-
6 files changed, 68 insertions(+), 5 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index ae7771c7d750..4008f4a00851 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1049,6 +1049,7 @@ struct bpf_attach_target_info {
long tgt_addr;
const char *tgt_name;
const struct btf_type *tgt_type;
+ bool is_lsm_target;
};
#define BPF_DISPATCHER_MAX 48 /* Fits in 2048B */
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 1de7ece5d36d..cce615af93d5 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -29,6 +29,7 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
bool bpf_lsm_is_sleepable_hook(u32 btf_id);
bool bpf_lsm_is_trusted(const struct bpf_prog *prog);
+void bpf_lsm_toggle_hook(void *addr, bool value);
static inline struct bpf_storage_blob *bpf_inode(
const struct inode *inode)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c82d15a4ef50..5e85d3340a07 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1716,11 +1716,14 @@ struct lsm_static_calls_table {
* @scalls: The beginning of the array of static calls assigned to this hook.
* @hook: The callback for the hook.
* @lsm: The name of the lsm that owns this hook.
+ * @default_state: The state of the LSM hook when initialized. If set to false,
+ * the static key guarding the hook will be set to disabled.
*/
struct security_hook_list {
struct lsm_static_call *scalls;
union security_list_options hook;
const char *lsm;
+ bool default_state;
} __randomize_layout;
/*
@@ -1751,7 +1754,15 @@ struct lsm_blob_sizes {
#define LSM_HOOK_INIT(NAME, CALLBACK) \
{ \
.scalls = static_calls_table.NAME, \
- .hook = { .NAME = CALLBACK } \
+ .hook = { .NAME = CALLBACK }, \
+ .default_state = true \
+ }
+
+#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK) \
+ { \
+ .scalls = static_calls_table.NAME, \
+ .hook = { .NAME = CALLBACK }, \
+ .default_state = false \
}
extern char *lsm_names;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index d0ed7d6f5eec..9789ecf6f29c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -14,6 +14,7 @@
#include <linux/bpf_verifier.h>
#include <linux/bpf_lsm.h>
#include <linux/delay.h>
+#include <linux/bpf_lsm.h>
/* dummy _ops. The verifier will operate on target program's ops. */
const struct bpf_verifier_ops bpf_extension_verifier_ops = {
@@ -536,7 +537,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
{
enum bpf_tramp_prog_type kind;
struct bpf_tramp_link *link_exiting;
- int err = 0;
+ int err = 0, num_lsm_progs = 0;
int cnt = 0, i;
kind = bpf_attach_type_to_tramp(link->link.prog);
@@ -567,8 +568,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
continue;
/* prog already linked */
return -EBUSY;
+
+ if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM)
+ num_lsm_progs++;
}
+ if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM)
+ bpf_lsm_toggle_hook(tr->func.addr, true);
+
hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
tr->progs_cnt[kind]++;
err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
@@ -591,8 +598,10 @@ int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline
static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
{
+ struct bpf_tramp_link *link_exiting;
enum bpf_tramp_prog_type kind;
- int err;
+ bool lsm_link_found = false;
+ int err, num_lsm_progs = 0;
kind = bpf_attach_type_to_tramp(link->link.prog);
if (kind == BPF_TRAMP_REPLACE) {
@@ -602,8 +611,24 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
tr->extension_prog = NULL;
return err;
}
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+ hlist_for_each_entry(link_exiting, &tr->progs_hlist[kind],
+ tramp_hlist) {
+ if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM)
+ num_lsm_progs++;
+
+ if (link_exiting->link.prog == link->link.prog)
+ lsm_link_found = true;
+ }
+ }
+
hlist_del_init(&link->tramp_hlist);
tr->progs_cnt[kind]--;
+
+ if (lsm_link_found && num_lsm_progs == 1)
+ bpf_lsm_toggle_hook(tr->func.addr, false);
+
return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
}
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index e5971fa74fd7..a39799e9f648 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -8,7 +8,7 @@
static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
- LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
+ LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME),
#include <linux/lsm_hook_defs.h>
#undef LSM_HOOK
LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
@@ -32,3 +32,27 @@ DEFINE_LSM(bpf) = {
.init = bpf_lsm_init,
.blobs = &bpf_lsm_blob_sizes
};
+
+void bpf_lsm_toggle_hook(void *addr, bool value)
+{
+ struct security_hook_list *h;
+ struct lsm_static_call *scalls;
+ int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) {
+ h = &bpf_lsm_hooks[i];
+ scalls = h->scalls;
+ if (h->hook.lsm_callback == addr)
+ continue;
+
+ for (j = 0; j < MAX_LSM_COUNT; j++) {
+ if (scalls[j].hl != h)
+ continue;
+
+ if (value)
+ static_key_enable(scalls[j].enabled_key);
+ else
+ static_key_disable(scalls[j].enabled_key);
+ }
+ }
+}
diff --git a/security/security.c b/security/security.c
index e54d5ba187d1..f74135349429 100644
--- a/security/security.c
+++ b/security/security.c
@@ -366,7 +366,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl)
if (!scall->hl) {
__static_call_update(scall->key, scall->trampoline, hl->hook.lsm_callback);
scall->hl = hl;
- static_key_enable(scall->enabled_key);
+ if (hl->default_state)
+ static_key_enable(scall->enabled_key);
return;
}
scall++;
--
2.39.0.246.g2a6d74b583-goog
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
` (3 preceding siblings ...)
2023-01-19 23:10 ` [PATCH bpf-next 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
@ 2023-01-20 1:13 ` Casey Schaufler
2023-01-20 2:17 ` KP Singh
2023-01-27 19:22 ` Song Liu
2023-01-27 20:16 ` Paul Moore
6 siblings, 1 reply; 27+ messages in thread
From: Casey Schaufler @ 2023-01-20 1:13 UTC (permalink / raw)
To: KP Singh, linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, song, revest, keescook, casey
On 1/19/2023 3:10 PM, KP Singh wrote:
> # Background
>
> LSM hooks (callbacks) are currently invoked as indirect function calls. These
> callbacks are registered into a linked list at boot time as the order of the
> LSMs can be configured on the kernel command line with the "lsm=" command line
> parameter.
>
> Indirect function calls have a high overhead due to retpoline mitigation for
> various speculative execution attacks.
>
> Retpolines remain relevant even with newer generation CPUs as recently
> discovered speculative attacks, like Spectre BHB need Retpolines to mitigate
> against branch history injection and still need to be used in combination with
> newer mitigation features like eIBRS.
>
> This overhead is especially significant for the "bpf" LSM which allows the user
> to implement LSM functionality with eBPF program. In order to facilitate this
> the "bpf" LSM provides a default callback for all LSM hooks. When enabled,
> the "bpf" LSM incurs an unnecessary / avoidable indirect call. This is
> especially bad in OS hot paths (e.g. in the networking stack).
> This overhead prevents the adoption of bpf LSM on performance critical
> systems, and also, in general, slows down all LSMs.
>
> Since we know the address of the enabled LSM callbacks at compile time and only
> the order is determined at boot time,
No quite true. A system with Smack and AppArmor compiled in will only
be allowed to use one or the other.
> the LSM framework can allocate static
> calls for each of the possible LSM callbacks and these calls can be updated once
> the order is determined at boot.
True if you also provide for the single "major" LSM restriction.
> This series is a respin of the RFC proposed by Paul Renauld (renauld@google.com)
> and Brendan Jackman (jackmanb@google.com) [1]
>
> # Performance improvement
>
> With this patch-set some syscalls with lots of LSM hooks in their path
> benefitted at an average of ~3%. Here are the results of the relevant Unixbench
> system benchmarks with BPF LSM and a major LSM (in this case apparmor) enabled
> with and without the series.
>
> Benchmark Delta(%): (+ is better)
> ===============================================================================
> Execl Throughput +2.9015
> File Write 1024 bufsize 2000 maxblocks +5.4196
> Pipe Throughput +7.7434
> Pipe-based Context Switching +3.5118
> Process Creation +0.3552
> Shell Scripts (1 concurrent) +1.7106
> System Call Overhead +3.0067
> System Benchmarks Index Score (Partial Only): +3.1809
How about socket creation and packet delivery impact? You'll need to
use either SELinux or Smack to get those numbers.
> In the best case, some syscalls like eventfd_create benefitted to about ~10%.
> The full analysis can be viewed at https://kpsingh.ch/lsm-perf
>
> [1] https://lore.kernel.org/linux-security-module/20200820164753.3256899-1-jackmanb@chromium.org/
>
> KP Singh (4):
> kernel: Add helper macros for loop unrolling
> security: Generate a header with the count of enabled LSMs
> security: Replace indirect LSM hook calls with static calls
> bpf: Only enable BPF LSM hooks when an LSM program is attached
>
> include/linux/bpf.h | 1 +
> include/linux/bpf_lsm.h | 1 +
> include/linux/lsm_hooks.h | 94 +++++++++++--
> include/linux/unroll.h | 35 +++++
> kernel/bpf/trampoline.c | 29 ++++-
> scripts/Makefile | 1 +
> scripts/security/.gitignore | 1 +
> scripts/security/Makefile | 4 +
> scripts/security/gen_lsm_count.c | 57 ++++++++
> security/Makefile | 11 ++
> security/bpf/hooks.c | 26 +++-
> security/security.c | 217 ++++++++++++++++++++-----------
> 12 files changed, 386 insertions(+), 91 deletions(-)
> create mode 100644 include/linux/unroll.h
> create mode 100644 scripts/security/.gitignore
> create mode 100644 scripts/security/Makefile
> create mode 100644 scripts/security/gen_lsm_count.c
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs
2023-01-19 23:10 ` [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs KP Singh
@ 2023-01-20 1:32 ` Casey Schaufler
2023-01-20 2:15 ` KP Singh
0 siblings, 1 reply; 27+ messages in thread
From: Casey Schaufler @ 2023-01-20 1:32 UTC (permalink / raw)
To: KP Singh, linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, song, revest, keescook, casey
On 1/19/2023 3:10 PM, KP Singh wrote:
> The header defines a MAX_LSM_COUNT constant which is used in a
> subsequent patch to generate the static calls for each LSM hook which
> are named using preprocessor token pasting. Since token pasting does not
> work with arithmetic expressions, generate a simple lsm_count.h header
> which represents the subset of LSMs that can be enabled on a given
> kernel based on the config.
>
> While one can generate static calls for all the possible LSMs that the
> kernel has, this is actually wasteful as most kernels only enable a
> handful of LSMs.
Why "generate" anything? Why not include your GEN_MAX_LSM_COUNT macro
in security.h and be done with it? I've proposed doing just that in the
stacking patch set for some time. This seems to be much more complicated
than it needs to be.
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
> scripts/Makefile | 1 +
> scripts/security/.gitignore | 1 +
> scripts/security/Makefile | 4 +++
> scripts/security/gen_lsm_count.c | 57 ++++++++++++++++++++++++++++++++
> security/Makefile | 11 ++++++
> 5 files changed, 74 insertions(+)
> create mode 100644 scripts/security/.gitignore
> create mode 100644 scripts/security/Makefile
> create mode 100644 scripts/security/gen_lsm_count.c
>
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 1575af84d557..9712249c0fb3 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -41,6 +41,7 @@ targets += module.lds
> subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
> subdir-$(CONFIG_MODVERSIONS) += genksyms
> subdir-$(CONFIG_SECURITY_SELINUX) += selinux
> +subdir-$(CONFIG_SECURITY) += security
>
> # Let clean descend into subdirs
> subdir- += basic dtc gdb kconfig mod
> diff --git a/scripts/security/.gitignore b/scripts/security/.gitignore
> new file mode 100644
> index 000000000000..684af16735f1
> --- /dev/null
> +++ b/scripts/security/.gitignore
> @@ -0,0 +1 @@
> +gen_lsm_count
> diff --git a/scripts/security/Makefile b/scripts/security/Makefile
> new file mode 100644
> index 000000000000..05f7e4109052
> --- /dev/null
> +++ b/scripts/security/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +hostprogs-always-y += gen_lsm_count
> +HOST_EXTRACFLAGS += \
> + -I$(srctree)/include/uapi -I$(srctree)/include
> diff --git a/scripts/security/gen_lsm_count.c b/scripts/security/gen_lsm_count.c
> new file mode 100644
> index 000000000000..a9a227724d84
> --- /dev/null
> +++ b/scripts/security/gen_lsm_count.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* NOTE: we really do want to use the kernel headers here */
> +#define __EXPORTED_HEADERS__
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <ctype.h>
> +
> +#include <linux/kconfig.h>
> +
> +#define GEN_MAX_LSM_COUNT ( \
> + /* Capabilities */ \
> + IS_ENABLED(CONFIG_SECURITY) + \
> + IS_ENABLED(CONFIG_SECURITY_SELINUX) + \
> + IS_ENABLED(CONFIG_SECURITY_SMACK) + \
> + IS_ENABLED(CONFIG_SECURITY_TOMOYO) + \
> + IS_ENABLED(CONFIG_SECURITY_APPARMOR) + \
> + IS_ENABLED(CONFIG_SECURITY_YAMA) + \
> + IS_ENABLED(CONFIG_SECURITY_LOADPIN) + \
> + IS_ENABLED(CONFIG_SECURITY_SAFESETID) + \
> + IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM) + \
> + IS_ENABLED(CONFIG_BPF_LSM) + \
> + IS_ENABLED(CONFIG_SECURITY_LANDLOCK))
> +
> +const char *progname;
> +
> +static void usage(void)
> +{
> + printf("usage: %s lsm_count.h\n", progname);
> + exit(1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + FILE *fout;
> +
> + progname = argv[0];
> +
> + if (argc < 2)
> + usage();
> +
> + fout = fopen(argv[1], "w");
> + if (!fout) {
> + fprintf(stderr, "Could not open %s for writing: %s\n",
> + argv[1], strerror(errno));
> + exit(2);
> + }
> +
> + fprintf(fout, "#ifndef _LSM_COUNT_H_\n#define _LSM_COUNT_H_\n\n");
> + fprintf(fout, "\n#define MAX_LSM_COUNT %d\n", GEN_MAX_LSM_COUNT);
> + fprintf(fout, "#endif /* _LSM_COUNT_H_ */\n");
> + exit(0);
> +}
> diff --git a/security/Makefile b/security/Makefile
> index 18121f8f85cd..7a47174831f4 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -3,6 +3,7 @@
> # Makefile for the kernel security code
> #
>
> +gen := include/generated
> obj-$(CONFIG_KEYS) += keys/
>
> # always enable default capabilities
> @@ -27,3 +28,13 @@ obj-$(CONFIG_SECURITY_LANDLOCK) += landlock/
>
> # Object integrity file lists
> obj-$(CONFIG_INTEGRITY) += integrity/
> +
> +$(addprefix $(obj)/,$(obj-y)): $(gen)/lsm_count.h
> +
> +quiet_cmd_lsm_count = GEN ${gen}/lsm_count.h
> + cmd_lsm_count = scripts/security/gen_lsm_count ${gen}/lsm_count.h
> +
> +targets += lsm_count.h
> +
> +${gen}/lsm_count.h: FORCE
> + $(call if_changed,lsm_count)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
2023-01-19 23:10 ` [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls KP Singh
@ 2023-01-20 1:43 ` Casey Schaufler
2023-01-20 2:13 ` KP Singh
0 siblings, 1 reply; 27+ messages in thread
From: Casey Schaufler @ 2023-01-20 1:43 UTC (permalink / raw)
To: KP Singh, linux-security-module, bpf
Cc: ast, daniel, jackmanb, renauld, paul, song, revest, keescook, casey
On 1/19/2023 3:10 PM, KP Singh wrote:
> LSM hooks are currently invoked from a linked list as indirect calls
> which are invoked using retpolines as a mitigation for speculative
> attacks (Branch History / Target injection) and add extra overhead which
> is especially bad in kernel hot paths:
>
> security_file_ioctl:
> 0xffffffff814f0320 <+0>: endbr64
> 0xffffffff814f0324 <+4>: push %rbp
> 0xffffffff814f0325 <+5>: push %r15
> 0xffffffff814f0327 <+7>: push %r14
> 0xffffffff814f0329 <+9>: push %rbx
> 0xffffffff814f032a <+10>: mov %rdx,%rbx
> 0xffffffff814f032d <+13>: mov %esi,%ebp
> 0xffffffff814f032f <+15>: mov %rdi,%r14
> 0xffffffff814f0332 <+18>: mov $0xffffffff834a7030,%r15
> 0xffffffff814f0339 <+25>: mov (%r15),%r15
> 0xffffffff814f033c <+28>: test %r15,%r15
> 0xffffffff814f033f <+31>: je 0xffffffff814f0358 <security_file_ioctl+56>
> 0xffffffff814f0341 <+33>: mov 0x18(%r15),%r11
> 0xffffffff814f0345 <+37>: mov %r14,%rdi
> 0xffffffff814f0348 <+40>: mov %ebp,%esi
> 0xffffffff814f034a <+42>: mov %rbx,%rdx
>
>>>> 0xffffffff814f034d <+45>: call 0xffffffff81f742e0 <__x86_indirect_thunk_array+352>
> Indirect calls that use retpolines leading to overhead, not just due
> to extra instruction but also branch misses.
>
> 0xffffffff814f0352 <+50>: test %eax,%eax
> 0xffffffff814f0354 <+52>: je 0xffffffff814f0339 <security_file_ioctl+25>
> 0xffffffff814f0356 <+54>: jmp 0xffffffff814f035a <security_file_ioctl+58>
> 0xffffffff814f0358 <+56>: xor %eax,%eax
> 0xffffffff814f035a <+58>: pop %rbx
> 0xffffffff814f035b <+59>: pop %r14
> 0xffffffff814f035d <+61>: pop %r15
> 0xffffffff814f035f <+63>: pop %rbp
> 0xffffffff814f0360 <+64>: jmp 0xffffffff81f747c4 <__x86_return_thunk>
>
> The indirect calls are not really needed as one knows the addresses of
> enabled LSM callbacks at boot time and only the order can possibly
> change at boot time with the lsm= kernel command line parameter.
>
> An array of static calls is defined per LSM hook and the static calls
> are updated at boot time once the order has been determined.
>
> A static key guards whether an LSM static call is enabled or not,
> without this static key, for LSM hooks that return an int, the presence
> of the hook that returns a default value can create side-effects which
> has resulted in bugs [1].
>
> With the hook now exposed as a static call, one can see that the
> retpolines are no longer there and the LSM callbacks are invoked
> directly:
>
> security_file_ioctl:
> 0xffffffff814f0dd0 <+0>: endbr64
> 0xffffffff814f0dd4 <+4>: push %rbp
> 0xffffffff814f0dd5 <+5>: push %r14
> 0xffffffff814f0dd7 <+7>: push %rbx
> 0xffffffff814f0dd8 <+8>: mov %rdx,%rbx
> 0xffffffff814f0ddb <+11>: mov %esi,%ebp
> 0xffffffff814f0ddd <+13>: mov %rdi,%r14
>
>>>> 0xffffffff814f0de0 <+16>: jmp 0xffffffff814f0df1 <security_file_ioctl+33>
> Static key enabled for selinux_file_ioctl
>
>>>> 0xffffffff814f0de2 <+18>: jmp 0xffffffff814f0e08 <security_file_ioctl+56>
> Static key enabled for bpf_lsm_file_ioctl. This is something that is
> changed to default to false to avoid the existing side effect issues
> of BPF LSM [1]
>
> 0xffffffff814f0de4 <+20>: xor %eax,%eax
> 0xffffffff814f0de6 <+22>: xchg %ax,%ax
> 0xffffffff814f0de8 <+24>: pop %rbx
> 0xffffffff814f0de9 <+25>: pop %r14
> 0xffffffff814f0deb <+27>: pop %rbp
> 0xffffffff814f0dec <+28>: jmp 0xffffffff81f767c4 <__x86_return_thunk>
> 0xffffffff814f0df1 <+33>: endbr64
> 0xffffffff814f0df5 <+37>: mov %r14,%rdi
> 0xffffffff814f0df8 <+40>: mov %ebp,%esi
> 0xffffffff814f0dfa <+42>: mov %rbx,%rdx
>
>>>> 0xffffffff814f0dfd <+45>: call 0xffffffff814fe820 <selinux_file_ioctl>
> Direct call to SELinux.
>
> 0xffffffff814f0e02 <+50>: test %eax,%eax
> 0xffffffff814f0e04 <+52>: jne 0xffffffff814f0de8 <security_file_ioctl+24>
> 0xffffffff814f0e06 <+54>: jmp 0xffffffff814f0de2 <security_file_ioctl+18>
> 0xffffffff814f0e08 <+56>: endbr64
> 0xffffffff814f0e0c <+60>: mov %r14,%rdi
> 0xffffffff814f0e0f <+63>: mov %ebp,%esi
> 0xffffffff814f0e11 <+65>: mov %rbx,%rdx
>
>>>> 0xffffffff814f0e14 <+68>: call 0xffffffff8123b7d0 <bpf_lsm_file_ioctl>
> Direct call to bpf_lsm_file_ioctl
>
> 0xffffffff814f0e19 <+73>: test %eax,%eax
> 0xffffffff814f0e1b <+75>: jne 0xffffffff814f0de8 <security_file_ioctl+24>
> 0xffffffff814f0e1d <+77>: jmp 0xffffffff814f0de4 <security_file_ioctl+20>
> 0xffffffff814f0e1f <+79>: endbr64
> 0xffffffff814f0e23 <+83>: mov %r14,%rdi
> 0xffffffff814f0e26 <+86>: mov %ebp,%esi
> 0xffffffff814f0e28 <+88>: mov %rbx,%rdx
> 0xffffffff814f0e2b <+91>: pop %rbx
> 0xffffffff814f0e2c <+92>: pop %r14
> 0xffffffff814f0e2e <+94>: pop %rbp
> 0xffffffff814f0e2f <+95>: ret
> 0xffffffff814f0e30 <+96>: int3
> 0xffffffff814f0e31 <+97>: int3
> 0xffffffff814f0e32 <+98>: int3
> 0xffffffff814f0e33 <+99>: int3
>
> There are some hooks that don't use the call_int_hook and
> call_void_hook. These hooks are updated to use a new macro called
> security_for_each_hook
There has got to be a simpler way to do this. Putting all the code
for an extraordinary hook into a macro scares the bedickens out of me.
The call_{int,void}_hook macros work fine for the simple cases, but
that's because they are the simple cases. What would the hooks that use
your new macro look like if you coded them directly?
> where the lsm_callback is directly invoked as an
> indirect call. Currently, there are no performance sensitive hooks that
> use the security_for_each_hook macro. However, if, some performance
> sensitive hooks are discovered, these can be updated to use
> static calls with loop unrolling as well using a custom macro.
Or how about no macro at all? I would really like to see what the code
would look like without this level of macro processing.
>
> [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/
>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
> include/linux/lsm_hooks.h | 83 +++++++++++++--
> security/security.c | 216 ++++++++++++++++++++++++--------------
> 2 files changed, 211 insertions(+), 88 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 0a5ba81f7367..c82d15a4ef50 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -28,6 +28,26 @@
> #include <linux/security.h>
> #include <linux/init.h>
> #include <linux/rculist.h>
> +#include <linux/static_call.h>
> +#include <linux/unroll.h>
> +#include <linux/jump_label.h>
> +
> +/* Include the generated MAX_LSM_COUNT */
> +#include <generated/lsm_count.h>
> +
> +#define SECURITY_HOOK_ENABLED_KEY(HOOK, IDX) security_enabled_key_##HOOK##_##IDX
> +
> +/*
> + * Identifier for the LSM static calls.
> + * HOOK is an LSM hook as defined in linux/lsm_hookdefs.h
> + * IDX is the index of the static call. 0 <= NUM < MAX_LSM_COUNT
> + */
> +#define LSM_STATIC_CALL(HOOK, IDX) lsm_static_call_##HOOK##_##IDX
> +
> +/*
> + * Call the macro M for each LSM hook MAX_LSM_COUNT times.
> + */
> +#define LSM_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__)
>
> /**
> * union security_list_options - Linux Security Module hook function list
> @@ -1657,21 +1677,48 @@ union security_list_options {
> #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
> #include "lsm_hook_defs.h"
> #undef LSM_HOOK
> + void *lsm_callback;
> };
>
> -struct security_hook_heads {
> - #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME;
> - #include "lsm_hook_defs.h"
> +/*
> + * @key: static call key as defined by STATIC_CALL_KEY
> + * @trampoline: static call trampoline as defined by STATIC_CALL_TRAMP
> + * @hl: The security_hook_list as initialized by the owning LSM.
> + * @enabled_key: Enabled when the static call has an LSM hook associated.
> + */
> +struct lsm_static_call {
> + struct static_call_key *key;
> + void *trampoline;
> + struct security_hook_list *hl;
> + struct static_key *enabled_key;
> +};
> +
> +/*
> + * Table of the static calls for each LSM hook.
> + * Once the LSMs are initialized, their callbacks will be copied to these
> + * tables such that the calls are filled backwards (from last to first).
> + * This way, we can jump directly to the first used static call, and execute
> + * all of them after. This essentially makes the entry point
> + * dynamic to adapt the number of static calls to the number of callbacks.
> + */
> +struct lsm_static_calls_table {
> + #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> + struct lsm_static_call NAME[MAX_LSM_COUNT];
> + #include <linux/lsm_hook_defs.h>
> #undef LSM_HOOK
> } __randomize_layout;
>
> /*
> * Security module hook list structure.
> * For use with generic list macros for common operations.
> + *
> + * struct security_hook_list - Contents of a cacheable, mappable object.
> + * @scalls: The beginning of the array of static calls assigned to this hook.
> + * @hook: The callback for the hook.
> + * @lsm: The name of the lsm that owns this hook.
> */
> struct security_hook_list {
> - struct hlist_node list;
> - struct hlist_head *head;
> + struct lsm_static_call *scalls;
> union security_list_options hook;
> const char *lsm;
> } __randomize_layout;
> @@ -1701,10 +1748,12 @@ struct lsm_blob_sizes {
> * care of the common case and reduces the amount of
> * text involved.
> */
> -#define LSM_HOOK_INIT(HEAD, HOOK) \
> - { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
> +#define LSM_HOOK_INIT(NAME, CALLBACK) \
> + { \
> + .scalls = static_calls_table.NAME, \
> + .hook = { .NAME = CALLBACK } \
> + }
>
> -extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> @@ -1756,10 +1805,21 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
> static inline void security_delete_hooks(struct security_hook_list *hooks,
> int count)
> {
> - int i;
> + struct lsm_static_call *scalls;
> + int i, j;
> +
> + for (i = 0; i < count; i++) {
> + scalls = hooks[i].scalls;
> + for (j = 0; j < MAX_LSM_COUNT; j++) {
> + if (scalls[j].hl != &hooks[i])
> + continue;
>
> - for (i = 0; i < count; i++)
> - hlist_del_rcu(&hooks[i].list);
> + static_key_disable(scalls[j].enabled_key);
> + __static_call_update(scalls[j].key,
> + scalls[j].trampoline, NULL);
> + scalls[j].hl = NULL;
> + }
> + }
> }
> #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>
> @@ -1771,5 +1831,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
> #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
>
> extern int lsm_inode_alloc(struct inode *inode);
> +extern struct lsm_static_calls_table static_calls_table __ro_after_init;
>
> #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/security.c b/security/security.c
> index d1571900a8c7..e54d5ba187d1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -29,6 +29,8 @@
> #include <linux/string.h>
> #include <linux/msg.h>
> #include <net/flow.h>
> +#include <linux/static_call.h>
> +#include <linux/jump_label.h>
>
> #define MAX_LSM_EVM_XATTR 2
>
> @@ -74,7 +76,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> };
>
> -struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
>
> static struct kmem_cache *lsm_file_cache;
> @@ -93,6 +94,43 @@ static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
> static __initdata struct lsm_info **ordered_lsms;
> static __initdata struct lsm_info *exclusive;
>
> +/*
> + * Define static calls and static keys for each LSM hook.
> + */
> +
> +#define DEFINE_LSM_STATIC_CALL(NUM, NAME, RET, ...) \
> + DEFINE_STATIC_CALL_NULL(LSM_STATIC_CALL(NAME, NUM), \
> + *((RET(*)(__VA_ARGS__))NULL)); \
> + DEFINE_STATIC_KEY_FALSE(SECURITY_HOOK_ENABLED_KEY(NAME, NUM));
> +
> +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> + LSM_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__)
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
> +#undef DEFINE_LSM_STATIC_CALL
> +
> +/*
> + * Initialise a table of static calls for each LSM hook.
> + * DEFINE_STATIC_CALL_NULL invocation above generates a key (STATIC_CALL_KEY)
> + * and a trampoline (STATIC_CALL_TRAMP) which are used to call
> + * __static_call_update when updating the static call.
> + */
> +struct lsm_static_calls_table static_calls_table __lsm_ro_after_init = {
> +#define INIT_LSM_STATIC_CALL(NUM, NAME) \
> + (struct lsm_static_call) { \
> + .key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)), \
> + .trampoline = &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM)),\
> + .enabled_key = &SECURITY_HOOK_ENABLED_KEY(NAME, NUM).key,\
> + },
> +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> + .NAME = { \
> + LSM_UNROLL(INIT_LSM_STATIC_CALL, NAME) \
> + },
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
> +#undef INIT_LSM_STATIC_CALL
> +};
> +
> static __initdata bool debug;
> #define init_debug(...) \
> do { \
> @@ -153,7 +191,7 @@ static void __init append_ordered_lsm(struct lsm_info *lsm, const char *from)
> if (exists_ordered_lsm(lsm))
> return;
>
> - if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM slots!?\n", from))
> + if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM static calls!?\n", from))
> return;
>
> /* Enable this LSM, if it is not already set. */
> @@ -318,6 +356,24 @@ static void __init ordered_lsm_parse(const char *order, const char *origin)
> kfree(sep);
> }
>
> +static void __init lsm_static_call_init(struct security_hook_list *hl)
> +{
> + struct lsm_static_call *scall = hl->scalls;
> + int i;
> +
> + for (i = 0; i < MAX_LSM_COUNT; i++) {
> + /* Update the first static call that is not used yet */
> + if (!scall->hl) {
> + __static_call_update(scall->key, scall->trampoline, hl->hook.lsm_callback);
> + scall->hl = hl;
> + static_key_enable(scall->enabled_key);
> + return;
> + }
> + scall++;
> + }
> + panic("%s - No static call remaining to add LSM hook.\n", __func__);
> +}
> +
> static void __init lsm_early_cred(struct cred *cred);
> static void __init lsm_early_task(struct task_struct *task);
>
> @@ -395,11 +451,6 @@ int __init early_security_init(void)
> {
> struct lsm_info *lsm;
>
> -#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> - INIT_HLIST_HEAD(&security_hook_heads.NAME);
> -#include "linux/lsm_hook_defs.h"
> -#undef LSM_HOOK
> -
> for (lsm = __start_early_lsm_info; lsm < __end_early_lsm_info; lsm++) {
> if (!lsm->enabled)
> lsm->enabled = &lsm_enabled_true;
> @@ -515,7 +566,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>
> for (i = 0; i < count; i++) {
> hooks[i].lsm = lsm;
> - hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
> + lsm_static_call_init(&hooks[i]);
> }
>
> /*
> @@ -753,28 +804,42 @@ static int lsm_superblock_alloc(struct super_block *sb)
> * call_int_hook:
> * This is a hook that returns a value.
> */
> +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \
> + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \
> + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + }
>
> -#define call_void_hook(FUNC, ...) \
> - do { \
> - struct security_hook_list *P; \
> - \
> - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> - P->hook.FUNC(__VA_ARGS__); \
> +#define call_void_hook(FUNC, ...) \
> + do { \
> + LSM_UNROLL(__CALL_STATIC_VOID, FUNC, __VA_ARGS__) \
> } while (0)
>
> -#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; \
> -})
> +#define __CALL_STATIC_INT(NUM, R, HOOK, ...) \
> + if (static_branch_unlikely(&SECURITY_HOOK_ENABLED_KEY(HOOK, NUM))) { \
> + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + if (R != 0) \
> + goto out; \
> + }
> +
> +#define call_int_hook(FUNC, IRC, ...) \
> + ({ \
> + __label__ out; \
> + int RC = IRC; \
> + do { \
> + LSM_UNROLL(__CALL_STATIC_INT, RC, FUNC, __VA_ARGS__) \
> + \
> + } while (0); \
> +out: \
> + RC; \
> + })
> +
> +#define security_for_each_hook(scall, NAME, expression) \
> + for (scall = static_calls_table.NAME; \
> + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) { \
> + if (!static_key_enabled(scall->enabled_key)) \
> + continue; \
> + (expression); \
> + }
>
> /* Security operations */
>
> @@ -859,7 +924,7 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz)
>
> int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int cap_sys_admin = 1;
> int rc;
>
> @@ -870,13 +935,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> * agree that it should be set it will. If any module
> * thinks it should not be set it won't.
> */
> - hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
> - rc = hp->hook.vm_enough_memory(mm, pages);
> + security_for_each_hook(scall, vm_enough_memory, ({
> + rc = scall->hl->hook.vm_enough_memory(mm, pages);
> if (rc <= 0) {
> cap_sys_admin = 0;
> break;
> }
> - }
> + }));
> return __vm_enough_memory(mm, pages, cap_sys_admin);
> }
>
> @@ -918,18 +983,17 @@ int security_fs_context_dup(struct fs_context *fc, struct fs_context *src_fc)
> int security_fs_context_parse_param(struct fs_context *fc,
> struct fs_parameter *param)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int trc;
> int rc = -ENOPARAM;
>
> - hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param,
> - list) {
> - trc = hp->hook.fs_context_parse_param(fc, param);
> + security_for_each_hook(scall, fs_context_parse_param, ({
> + trc = scall->hl->hook.fs_context_parse_param(fc, param);
> if (trc == 0)
> rc = 0;
> else if (trc != -ENOPARAM)
> return trc;
> - }
> + }));
> return rc;
> }
>
> @@ -1092,18 +1156,19 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
> const char **xattr_name, void **ctx,
> u32 *ctxlen)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int rc;
>
> /*
> * Only one module will provide a security context.
> */
> - hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
> - rc = hp->hook.dentry_init_security(dentry, mode, name,
> + security_for_each_hook(scall, dentry_init_security, ({
> + rc = scall->hl->hook.dentry_init_security(dentry, mode, name,
> xattr_name, ctx, ctxlen);
> if (rc != LSM_RET_DEFAULT(dentry_init_security))
> return rc;
> - }
> + }));
> +
> return LSM_RET_DEFAULT(dentry_init_security);
> }
> EXPORT_SYMBOL(security_dentry_init_security);
> @@ -1502,7 +1567,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
> struct inode *inode, const char *name,
> void **buffer, bool alloc)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int rc;
>
> if (unlikely(IS_PRIVATE(inode)))
> @@ -1510,17 +1575,17 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
> /*
> * Only one module will provide an attribute with a given name.
> */
> - hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
> - rc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
> + security_for_each_hook(scall, inode_getsecurity, ({
> + rc = scall->hl->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
> if (rc != LSM_RET_DEFAULT(inode_getsecurity))
> return rc;
> - }
> + }));
> return LSM_RET_DEFAULT(inode_getsecurity);
> }
>
> int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int rc;
>
> if (unlikely(IS_PRIVATE(inode)))
> @@ -1528,12 +1593,11 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
> /*
> * Only one module will provide an attribute with a given name.
> */
> - hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
> - rc = hp->hook.inode_setsecurity(inode, name, value, size,
> - flags);
> + security_for_each_hook(scall, inode_setsecurity, ({
> + rc = scall->hl->hook.inode_setsecurity(inode, name, value, size, flags);
> if (rc != LSM_RET_DEFAULT(inode_setsecurity))
> return rc;
> - }
> + }));
> return LSM_RET_DEFAULT(inode_setsecurity);
> }
>
> @@ -1558,7 +1622,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
>
> int security_inode_copy_up_xattr(const char *name)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int rc;
>
> /*
> @@ -1566,12 +1630,11 @@ int security_inode_copy_up_xattr(const char *name)
> * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
> * any other error code incase of an error.
> */
> - hlist_for_each_entry(hp,
> - &security_hook_heads.inode_copy_up_xattr, list) {
> - rc = hp->hook.inode_copy_up_xattr(name);
> + security_for_each_hook(scall, inode_copy_up_xattr, ({
> + rc = scall->hl->hook.inode_copy_up_xattr(name);
> if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
> return rc;
> - }
> + }));
>
> return LSM_RET_DEFAULT(inode_copy_up_xattr);
> }
> @@ -1968,16 +2031,16 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> {
> int thisrc;
> int rc = LSM_RET_DEFAULT(task_prctl);
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
>
> - hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
> - thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
> + security_for_each_hook(scall, task_prctl, ({
> + thisrc = scall->hl->hook.task_prctl(option, arg2, arg3, arg4, arg5);
> if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
> rc = thisrc;
> if (thisrc != 0)
> break;
> }
> - }
> + }));
> return rc;
> }
>
> @@ -2142,26 +2205,26 @@ EXPORT_SYMBOL(security_d_instantiate);
> int security_getprocattr(struct task_struct *p, const char *lsm,
> const char *name, char **value)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
>
> - hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> - if (lsm != NULL && strcmp(lsm, hp->lsm))
> + security_for_each_hook(scall, getprocattr, ({
> + if (lsm != NULL && strcmp(lsm, scall->hl->lsm))
> continue;
> - return hp->hook.getprocattr(p, name, value);
> - }
> + return scall->hl->hook.getprocattr(p, name, value);
> + }));
> return LSM_RET_DEFAULT(getprocattr);
> }
>
> int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
>
> - hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> - if (lsm != NULL && strcmp(lsm, hp->lsm))
> + security_for_each_hook(scall, setprocattr, ({
> + if (lsm != NULL && strcmp(lsm, scall->hl->lsm))
> continue;
> - return hp->hook.setprocattr(name, value, size);
> - }
> + return scall->hl->hook.setprocattr(name, value, size);
> + }));
> return LSM_RET_DEFAULT(setprocattr);
> }
>
> @@ -2178,18 +2241,18 @@ EXPORT_SYMBOL(security_ismaclabel);
>
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int rc;
>
> /*
> * Currently, only one LSM can implement secid_to_secctx (i.e this
> * LSM hook is not "stackable").
> */
> - hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> - rc = hp->hook.secid_to_secctx(secid, secdata, seclen);
> + security_for_each_hook(scall, secid_to_secctx, ({
> + rc = scall->hl->hook.secid_to_secctx(secid, secdata, seclen);
> if (rc != LSM_RET_DEFAULT(secid_to_secctx))
> return rc;
> - }
> + }));
>
> return LSM_RET_DEFAULT(secid_to_secctx);
> }
> @@ -2582,7 +2645,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> struct xfrm_policy *xp,
> const struct flowi_common *flic)
> {
> - struct security_hook_list *hp;
> + struct lsm_static_call *scall;
> int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
>
> /*
> @@ -2594,11 +2657,10 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> * For speed optimization, we explicitly break the loop rather than
> * using the macro
> */
> - hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
> - list) {
> - rc = hp->hook.xfrm_state_pol_flow_match(x, xp, flic);
> + security_for_each_hook(scall, xfrm_state_pol_flow_match, ({
> + rc = scall->hl->hook.xfrm_state_pol_flow_match(x, xp, flic);
> break;
> - }
> + }));
> return rc;
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls
2023-01-20 1:43 ` Casey Schaufler
@ 2023-01-20 2:13 ` KP Singh
0 siblings, 0 replies; 27+ messages in thread
From: KP Singh @ 2023-01-20 2:13 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld, paul,
song, revest, keescook
On Fri, Jan 20, 2023 at 2:43 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 1/19/2023 3:10 PM, KP Singh wrote:
> > LSM hooks are currently invoked from a linked list as indirect calls
> > which are invoked using retpolines as a mitigation for speculative
> > attacks (Branch History / Target injection) and add extra overhead which
> > is especially bad in kernel hot paths:
> >
> > security_file_ioctl:
> > 0xffffffff814f0320 <+0>: endbr64
> > 0xffffffff814f0324 <+4>: push %rbp
> > 0xffffffff814f0325 <+5>: push %r15
> > 0xffffffff814f0327 <+7>: push %r14
> > 0xffffffff814f0329 <+9>: push %rbx
> > 0xffffffff814f032a <+10>: mov %rdx,%rbx
> > 0xffffffff814f032d <+13>: mov %esi,%ebp
> > 0xffffffff814f032f <+15>: mov %rdi,%r14
> > 0xffffffff814f0332 <+18>: mov $0xffffffff834a7030,%r15
> > 0xffffffff814f0339 <+25>: mov (%r15),%r15
> > 0xffffffff814f033c <+28>: test %r15,%r15
> > 0xffffffff814f033f <+31>: je 0xffffffff814f0358 <security_file_ioctl+56>
> > 0xffffffff814f0341 <+33>: mov 0x18(%r15),%r11
> > 0xffffffff814f0345 <+37>: mov %r14,%rdi
> > 0xffffffff814f0348 <+40>: mov %ebp,%esi
> > 0xffffffff814f034a <+42>: mov %rbx,%rdx
> >
> >>>> 0xffffffff814f034d <+45>: call 0xffffffff81f742e0 <__x86_indirect_thunk_array+352>
> > Indirect calls that use retpolines leading to overhead, not just due
> > to extra instruction but also branch misses.
> >
> > 0xffffffff814f0352 <+50>: test %eax,%eax
> > 0xffffffff814f0354 <+52>: je 0xffffffff814f0339 <security_file_ioctl+25>
> > 0xffffffff814f0356 <+54>: jmp 0xffffffff814f035a <security_file_ioctl+58>
> > 0xffffffff814f0358 <+56>: xor %eax,%eax
> > 0xffffffff814f035a <+58>: pop %rbx
> > 0xffffffff814f035b <+59>: pop %r14
> > 0xffffffff814f035d <+61>: pop %r15
> > 0xffffffff814f035f <+63>: pop %rbp
> > 0xffffffff814f0360 <+64>: jmp 0xffffffff81f747c4 <__x86_return_thunk>
> >
> > The indirect calls are not really needed as one knows the addresses of
> > enabled LSM callbacks at boot time and only the order can possibly
> > change at boot time with the lsm= kernel command line parameter.
> >
> > An array of static calls is defined per LSM hook and the static calls
> > are updated at boot time once the order has been determined.
> >
> > A static key guards whether an LSM static call is enabled or not,
> > without this static key, for LSM hooks that return an int, the presence
> > of the hook that returns a default value can create side-effects which
> > has resulted in bugs [1].
> >
> > With the hook now exposed as a static call, one can see that the
> > retpolines are no longer there and the LSM callbacks are invoked
> > directly:
> >
> > security_file_ioctl:
> > 0xffffffff814f0dd0 <+0>: endbr64
> > 0xffffffff814f0dd4 <+4>: push %rbp
> > 0xffffffff814f0dd5 <+5>: push %r14
> > 0xffffffff814f0dd7 <+7>: push %rbx
> > 0xffffffff814f0dd8 <+8>: mov %rdx,%rbx
> > 0xffffffff814f0ddb <+11>: mov %esi,%ebp
> > 0xffffffff814f0ddd <+13>: mov %rdi,%r14
> >
> >>>> 0xffffffff814f0de0 <+16>: jmp 0xffffffff814f0df1 <security_file_ioctl+33>
> > Static key enabled for selinux_file_ioctl
> >
> >>>> 0xffffffff814f0de2 <+18>: jmp 0xffffffff814f0e08 <security_file_ioctl+56>
> > Static key enabled for bpf_lsm_file_ioctl. This is something that is
> > changed to default to false to avoid the existing side effect issues
> > of BPF LSM [1]
> >
> > 0xffffffff814f0de4 <+20>: xor %eax,%eax
> > 0xffffffff814f0de6 <+22>: xchg %ax,%ax
> > 0xffffffff814f0de8 <+24>: pop %rbx
> > 0xffffffff814f0de9 <+25>: pop %r14
> > 0xffffffff814f0deb <+27>: pop %rbp
> > 0xffffffff814f0dec <+28>: jmp 0xffffffff81f767c4 <__x86_return_thunk>
> > 0xffffffff814f0df1 <+33>: endbr64
> > 0xffffffff814f0df5 <+37>: mov %r14,%rdi
> > 0xffffffff814f0df8 <+40>: mov %ebp,%esi
> > 0xffffffff814f0dfa <+42>: mov %rbx,%rdx
> >
> >>>> 0xffffffff814f0dfd <+45>: call 0xffffffff814fe820 <selinux_file_ioctl>
> > Direct call to SELinux.
> >
> > 0xffffffff814f0e02 <+50>: test %eax,%eax
> > 0xffffffff814f0e04 <+52>: jne 0xffffffff814f0de8 <security_file_ioctl+24>
> > 0xffffffff814f0e06 <+54>: jmp 0xffffffff814f0de2 <security_file_ioctl+18>
> > 0xffffffff814f0e08 <+56>: endbr64
> > 0xffffffff814f0e0c <+60>: mov %r14,%rdi
> > 0xffffffff814f0e0f <+63>: mov %ebp,%esi
> > 0xffffffff814f0e11 <+65>: mov %rbx,%rdx
> >
> >>>> 0xffffffff814f0e14 <+68>: call 0xffffffff8123b7d0 <bpf_lsm_file_ioctl>
> > Direct call to bpf_lsm_file_ioctl
> >
> > 0xffffffff814f0e19 <+73>: test %eax,%eax
> > 0xffffffff814f0e1b <+75>: jne 0xffffffff814f0de8 <security_file_ioctl+24>
> > 0xffffffff814f0e1d <+77>: jmp 0xffffffff814f0de4 <security_file_ioctl+20>
> > 0xffffffff814f0e1f <+79>: endbr64
> > 0xffffffff814f0e23 <+83>: mov %r14,%rdi
> > 0xffffffff814f0e26 <+86>: mov %ebp,%esi
> > 0xffffffff814f0e28 <+88>: mov %rbx,%rdx
> > 0xffffffff814f0e2b <+91>: pop %rbx
> > 0xffffffff814f0e2c <+92>: pop %r14
> > 0xffffffff814f0e2e <+94>: pop %rbp
> > 0xffffffff814f0e2f <+95>: ret
> > 0xffffffff814f0e30 <+96>: int3
> > 0xffffffff814f0e31 <+97>: int3
> > 0xffffffff814f0e32 <+98>: int3
> > 0xffffffff814f0e33 <+99>: int3
> >
> > There are some hooks that don't use the call_int_hook and
> > call_void_hook. These hooks are updated to use a new macro called
> > security_for_each_hook
>
> There has got to be a simpler way to do this. Putting all the code
> for an extraordinary hook into a macro scares the bedickens out of me.
> The call_{int,void}_hook macros work fine for the simple cases, but
> that's because they are the simple cases. What would the hooks that use
> your new macro look like if you coded them directly?
It cannot be coded directly, the number of possible LSMs is determined
at compile time using CONFIG_* macros, so directly coding the slots
would become even more cumbersome and error prone (e.g. missing out
stuff when a new hook is added etc) and updating the static calls at
boot time without a compile time enforced macro would make the logic
even more complex.
Also note, what I dumped here is assembly at runtime, this looks very
different at compile time: Here's the compile time assembly:
ecurity_file_ioctl:
0xffffffff814f0e90 <+0>: endbr64
0xffffffff814f0e94 <+4>: push %rbp
0xffffffff814f0e95 <+5>: push %r14
0xffffffff814f0e97 <+7>: push %rbx
0xffffffff814f0e98 <+8>: mov %rdx,%rbx
0xffffffff814f0e9b <+11>: mov %esi,%ebp
0xffffffff814f0e9d <+13>: mov %rdi,%r14
0xffffffff814f0ea0 <+16>: xchg %ax,%ax
0xffffffff814f0ea2 <+18>: xchg %ax,%ax
0xffffffff814f0ea4 <+20>: xor %eax,%eax
0xffffffff814f0ea6 <+22>: xchg %ax,%ax
0xffffffff814f0ea8 <+24>: pop %rbx
0xffffffff814f0ea9 <+25>: pop %r14
0xffffffff814f0eab <+27>: pop %rbp
0xffffffff814f0eac <+28>: jmp 0xffffffff81f767c4 <__x86_return_thunk>
0xffffffff814f0eb1 <+33>: endbr64
0xffffffff814f0eb5 <+37>: mov %r14,%rdi
0xffffffff814f0eb8 <+40>: mov %ebp,%esi
0xffffffff814f0eba <+42>: mov %rbx,%rdx
0xffffffff814f0ebd <+45>: call 0xffffffff81f784f0
<__SCT__lsm_static_call_file_ioctl_0>
0xffffffff814f0ec2 <+50>: test %eax,%eax
0xffffffff814f0ec4 <+52>: jne 0xffffffff814f0ea8 <security_file_ioctl+24>
0xffffffff814f0ec6 <+54>: jmp 0xffffffff814f0ea2 <security_file_ioctl+18>
0xffffffff814f0ec8 <+56>: endbr64
0xffffffff814f0ecc <+60>: mov %r14,%rdi
0xffffffff814f0ecf <+63>: mov %ebp,%esi
0xffffffff814f0ed1 <+65>: mov %rbx,%rdx
0xffffffff814f0ed4 <+68>: call 0xffffffff81f784f8
<__SCT__lsm_static_call_file_ioctl_1>
0xffffffff814f0ed9 <+73>: test %eax,%eax
0xffffffff814f0edb <+75>: jne 0xffffffff814f0ea8 <security_file_ioctl+24>
0xffffffff814f0edd <+77>: jmp 0xffffffff814f0ea4 <security_file_ioctl+20>
0xffffffff814f0edf <+79>: endbr64
0xffffffff814f0ee3 <+83>: mov %r14,%rdi
0xffffffff814f0ee6 <+86>: mov %ebp,%esi
0xffffffff814f0ee8 <+88>: mov %rbx,%rdx
0xffffffff814f0eeb <+91>: pop %rbx
0xffffffff814f0eec <+92>: pop %r14
0xffffffff814f0eee <+94>: pop %rbp
0xffffffff814f0eef <+95>: jmp 0xffffffff81f78500
<__SCT__lsm_static_call_file_ioctl_2>
>
> > where the lsm_callback is directly invoked as an
> > indirect call. Currently, there are no performance sensitive hooks that
> > use the security_for_each_hook macro. However, if, some performance
> > sensitive hooks are discovered, these can be updated to use
> > static calls with loop unrolling as well using a custom macro.
>
> Or how about no macro at all? I would really like to see what the code
> would look like without this level of macro processing.
>
One can inline the static slot generation logic, but the code would
become quite complex and tricky to write without macros. Open for
suggestions.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs
2023-01-20 1:32 ` Casey Schaufler
@ 2023-01-20 2:15 ` KP Singh
2023-01-20 18:35 ` Kui-Feng Lee
0 siblings, 1 reply; 27+ messages in thread
From: KP Singh @ 2023-01-20 2:15 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld, paul,
song, revest, keescook
On Fri, Jan 20, 2023 at 2:32 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 1/19/2023 3:10 PM, KP Singh wrote:
> > The header defines a MAX_LSM_COUNT constant which is used in a
> > subsequent patch to generate the static calls for each LSM hook which
> > are named using preprocessor token pasting. Since token pasting does not
> > work with arithmetic expressions, generate a simple lsm_count.h header
> > which represents the subset of LSMs that can be enabled on a given
> > kernel based on the config.
> >
> > While one can generate static calls for all the possible LSMs that the
> > kernel has, this is actually wasteful as most kernels only enable a
> > handful of LSMs.
>
> Why "generate" anything? Why not include your GEN_MAX_LSM_COUNT macro
> in security.h and be done with it? I've proposed doing just that in the
> stacking patch set for some time. This seems to be much more complicated
> than it needs to be.
The answer is in the commit description, the count is used in token
pasting and you cannot have arithmetic in when you generate tokens in
preprocessor macros.
you cannot generate bprm_check_security_call_1 + 1 + 1 this does not
get resolved by preprocessor.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-01-20 1:13 ` [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls Casey Schaufler
@ 2023-01-20 2:17 ` KP Singh
2023-01-20 18:40 ` Casey Schaufler
0 siblings, 1 reply; 27+ messages in thread
From: KP Singh @ 2023-01-20 2:17 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld, paul,
song, revest, keescook
On Fri, Jan 20, 2023 at 2:13 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 1/19/2023 3:10 PM, KP Singh wrote:
> > # Background
> >
> > LSM hooks (callbacks) are currently invoked as indirect function calls. These
> > callbacks are registered into a linked list at boot time as the order of the
> > LSMs can be configured on the kernel command line with the "lsm=" command line
> > parameter.
> >
> > Indirect function calls have a high overhead due to retpoline mitigation for
> > various speculative execution attacks.
> >
> > Retpolines remain relevant even with newer generation CPUs as recently
> > discovered speculative attacks, like Spectre BHB need Retpolines to mitigate
> > against branch history injection and still need to be used in combination with
> > newer mitigation features like eIBRS.
> >
> > This overhead is especially significant for the "bpf" LSM which allows the user
> > to implement LSM functionality with eBPF program. In order to facilitate this
> > the "bpf" LSM provides a default callback for all LSM hooks. When enabled,
> > the "bpf" LSM incurs an unnecessary / avoidable indirect call. This is
> > especially bad in OS hot paths (e.g. in the networking stack).
> > This overhead prevents the adoption of bpf LSM on performance critical
> > systems, and also, in general, slows down all LSMs.
> >
> > Since we know the address of the enabled LSM callbacks at compile time and only
> > the order is determined at boot time,
>
> No quite true. A system with Smack and AppArmor compiled in will only
> be allowed to use one or the other.
>
> > the LSM framework can allocate static
> > calls for each of the possible LSM callbacks and these calls can be updated once
> > the order is determined at boot.
>
> True if you also provide for the single "major" LSM restriction.
>
> > This series is a respin of the RFC proposed by Paul Renauld (renauld@google.com)
> > and Brendan Jackman (jackmanb@google.com) [1]
> >
> > # Performance improvement
> >
> > With this patch-set some syscalls with lots of LSM hooks in their path
> > benefitted at an average of ~3%. Here are the results of the relevant Unixbench
> > system benchmarks with BPF LSM and a major LSM (in this case apparmor) enabled
> > with and without the series.
> >
> > Benchmark Delta(%): (+ is better)
> > ===============================================================================
> > Execl Throughput +2.9015
> > File Write 1024 bufsize 2000 maxblocks +5.4196
> > Pipe Throughput +7.7434
> > Pipe-based Context Switching +3.5118
> > Process Creation +0.3552
> > Shell Scripts (1 concurrent) +1.7106
> > System Call Overhead +3.0067
> > System Benchmarks Index Score (Partial Only): +3.1809
>
> How about socket creation and packet delivery impact? You'll need to
> use either SELinux or Smack to get those numbers.
I think the goal here is to show that hot paths are beneficial, and
the results are pretty clear from this. I have an even more detailed
analysis in https://kpsingh.ch/lsm-perf as to what happens when the
static calls are enabled v/s not enabled. I don't have the socket
numbers, but I expect this to be very similar to pipes. Is there a
particular Unixbench test you want me to run?
>
> > In the best case, some syscalls like eventfd_create benefitted to about ~10%.
> > The full analysis can be viewed at https://kpsingh.ch/lsm-perf
> >
> > [1] https://lore.kernel.org/linux-security-module/20200820164753.3256899-1-jackmanb@chromium.org/
> >
> > KP Singh (4):
> > kernel: Add helper macros for loop unrolling
> > security: Generate a header with the count of enabled LSMs
> > security: Replace indirect LSM hook calls with static calls
> > bpf: Only enable BPF LSM hooks when an LSM program is attached
> >
> > include/linux/bpf.h | 1 +
> > include/linux/bpf_lsm.h | 1 +
> > include/linux/lsm_hooks.h | 94 +++++++++++--
> > include/linux/unroll.h | 35 +++++
> > kernel/bpf/trampoline.c | 29 ++++-
> > scripts/Makefile | 1 +
> > scripts/security/.gitignore | 1 +
> > scripts/security/Makefile | 4 +
> > scripts/security/gen_lsm_count.c | 57 ++++++++
> > security/Makefile | 11 ++
> > security/bpf/hooks.c | 26 +++-
> > security/security.c | 217 ++++++++++++++++++++-----------
> > 12 files changed, 386 insertions(+), 91 deletions(-)
> > create mode 100644 include/linux/unroll.h
> > create mode 100644 scripts/security/.gitignore
> > create mode 100644 scripts/security/Makefile
> > create mode 100644 scripts/security/gen_lsm_count.c
> >
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs
2023-01-20 2:15 ` KP Singh
@ 2023-01-20 18:35 ` Kui-Feng Lee
2023-01-20 19:40 ` Kees Cook
0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-01-20 18:35 UTC (permalink / raw)
To: KP Singh, Casey Schaufler
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld, paul,
song, revest, keescook
The following idea should work with the use case here.
#define COUNT_8(x, y...) 8
#define COUNT_7(x, y...) 7
#define COUNT_6(x, y...) 6
#define COUNT_5(x, y...) 5
#define COUNT_4(x, y...) 4
#define COUNT_3(x, y...) 3
#define COUNT_2(x, y...) 2
#define COUNT_1(x, y...) 1
#define COUNT_0(x, y...) 0
#define COUNT1_8(x, y...) COUNT ## x ## _9(y)
#define COUNT1_7(x, y...) COUNT ## x ## _8(y)
#define COUNT1_6(x, y...) COUNT ## x ## _7(y)
#define COUNT1_5(x, y...) COUNT ## x ## _6(y)
#define COUNT1_4(x, y...) COUNT ## x ## _5(y)
#define COUNT1_3(x, y...) COUNT ## x ## _4(y)
#define COUNT1_2(x, y...) COUNT ## x ## _3(y)
#define COUNT1_1(x, y...) COUNT ## x ## _2(y)
#define COUNT1_0(x, y...) COUNT ## x ## _1(y)
#define COUNT(x, y...) COUNT ## x ## _0(y)
#define COUNT_EXPAND(x...) COUNT(x)
#if IS_ENABLED(CONFIG_SECURITY_SELINUX)
#define SELINUX_ENABLE 1,
#else
#define SELINUX_ENABLE
#endif
#if IS_ENABLED(CONFIG_SECURITY_XXXX)
#define XXX_ENABLE 1,
#else
#define XXX_ENABLE
#endif
....
#define MAX_LSM_COUNT COUNT_EXPAND(SELINUX_ENABLE XXX_ENABLE ......)
On 1/19/23 18:15, KP Singh wrote:
> On Fri, Jan 20, 2023 at 2:32 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/19/2023 3:10 PM, KP Singh wrote:
>>> The header defines a MAX_LSM_COUNT constant which is used in a
>>> subsequent patch to generate the static calls for each LSM hook which
>>> are named using preprocessor token pasting. Since token pasting does not
>>> work with arithmetic expressions, generate a simple lsm_count.h header
>>> which represents the subset of LSMs that can be enabled on a given
>>> kernel based on the config.
>>>
>>> While one can generate static calls for all the possible LSMs that the
>>> kernel has, this is actually wasteful as most kernels only enable a
>>> handful of LSMs.
>> Why "generate" anything? Why not include your GEN_MAX_LSM_COUNT macro
>> in security.h and be done with it? I've proposed doing just that in the
>> stacking patch set for some time. This seems to be much more complicated
>> than it needs to be.
> The answer is in the commit description, the count is used in token
> pasting and you cannot have arithmetic in when you generate tokens in
> preprocessor macros.
>
> you cannot generate bprm_check_security_call_1 + 1 + 1 this does not
> get resolved by preprocessor.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-01-20 2:17 ` KP Singh
@ 2023-01-20 18:40 ` Casey Schaufler
0 siblings, 0 replies; 27+ messages in thread
From: Casey Schaufler @ 2023-01-20 18:40 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld, paul,
song, revest, keescook, casey
On 1/19/2023 6:17 PM, KP Singh wrote:
> On Fri, Jan 20, 2023 at 2:13 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/19/2023 3:10 PM, KP Singh wrote:
>>> # Background
>>>
>>> LSM hooks (callbacks) are currently invoked as indirect function calls. These
>>> callbacks are registered into a linked list at boot time as the order of the
>>> LSMs can be configured on the kernel command line with the "lsm=" command line
>>> parameter.
>>>
>>> Indirect function calls have a high overhead due to retpoline mitigation for
>>> various speculative execution attacks.
>>>
>>> Retpolines remain relevant even with newer generation CPUs as recently
>>> discovered speculative attacks, like Spectre BHB need Retpolines to mitigate
>>> against branch history injection and still need to be used in combination with
>>> newer mitigation features like eIBRS.
>>>
>>> This overhead is especially significant for the "bpf" LSM which allows the user
>>> to implement LSM functionality with eBPF program. In order to facilitate this
>>> the "bpf" LSM provides a default callback for all LSM hooks. When enabled,
>>> the "bpf" LSM incurs an unnecessary / avoidable indirect call. This is
>>> especially bad in OS hot paths (e.g. in the networking stack).
>>> This overhead prevents the adoption of bpf LSM on performance critical
>>> systems, and also, in general, slows down all LSMs.
>>>
>>> Since we know the address of the enabled LSM callbacks at compile time and only
>>> the order is determined at boot time,
>> No quite true. A system with Smack and AppArmor compiled in will only
>> be allowed to use one or the other.
>>
>>> the LSM framework can allocate static
>>> calls for each of the possible LSM callbacks and these calls can be updated once
>>> the order is determined at boot.
>> True if you also provide for the single "major" LSM restriction.
>>
>>> This series is a respin of the RFC proposed by Paul Renauld (renauld@google.com)
>>> and Brendan Jackman (jackmanb@google.com) [1]
>>>
>>> # Performance improvement
>>>
>>> With this patch-set some syscalls with lots of LSM hooks in their path
>>> benefitted at an average of ~3%. Here are the results of the relevant Unixbench
>>> system benchmarks with BPF LSM and a major LSM (in this case apparmor) enabled
>>> with and without the series.
>>>
>>> Benchmark Delta(%): (+ is better)
>>> ===============================================================================
>>> Execl Throughput +2.9015
>>> File Write 1024 bufsize 2000 maxblocks +5.4196
>>> Pipe Throughput +7.7434
>>> Pipe-based Context Switching +3.5118
>>> Process Creation +0.3552
>>> Shell Scripts (1 concurrent) +1.7106
>>> System Call Overhead +3.0067
>>> System Benchmarks Index Score (Partial Only): +3.1809
>> How about socket creation and packet delivery impact? You'll need to
>> use either SELinux or Smack to get those numbers.
> I think the goal here is to show that hot paths are beneficial, and
> the results are pretty clear from this. I have an even more detailed
> analysis in https://kpsingh.ch/lsm-perf as to what happens when the
> static calls are enabled v/s not enabled. I don't have the socket
> numbers, but I expect this to be very similar to pipes. Is there a
> particular Unixbench test you want me to run?
It isn't wise to assume that the paths used in IP code behave the same
way as any others. Unixbench doesn't look like a great tool for doing
this measurement. I would look at iperf or even some of the low level
tests in lmbench.
>
>>> In the best case, some syscalls like eventfd_create benefitted to about ~10%.
>>> The full analysis can be viewed at https://kpsingh.ch/lsm-perf
>>>
>>> [1] https://lore.kernel.org/linux-security-module/20200820164753.3256899-1-jackmanb@chromium.org/
>>>
>>> KP Singh (4):
>>> kernel: Add helper macros for loop unrolling
>>> security: Generate a header with the count of enabled LSMs
>>> security: Replace indirect LSM hook calls with static calls
>>> bpf: Only enable BPF LSM hooks when an LSM program is attached
>>>
>>> include/linux/bpf.h | 1 +
>>> include/linux/bpf_lsm.h | 1 +
>>> include/linux/lsm_hooks.h | 94 +++++++++++--
>>> include/linux/unroll.h | 35 +++++
>>> kernel/bpf/trampoline.c | 29 ++++-
>>> scripts/Makefile | 1 +
>>> scripts/security/.gitignore | 1 +
>>> scripts/security/Makefile | 4 +
>>> scripts/security/gen_lsm_count.c | 57 ++++++++
>>> security/Makefile | 11 ++
>>> security/bpf/hooks.c | 26 +++-
>>> security/security.c | 217 ++++++++++++++++++++-----------
>>> 12 files changed, 386 insertions(+), 91 deletions(-)
>>> create mode 100644 include/linux/unroll.h
>>> create mode 100644 scripts/security/.gitignore
>>> create mode 100644 scripts/security/Makefile
>>> create mode 100644 scripts/security/gen_lsm_count.c
>>>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs
2023-01-20 18:35 ` Kui-Feng Lee
@ 2023-01-20 19:40 ` Kees Cook
0 siblings, 0 replies; 27+ messages in thread
From: Kees Cook @ 2023-01-20 19:40 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: KP Singh, Casey Schaufler, linux-security-module, bpf, ast,
daniel, jackmanb, renauld, paul, song, revest
On Fri, Jan 20, 2023 at 10:35:02AM -0800, Kui-Feng Lee wrote:
> The following idea should work with the use case here.
>
> #define COUNT_8(x, y...) 8
> #define COUNT_7(x, y...) 7
> #define COUNT_6(x, y...) 6
> #define COUNT_5(x, y...) 5
> #define COUNT_4(x, y...) 4
> #define COUNT_3(x, y...) 3
> #define COUNT_2(x, y...) 2
> #define COUNT_1(x, y...) 1
> #define COUNT_0(x, y...) 0
> #define COUNT1_8(x, y...) COUNT ## x ## _9(y)
> #define COUNT1_7(x, y...) COUNT ## x ## _8(y)
> #define COUNT1_6(x, y...) COUNT ## x ## _7(y)
> #define COUNT1_5(x, y...) COUNT ## x ## _6(y)
> #define COUNT1_4(x, y...) COUNT ## x ## _5(y)
> #define COUNT1_3(x, y...) COUNT ## x ## _4(y)
> #define COUNT1_2(x, y...) COUNT ## x ## _3(y)
> #define COUNT1_1(x, y...) COUNT ## x ## _2(y)
> #define COUNT1_0(x, y...) COUNT ## x ## _1(y)
> #define COUNT(x, y...) COUNT ## x ## _0(y)
>
> #define COUNT_EXPAND(x...) COUNT(x)
>
>
> #if IS_ENABLED(CONFIG_SECURITY_SELINUX)
> #define SELINUX_ENABLE 1,
> #else
> #define SELINUX_ENABLE
> #endif
> #if IS_ENABLED(CONFIG_SECURITY_XXXX)
> #define XXX_ENABLE 1,
> #else
> #define XXX_ENABLE
> #endif
> ....
>
> #define MAX_LSM_COUNT COUNT_EXPAND(SELINUX_ENABLE XXX_ENABLE ......)
Oh, I love it! :) Yup, that should do it nicely.
--
Kees Cook
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
` (4 preceding siblings ...)
2023-01-20 1:13 ` [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls Casey Schaufler
@ 2023-01-27 19:22 ` Song Liu
2023-01-27 20:16 ` Paul Moore
6 siblings, 0 replies; 27+ messages in thread
From: Song Liu @ 2023-01-27 19:22 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld, paul,
casey, revest, keescook
Hi KP,
Thanks for working on this!
On Thu, Jan 19, 2023 at 3:10 PM KP Singh <kpsingh@kernel.org> wrote:
>
[...]
>
> # Performance improvement
>
> With this patch-set some syscalls with lots of LSM hooks in their path
> benefitted at an average of ~3%. Here are the results of the relevant Unixbench
> system benchmarks with BPF LSM and a major LSM (in this case apparmor) enabled
> with and without the series.
>
> Benchmark Delta(%): (+ is better)
> ===============================================================================
> Execl Throughput +2.9015
> File Write 1024 bufsize 2000 maxblocks +5.4196
> Pipe Throughput +7.7434
> Pipe-based Context Switching +3.5118
> Process Creation +0.3552
> Shell Scripts (1 concurrent) +1.7106
> System Call Overhead +3.0067
> System Benchmarks Index Score (Partial Only): +3.1809
>
> In the best case, some syscalls like eventfd_create benefitted to about ~10%.
The results look very promising. These optimizations will make it easier
for our users (at Meta) to adopt LSM solutions.
Thanks again!
Song
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
` (5 preceding siblings ...)
2023-01-27 19:22 ` Song Liu
@ 2023-01-27 20:16 ` Paul Moore
2023-02-09 16:56 ` Kees Cook
6 siblings, 1 reply; 27+ messages in thread
From: Paul Moore @ 2023-01-27 20:16 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, daniel, jackmanb, renauld,
casey, song, revest, keescook
On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
>
> # Background
>
> LSM hooks (callbacks) are currently invoked as indirect function calls. These
> callbacks are registered into a linked list at boot time as the order of the
> LSMs can be configured on the kernel command line with the "lsm=" command line
> parameter.
Thanks for sending this KP. I had hoped to make a proper pass through
this patchset this week but I ended up getting stuck trying to wrap my
head around some network segmentation offload issues and didn't quite
make it here. Rest assured it is still in my review queue.
However, I did manage to take a quick look at the patches and one of
the first things that jumped out at me is it *looks* like this
patchset is attempting two things: fix a problem where one LSM could
trample another (especially problematic with the BPF LSM due to its
nature), and reduce the overhead of making LSM calls. I realize that
in this patchset the fix and the optimization are heavily
intermingled, but I wonder what it would take to develop a standalone
fix using the existing indirect call approach? I'm guessing that is
going to potentially be a pretty significant patch, but if we could
add a little standardization to the LSM hooks without adding too much
in the way of code complexity or execution overhead I think that might
be a win independent of any changes to how we call the hooks.
Of course this could be crazy too, but I'm the guy who has to ask
these questions :)
--
paul-moore.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-01-27 20:16 ` Paul Moore
@ 2023-02-09 16:56 ` Kees Cook
2023-02-10 20:03 ` Paul Moore
0 siblings, 1 reply; 27+ messages in thread
From: Kees Cook @ 2023-02-09 16:56 UTC (permalink / raw)
To: Paul Moore
Cc: KP Singh, linux-security-module, bpf, ast, daniel, jackmanb,
renauld, casey, song, revest
On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
> On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > # Background
> >
> > LSM hooks (callbacks) are currently invoked as indirect function calls. These
> > callbacks are registered into a linked list at boot time as the order of the
> > LSMs can be configured on the kernel command line with the "lsm=" command line
> > parameter.
>
> Thanks for sending this KP. I had hoped to make a proper pass through
> this patchset this week but I ended up getting stuck trying to wrap my
> head around some network segmentation offload issues and didn't quite
> make it here. Rest assured it is still in my review queue.
>
> However, I did manage to take a quick look at the patches and one of
> the first things that jumped out at me is it *looks* like this
> patchset is attempting two things: fix a problem where one LSM could
> trample another (especially problematic with the BPF LSM due to its
> nature), and reduce the overhead of making LSM calls. I realize that
> in this patchset the fix and the optimization are heavily
> intermingled, but I wonder what it would take to develop a standalone
> fix using the existing indirect call approach? I'm guessing that is
> going to potentially be a pretty significant patch, but if we could
> add a little standardization to the LSM hooks without adding too much
> in the way of code complexity or execution overhead I think that might
> be a win independent of any changes to how we call the hooks.
>
> Of course this could be crazy too, but I'm the guy who has to ask
> these questions :)
Hm, I am expecting this patch series to _not_ change any semantics of
the LSM "stack". I would agree: nothing should change in this series, as
it should be strictly a mechanical change from "iterate a list of
indirect calls" to "make a series of direct calls". Perhaps I missed
a logical change?
--
Kees Cook
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-02-09 16:56 ` Kees Cook
@ 2023-02-10 20:03 ` Paul Moore
2023-02-11 2:32 ` Casey Schaufler
2023-06-13 22:02 ` KP Singh
0 siblings, 2 replies; 27+ messages in thread
From: Paul Moore @ 2023-02-10 20:03 UTC (permalink / raw)
To: Kees Cook
Cc: KP Singh, linux-security-module, bpf, ast, daniel, jackmanb,
renauld, casey, song, revest
On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
> > On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
> > >
> > > # Background
> > >
> > > LSM hooks (callbacks) are currently invoked as indirect function calls. These
> > > callbacks are registered into a linked list at boot time as the order of the
> > > LSMs can be configured on the kernel command line with the "lsm=" command line
> > > parameter.
> >
> > Thanks for sending this KP. I had hoped to make a proper pass through
> > this patchset this week but I ended up getting stuck trying to wrap my
> > head around some network segmentation offload issues and didn't quite
> > make it here. Rest assured it is still in my review queue.
> >
> > However, I did manage to take a quick look at the patches and one of
> > the first things that jumped out at me is it *looks* like this
> > patchset is attempting two things: fix a problem where one LSM could
> > trample another (especially problematic with the BPF LSM due to its
> > nature), and reduce the overhead of making LSM calls. I realize that
> > in this patchset the fix and the optimization are heavily
> > intermingled, but I wonder what it would take to develop a standalone
> > fix using the existing indirect call approach? I'm guessing that is
> > going to potentially be a pretty significant patch, but if we could
> > add a little standardization to the LSM hooks without adding too much
> > in the way of code complexity or execution overhead I think that might
> > be a win independent of any changes to how we call the hooks.
> >
> > Of course this could be crazy too, but I'm the guy who has to ask
> > these questions :)
>
> Hm, I am expecting this patch series to _not_ change any semantics of
> the LSM "stack". I would agree: nothing should change in this series, as
> it should be strictly a mechanical change from "iterate a list of
> indirect calls" to "make a series of direct calls". Perhaps I missed
> a logical change?
I might be missing something too, but I'm thinking of patch 4/4 in
this series that starts with this sentence:
"BPF LSM hooks have side-effects (even when a default value is
returned), as some hooks end up behaving differently due to
the very presence of the hook."
Ignoring the static call changes for a moment, I'm curious what it
would look like to have a better mechanism for handling things like
this. What would it look like if we expanded the individual LSM error
reporting back to the LSM layer to have a bit more information, e.g.
"this LSM erred, but it is safe to continue evaluating other LSMs" and
"this LSM erred, and it was too severe to continue evaluating other
LSMs"? Similarly, would we want to expand the hook registration to
have more info, e.g. "run this hook even when other LSMs have failed"
and "if other LSMs have failed, do not run this hook"?
I realize that loading a BPF LSM is a privileged operation so we've
largely mitigated the risk there, but with stacking on it's way to
being more full featured, and IMA slowly working its way to proper LSM
status, it seems to me like having a richer, and proper way to handle
individual LSM failures would be a good thing. I feel like patch 4/4
definitely hints at this, but I could be mistaken.
--
paul-moore.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-02-10 20:03 ` Paul Moore
@ 2023-02-11 2:32 ` Casey Schaufler
2023-02-12 22:00 ` Paul Moore
2023-06-13 22:02 ` KP Singh
1 sibling, 1 reply; 27+ messages in thread
From: Casey Schaufler @ 2023-02-11 2:32 UTC (permalink / raw)
To: Paul Moore, Kees Cook
Cc: KP Singh, linux-security-module, bpf, ast, daniel, jackmanb,
renauld, song, revest, casey
On 2/10/2023 12:03 PM, Paul Moore wrote:
> On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
>>> On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
>>>> # Background
>>>>
>>>> LSM hooks (callbacks) are currently invoked as indirect function calls. These
>>>> callbacks are registered into a linked list at boot time as the order of the
>>>> LSMs can be configured on the kernel command line with the "lsm=" command line
>>>> parameter.
>>> Thanks for sending this KP. I had hoped to make a proper pass through
>>> this patchset this week but I ended up getting stuck trying to wrap my
>>> head around some network segmentation offload issues and didn't quite
>>> make it here. Rest assured it is still in my review queue.
>>>
>>> However, I did manage to take a quick look at the patches and one of
>>> the first things that jumped out at me is it *looks* like this
>>> patchset is attempting two things: fix a problem where one LSM could
>>> trample another (especially problematic with the BPF LSM due to its
>>> nature), and reduce the overhead of making LSM calls. I realize that
>>> in this patchset the fix and the optimization are heavily
>>> intermingled, but I wonder what it would take to develop a standalone
>>> fix using the existing indirect call approach? I'm guessing that is
>>> going to potentially be a pretty significant patch, but if we could
>>> add a little standardization to the LSM hooks without adding too much
>>> in the way of code complexity or execution overhead I think that might
>>> be a win independent of any changes to how we call the hooks.
>>>
>>> Of course this could be crazy too, but I'm the guy who has to ask
>>> these questions :)
>> Hm, I am expecting this patch series to _not_ change any semantics of
>> the LSM "stack". I would agree: nothing should change in this series, as
>> it should be strictly a mechanical change from "iterate a list of
>> indirect calls" to "make a series of direct calls". Perhaps I missed
>> a logical change?
> I might be missing something too, but I'm thinking of patch 4/4 in
> this series that starts with this sentence:
>
> "BPF LSM hooks have side-effects (even when a default value is
> returned), as some hooks end up behaving differently due to
> the very presence of the hook."
My understanding of the current "agreement" is that we keep BPF
hooks at the end for this very reason.
> Ignoring the static call changes for a moment, I'm curious what it
> would look like to have a better mechanism for handling things like
> this. What would it look like if we expanded the individual LSM error
> reporting back to the LSM layer to have a bit more information, e.g.
> "this LSM erred, but it is safe to continue evaluating other LSMs" and
> "this LSM erred, and it was too severe to continue evaluating other
> LSMs"? Similarly, would we want to expand the hook registration to
> have more info, e.g. "run this hook even when other LSMs have failed"
> and "if other LSMs have failed, do not run this hook"?
I really don't want another LSM to have sway over Smack enforcement.
I would hate to see, for example, an LSM decide that because it has
initialized an inode no other LSM should be allowed to, even in an
error situation. There are really only two options Call all the hooks
every time and either succeed on all or report the most important
error. Or, "bail on fail", and acknowledge that following hooks may
not be called. Really, does "I failed, but it's not that important"
make sense as a return value?
If the return isn't important, make it a void hook.
> I realize that loading a BPF LSM is a privileged operation so we've
> largely mitigated the risk there, but with stacking on it's way to
> being more full featured, and IMA slowly working its way to proper LSM
> status, it seems to me like having a richer, and proper way to handle
> individual LSM failures would be a good thing. I feel like patch 4/4
> definitely hints at this, but I could be mistaken.
We have bigger issues with BPF. There's nothing to prevent BPF from
implementing a secid_to_secctx() hook and making a system with SELinux
go cattywampus. BPF is stacked as if it isn't a "major" LSM, while
allowing it to do "major" LSM things. One reason we need full stacking
is to address this.
My $0.02. That and $1.98 will get you a beer on Tuesdays, 3-5pm.
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-02-11 2:32 ` Casey Schaufler
@ 2023-02-12 22:00 ` Paul Moore
2023-02-13 18:04 ` Casey Schaufler
2023-02-13 18:29 ` Casey Schaufler
0 siblings, 2 replies; 27+ messages in thread
From: Paul Moore @ 2023-02-12 22:00 UTC (permalink / raw)
To: Casey Schaufler
Cc: Kees Cook, KP Singh, linux-security-module, bpf, ast, daniel,
jackmanb, renauld, song, revest
On Fri, Feb 10, 2023 at 9:32 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/10/2023 12:03 PM, Paul Moore wrote:
> > On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
> >> On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
> >>> On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
> >>>> # Background
> >>>>
> >>>> LSM hooks (callbacks) are currently invoked as indirect function calls. These
> >>>> callbacks are registered into a linked list at boot time as the order of the
> >>>> LSMs can be configured on the kernel command line with the "lsm=" command line
> >>>> parameter.
> >>> Thanks for sending this KP. I had hoped to make a proper pass through
> >>> this patchset this week but I ended up getting stuck trying to wrap my
> >>> head around some network segmentation offload issues and didn't quite
> >>> make it here. Rest assured it is still in my review queue.
> >>>
> >>> However, I did manage to take a quick look at the patches and one of
> >>> the first things that jumped out at me is it *looks* like this
> >>> patchset is attempting two things: fix a problem where one LSM could
> >>> trample another (especially problematic with the BPF LSM due to its
> >>> nature), and reduce the overhead of making LSM calls. I realize that
> >>> in this patchset the fix and the optimization are heavily
> >>> intermingled, but I wonder what it would take to develop a standalone
> >>> fix using the existing indirect call approach? I'm guessing that is
> >>> going to potentially be a pretty significant patch, but if we could
> >>> add a little standardization to the LSM hooks without adding too much
> >>> in the way of code complexity or execution overhead I think that might
> >>> be a win independent of any changes to how we call the hooks.
> >>>
> >>> Of course this could be crazy too, but I'm the guy who has to ask
> >>> these questions :)
> >> Hm, I am expecting this patch series to _not_ change any semantics of
> >> the LSM "stack". I would agree: nothing should change in this series, as
> >> it should be strictly a mechanical change from "iterate a list of
> >> indirect calls" to "make a series of direct calls". Perhaps I missed
> >> a logical change?
> > I might be missing something too, but I'm thinking of patch 4/4 in
> > this series that starts with this sentence:
> >
> > "BPF LSM hooks have side-effects (even when a default value is
> > returned), as some hooks end up behaving differently due to
> > the very presence of the hook."
>
> My understanding of the current "agreement" is that we keep BPF
> hooks at the end for this very reason.
It would be nice to not have these conventions. I get that it's the
only knob we have at the moment to tweak, but I would hope that we
could do better in the future.
> > Ignoring the static call changes for a moment, I'm curious what it
> > would look like to have a better mechanism for handling things like
> > this. What would it look like if we expanded the individual LSM error
> > reporting back to the LSM layer to have a bit more information, e.g.
> > "this LSM erred, but it is safe to continue evaluating other LSMs" and
> > "this LSM erred, and it was too severe to continue evaluating other
> > LSMs"? Similarly, would we want to expand the hook registration to
> > have more info, e.g. "run this hook even when other LSMs have failed"
> > and "if other LSMs have failed, do not run this hook"?
>
> I really don't want another LSM to have sway over Smack enforcement.
I think we can all agree that the one LSM should not have the ability
to affect the operation of another, especially when it would cause the
violation of a different LSM's security policy.
> I would hate to see, for example, an LSM decide that because it has
> initialized an inode no other LSM should be allowed to, even in an
> error situation. There are really only two options Call all the hooks
> every time and either succeed on all or report the most important
> error. Or, "bail on fail", and acknowledge that following hooks may
> not be called. Really, does "I failed, but it's not that important"
> make sense as a return value?
Of the two things I tossed out, richer return values and richer hook
registration, perhaps it's really only the latter, richer hook
registration that is important here. It would allow a LSM to indicate
to the LSM hook layer how the individual hook implementation should be
called: always, or only if previously called implementations have not
failed. I believe that should eliminate any worry of a BPF LSM, or
any LSM for that matter, from impacting the security policy of
another. However, I will admit that I haven't spent the necessary
amount of time chasing down all the hooks to verify if that is 100%
correct.
> > I realize that loading a BPF LSM is a privileged operation so we've
> > largely mitigated the risk there, but with stacking on it's way to
> > being more full featured, and IMA slowly working its way to proper LSM
> > status, it seems to me like having a richer, and proper way to handle
> > individual LSM failures would be a good thing. I feel like patch 4/4
> > definitely hints at this, but I could be mistaken.
>
> We have bigger issues with BPF. There's nothing to prevent BPF from
> implementing a secid_to_secctx() hook and making a system with SELinux
> go cattywampus. BPF is stacked as if it isn't a "major" LSM, while
> allowing it to do "major" LSM things. One reason we need full stacking
> is to address this.
That's a different issue, and one of the reasons why I suggested
taking an all-or-nothing approach to stacking many years ago, but ...
well, you know how that worked out. I promise to not keep saying "I
told you so" if you promise to not keep bringing up LSM stacking as
the answer to all that ails you ;)
--
paul-moore.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-02-12 22:00 ` Paul Moore
@ 2023-02-13 18:04 ` Casey Schaufler
2023-02-13 18:29 ` Casey Schaufler
1 sibling, 0 replies; 27+ messages in thread
From: Casey Schaufler @ 2023-02-13 18:04 UTC (permalink / raw)
To: Paul Moore
Cc: Kees Cook, KP Singh, linux-security-module, bpf, ast, daniel,
jackmanb, renauld, song, revest
On 2/12/2023 2:00 PM, Paul Moore wrote:
> On Fri, Feb 10, 2023 at 9:32 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/10/2023 12:03 PM, Paul Moore wrote:
>>> On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
>>>> On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
>>>>> On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
>>>>>> # Background
>>>>>>
>>>>>> LSM hooks (callbacks) are currently invoked as indirect function calls. These
>>>>>> callbacks are registered into a linked list at boot time as the order of the
>>>>>> LSMs can be configured on the kernel command line with the "lsm=" command line
>>>>>> parameter.
>>>>> Thanks for sending this KP. I had hoped to make a proper pass through
>>>>> this patchset this week but I ended up getting stuck trying to wrap my
>>>>> head around some network segmentation offload issues and didn't quite
>>>>> make it here. Rest assured it is still in my review queue.
>>>>>
>>>>> However, I did manage to take a quick look at the patches and one of
>>>>> the first things that jumped out at me is it *looks* like this
>>>>> patchset is attempting two things: fix a problem where one LSM could
>>>>> trample another (especially problematic with the BPF LSM due to its
>>>>> nature), and reduce the overhead of making LSM calls. I realize that
>>>>> in this patchset the fix and the optimization are heavily
>>>>> intermingled, but I wonder what it would take to develop a standalone
>>>>> fix using the existing indirect call approach? I'm guessing that is
>>>>> going to potentially be a pretty significant patch, but if we could
>>>>> add a little standardization to the LSM hooks without adding too much
>>>>> in the way of code complexity or execution overhead I think that might
>>>>> be a win independent of any changes to how we call the hooks.
>>>>>
>>>>> Of course this could be crazy too, but I'm the guy who has to ask
>>>>> these questions :)
>>>> Hm, I am expecting this patch series to _not_ change any semantics of
>>>> the LSM "stack". I would agree: nothing should change in this series, as
>>>> it should be strictly a mechanical change from "iterate a list of
>>>> indirect calls" to "make a series of direct calls". Perhaps I missed
>>>> a logical change?
>>> I might be missing something too, but I'm thinking of patch 4/4 in
>>> this series that starts with this sentence:
>>>
>>> "BPF LSM hooks have side-effects (even when a default value is
>>> returned), as some hooks end up behaving differently due to
>>> the very presence of the hook."
>> My understanding of the current "agreement" is that we keep BPF
>> hooks at the end for this very reason.
> It would be nice to not have these conventions. I get that it's the
> only knob we have at the moment to tweak, but I would hope that we
> could do better in the future.
Agreed. I don't care much for it.
>
> Ignoring the static call changes for a moment, I'm curious what it
> would look like to have a better mechanism for handling things like
> this. What would it look like if we expanded the individual LSM error
> reporting back to the LSM layer to have a bit more information, e.g.
> "this LSM erred, but it is safe to continue evaluating other LSMs" and
> "this LSM erred, and it was too severe to continue evaluating other
> LSMs"? Similarly, would we want to expand the hook registration to
> have more info, e.g. "run this hook even when other LSMs have failed"
> and "if other LSMs have failed, do not run this hook"?
>> I really don't want another LSM to have sway over Smack enforcement.
> I think we can all agree that the one LSM should not have the ability
> to affect the operation of another, especially when it would cause the
> violation of a different LSM's security policy.
>
>> I would hate to see, for example, an LSM decide that because it has
>> initialized an inode no other LSM should be allowed to, even in an
>> error situation. There are really only two options Call all the hooks
>> every time and either succeed on all or report the most important
>> error. Or, "bail on fail", and acknowledge that following hooks may
>> not be called. Really, does "I failed, but it's not that important"
>> make sense as a return value?
> Of the two things I tossed out, richer return values and richer hook
> registration, perhaps it's really only the latter, richer hook
> registration that is important here. It would allow a LSM to indicate
> to the LSM hook layer how the individual hook implementation should be
> called: always, or only if previously called implementations have not
> failed. I believe that should eliminate any worry of a BPF LSM, or
> any LSM for that matter, from impacting the security policy of
> another. However, I will admit that I haven't spent the necessary
> amount of time chasing down all the hooks to verify if that is 100%
> correct.
>
>>> I realize that loading a BPF LSM is a privileged operation so we've
>>> largely mitigated the risk there, but with stacking on it's way to
>>> being more full featured, and IMA slowly working its way to proper LSM
>>> status, it seems to me like having a richer, and proper way to handle
>>> individual LSM failures would be a good thing. I feel like patch 4/4
>>> definitely hints at this, but I could be mistaken.
>> We have bigger issues with BPF. There's nothing to prevent BPF from
>> implementing a secid_to_secctx() hook and making a system with SELinux
>> go cattywampus. BPF is stacked as if it isn't a "major" LSM, while
>> allowing it to do "major" LSM things. One reason we need full stacking
>> is to address this.
> That's a different issue, and one of the reasons why I suggested
> taking an all-or-nothing approach to stacking many years ago, but ...
> well, you know how that worked out. I promise to not keep saying "I
> told you so" if you promise to not keep bringing up LSM stacking as
> the answer to all that ails you ;)
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-02-12 22:00 ` Paul Moore
2023-02-13 18:04 ` Casey Schaufler
@ 2023-02-13 18:29 ` Casey Schaufler
1 sibling, 0 replies; 27+ messages in thread
From: Casey Schaufler @ 2023-02-13 18:29 UTC (permalink / raw)
To: Paul Moore
Cc: Kees Cook, KP Singh, linux-security-module, bpf, ast, daniel,
jackmanb, renauld, song, revest, casey
On 2/12/2023 2:00 PM, Paul Moore wrote:
> On Fri, Feb 10, 2023 at 9:32 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/10/2023 12:03 PM, Paul Moore wrote:
>>> On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
>>>> On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
>>>>> On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
>>>>>> # Background
>>>>>>
>>>>>> LSM hooks (callbacks) are currently invoked as indirect function calls. These
>>>>>> callbacks are registered into a linked list at boot time as the order of the
>>>>>> LSMs can be configured on the kernel command line with the "lsm=" command line
>>>>>> parameter.
>>>>> Thanks for sending this KP. I had hoped to make a proper pass through
>>>>> this patchset this week but I ended up getting stuck trying to wrap my
>>>>> head around some network segmentation offload issues and didn't quite
>>>>> make it here. Rest assured it is still in my review queue.
>>>>>
>>>>> However, I did manage to take a quick look at the patches and one of
>>>>> the first things that jumped out at me is it *looks* like this
>>>>> patchset is attempting two things: fix a problem where one LSM could
>>>>> trample another (especially problematic with the BPF LSM due to its
>>>>> nature), and reduce the overhead of making LSM calls. I realize that
>>>>> in this patchset the fix and the optimization are heavily
>>>>> intermingled, but I wonder what it would take to develop a standalone
>>>>> fix using the existing indirect call approach? I'm guessing that is
>>>>> going to potentially be a pretty significant patch, but if we could
>>>>> add a little standardization to the LSM hooks without adding too much
>>>>> in the way of code complexity or execution overhead I think that might
>>>>> be a win independent of any changes to how we call the hooks.
>>>>>
>>>>> Of course this could be crazy too, but I'm the guy who has to ask
>>>>> these questions :)
>>>> Hm, I am expecting this patch series to _not_ change any semantics of
>>>> the LSM "stack". I would agree: nothing should change in this series, as
>>>> it should be strictly a mechanical change from "iterate a list of
>>>> indirect calls" to "make a series of direct calls". Perhaps I missed
>>>> a logical change?
>>> I might be missing something too, but I'm thinking of patch 4/4 in
>>> this series that starts with this sentence:
>>>
>>> "BPF LSM hooks have side-effects (even when a default value is
>>> returned), as some hooks end up behaving differently due to
>>> the very presence of the hook."
>> My understanding of the current "agreement" is that we keep BPF
>> hooks at the end for this very reason.
> It would be nice to not have these conventions. I get that it's the
> only knob we have at the moment to tweak, but I would hope that we
> could do better in the future.
Agreed. I don't much care for what we have now. The enthusiasm for BPF
overwhelmed the caution that would normally protect the LSM infrastructure.
>>> Ignoring the static call changes for a moment, I'm curious what it
>>> would look like to have a better mechanism for handling things like
>>> this. What would it look like if we expanded the individual LSM error
>>> reporting back to the LSM layer to have a bit more information, e.g.
>>> "this LSM erred, but it is safe to continue evaluating other LSMs" and
>>> "this LSM erred, and it was too severe to continue evaluating other
>>> LSMs"? Similarly, would we want to expand the hook registration to
>>> have more info, e.g. "run this hook even when other LSMs have failed"
>>> and "if other LSMs have failed, do not run this hook"?
>> I really don't want another LSM to have sway over Smack enforcement.
> I think we can all agree that the one LSM should not have the ability
> to affect the operation of another, especially when it would cause the
> violation of a different LSM's security policy.
>
>> I would hate to see, for example, an LSM decide that because it has
>> initialized an inode no other LSM should be allowed to, even in an
>> error situation. There are really only two options Call all the hooks
>> every time and either succeed on all or report the most important
>> error. Or, "bail on fail", and acknowledge that following hooks may
>> not be called. Really, does "I failed, but it's not that important"
>> make sense as a return value?
> Of the two things I tossed out, richer return values and richer hook
> registration, perhaps it's really only the latter, richer hook
> registration that is important here. It would allow a LSM to indicate
> to the LSM hook layer how the individual hook implementation should be
> called: always, or only if previously called implementations have not
> failed. I believe that should eliminate any worry of a BPF LSM, or
> any LSM for that matter, from impacting the security policy of
> another. However, I will admit that I haven't spent the necessary
> amount of time chasing down all the hooks to verify if that is 100%
> correct.
Even this approach leads to the problem of which error to return in
the presence of multiple, unrelated failures. My earliest efforts on
stacking used a "call all" approach, with success returned if all
modules approved. I abandoned this because it's impossible to identify
for all cases which error is best to report. In some cases -EACCES is
less significant than -EPERM, but if you have both, what might the
application care about most?
>>> I realize that loading a BPF LSM is a privileged operation so we've
>>> largely mitigated the risk there, but with stacking on it's way to
>>> being more full featured, and IMA slowly working its way to proper LSM
>>> status, it seems to me like having a richer, and proper way to handle
>>> individual LSM failures would be a good thing. I feel like patch 4/4
>>> definitely hints at this, but I could be mistaken.
>> We have bigger issues with BPF. There's nothing to prevent BPF from
>> implementing a secid_to_secctx() hook and making a system with SELinux
>> go cattywampus. BPF is stacked as if it isn't a "major" LSM, while
>> allowing it to do "major" LSM things. One reason we need full stacking
>> is to address this.
> That's a different issue, and one of the reasons why I suggested
> taking an all-or-nothing approach to stacking many years ago, but ...
> well, you know how that worked out.
I'm still shooting for getting "all".
> I promise to not keep saying "I
> told you so" if you promise to not keep bringing up LSM stacking as
> the answer to all that ails you ;)
Sigh.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-02-10 20:03 ` Paul Moore
2023-02-11 2:32 ` Casey Schaufler
@ 2023-06-13 22:02 ` KP Singh
2023-06-20 23:40 ` Paul Moore
1 sibling, 1 reply; 27+ messages in thread
From: KP Singh @ 2023-06-13 22:02 UTC (permalink / raw)
To: Paul Moore
Cc: Kees Cook, linux-security-module, bpf, ast, daniel, jackmanb,
renauld, casey, song, revest
On Fri, Feb 10, 2023 at 9:03 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
> > > On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
> > > >
> > > > # Background
> > > >
> > > > LSM hooks (callbacks) are currently invoked as indirect function calls. These
> > > > callbacks are registered into a linked list at boot time as the order of the
> > > > LSMs can be configured on the kernel command line with the "lsm=" command line
> > > > parameter.
> > >
> > > Thanks for sending this KP. I had hoped to make a proper pass through
> > > this patchset this week but I ended up getting stuck trying to wrap my
> > > head around some network segmentation offload issues and didn't quite
> > > make it here. Rest assured it is still in my review queue.
> > >
> > > However, I did manage to take a quick look at the patches and one of
> > > the first things that jumped out at me is it *looks* like this
> > > patchset is attempting two things: fix a problem where one LSM could
> > > trample another (especially problematic with the BPF LSM due to its
> > > nature), and reduce the overhead of making LSM calls. I realize that
> > > in this patchset the fix and the optimization are heavily
> > > intermingled, but I wonder what it would take to develop a standalone
> > > fix using the existing indirect call approach? I'm guessing that is
> > > going to potentially be a pretty significant patch, but if we could
> > > add a little standardization to the LSM hooks without adding too much
> > > in the way of code complexity or execution overhead I think that might
> > > be a win independent of any changes to how we call the hooks.
> > >
> > > Of course this could be crazy too, but I'm the guy who has to ask
> > > these questions :)
> >
> > Hm, I am expecting this patch series to _not_ change any semantics of
> > the LSM "stack". I would agree: nothing should change in this series, as
> > it should be strictly a mechanical change from "iterate a list of
> > indirect calls" to "make a series of direct calls". Perhaps I missed
> > a logical change?
>
There is no logical change in the 2nd patch that introduces static
calls. There is however a logical change in the fourth patch (as you
noticed) which allows some hooks to register themselves as disabled by
default. This reduces the buggy side effects we have currently with
BPF LSM.
> I might be missing something too, but I'm thinking of patch 4/4 in
> this series that starts with this sentence:
Patch 4/4 is the semantic change but we do need that for both a
performant BPF LSM and eliminating the side effects.
>
> "BPF LSM hooks have side-effects (even when a default value is
> returned), as some hooks end up behaving differently due to
> the very presence of the hook."
>
> Ignoring the static call changes for a moment, I'm curious what it
> would look like to have a better mechanism for handling things like
> this. What would it look like if we expanded the individual LSM error
> reporting back to the LSM layer to have a bit more information, e.g.
> "this LSM erred, but it is safe to continue evaluating other LSMs" and
> "this LSM erred, and it was too severe to continue evaluating other
I tried proposing an idea in
https://patchwork.kernel.org/project/netdevbpf/patch/20220609234601.2026362-1-kpsingh@kernel.org/
as an LSM_HOOK_NO_EFFECT but that did not seemed to have stuck.
> LSMs"? Similarly, would we want to expand the hook registration to
> have more info, e.g. "run this hook even when other LSMs have failed"
> and "if other LSMs have failed, do not run this hook"?
>
> I realize that loading a BPF LSM is a privileged operation so we've
> largely mitigated the risk there, but with stacking on it's way to
> being more full featured, and IMA slowly working its way to proper LSM
> status, it seems to me like having a richer, and proper way to handle
> individual LSM failures would be a good thing. I feel like patch 4/4
> definitely hints at this, but I could be mistaken.
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-06-13 22:02 ` KP Singh
@ 2023-06-20 23:40 ` Paul Moore
2023-07-26 11:07 ` Paolo Abeni
0 siblings, 1 reply; 27+ messages in thread
From: Paul Moore @ 2023-06-20 23:40 UTC (permalink / raw)
To: KP Singh
Cc: Kees Cook, linux-security-module, bpf, ast, daniel, jackmanb,
renauld, casey, song, revest
On Tue, Jun 13, 2023 at 6:03 PM KP Singh <kpsingh@kernel.org> wrote:
> On Fri, Feb 10, 2023 at 9:03 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Feb 9, 2023 at 11:56 AM Kees Cook <keescook@chromium.org> wrote:
> > > On Fri, Jan 27, 2023 at 03:16:38PM -0500, Paul Moore wrote:
> > > > On Thu, Jan 19, 2023 at 6:10 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > >
> > > > > # Background
> > > > >
> > > > > LSM hooks (callbacks) are currently invoked as indirect function calls. These
> > > > > callbacks are registered into a linked list at boot time as the order of the
> > > > > LSMs can be configured on the kernel command line with the "lsm=" command line
> > > > > parameter.
> > > >
> > > > Thanks for sending this KP. I had hoped to make a proper pass through
> > > > this patchset this week but I ended up getting stuck trying to wrap my
> > > > head around some network segmentation offload issues and didn't quite
> > > > make it here. Rest assured it is still in my review queue.
> > > >
> > > > However, I did manage to take a quick look at the patches and one of
> > > > the first things that jumped out at me is it *looks* like this
> > > > patchset is attempting two things: fix a problem where one LSM could
> > > > trample another (especially problematic with the BPF LSM due to its
> > > > nature), and reduce the overhead of making LSM calls. I realize that
> > > > in this patchset the fix and the optimization are heavily
> > > > intermingled, but I wonder what it would take to develop a standalone
> > > > fix using the existing indirect call approach? I'm guessing that is
> > > > going to potentially be a pretty significant patch, but if we could
> > > > add a little standardization to the LSM hooks without adding too much
> > > > in the way of code complexity or execution overhead I think that might
> > > > be a win independent of any changes to how we call the hooks.
> > > >
> > > > Of course this could be crazy too, but I'm the guy who has to ask
> > > > these questions :)
> > >
> > > Hm, I am expecting this patch series to _not_ change any semantics of
> > > the LSM "stack". I would agree: nothing should change in this series, as
> > > it should be strictly a mechanical change from "iterate a list of
> > > indirect calls" to "make a series of direct calls". Perhaps I missed
> > > a logical change?
>
> There is no logical change in the 2nd patch that introduces static
> calls. There is however a logical change in the fourth patch (as you
> noticed) which allows some hooks to register themselves as disabled by
> default. This reduces the buggy side effects we have currently with
> BPF LSM.
>
> > I might be missing something too, but I'm thinking of patch 4/4 in
> > this series that starts with this sentence:
>
> Patch 4/4 is the semantic change but we do need that for both a
> performant BPF LSM and eliminating the side effects.
>
> > "BPF LSM hooks have side-effects (even when a default value is
> > returned), as some hooks end up behaving differently due to
> > the very presence of the hook."
> >
> > Ignoring the static call changes for a moment, I'm curious what it
> > would look like to have a better mechanism for handling things like
> > this. What would it look like if we expanded the individual LSM error
> > reporting back to the LSM layer to have a bit more information, e.g.
> > "this LSM erred, but it is safe to continue evaluating other LSMs" and
> > "this LSM erred, and it was too severe to continue evaluating other
>
> I tried proposing an idea in
> https://patchwork.kernel.org/project/netdevbpf/patch/20220609234601.2026362-1-kpsingh@kernel.org/
> as an LSM_HOOK_NO_EFFECT but that did not seemed to have stuck.
It looks like this was posted about a month before I became
responsible for the LSM layer as a whole, and likely was lost (at
least on the LSM side of things) as a result.
I would much rather see a standalone fix to address the unintended LSM
interactions, then the static call performance improvements in a
separate patchset.
> > LSMs"? Similarly, would we want to expand the hook registration to
> > have more info, e.g. "run this hook even when other LSMs have failed"
> > and "if other LSMs have failed, do not run this hook"?
> >
> > I realize that loading a BPF LSM is a privileged operation so we've
> > largely mitigated the risk there, but with stacking on it's way to
> > being more full featured, and IMA slowly working its way to proper LSM
> > status, it seems to me like having a richer, and proper way to handle
> > individual LSM failures would be a good thing. I feel like patch 4/4
> > definitely hints at this, but I could be mistaken.
--
paul-moore.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-06-20 23:40 ` Paul Moore
@ 2023-07-26 11:07 ` Paolo Abeni
2023-09-16 0:57 ` KP Singh
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Abeni @ 2023-07-26 11:07 UTC (permalink / raw)
To: Paul Moore, KP Singh
Cc: Kees Cook, linux-security-module, bpf, ast, daniel, jackmanb,
renauld, casey, song, revest
Hi all,
On Tue, 2023-06-20 at 19:40 -0400, Paul Moore wrote:
> On Tue, Jun 13, 2023 at 6:03 PM KP Singh <kpsingh@kernel.org> wrote:
> > I tried proposing an idea in
> > https://patchwork.kernel.org/project/netdevbpf/patch/20220609234601.2026362-1-kpsingh@kernel.org/
> > as an LSM_HOOK_NO_EFFECT but that did not seemed to have stuck.
>
> It looks like this was posted about a month before I became
> responsible for the LSM layer as a whole, and likely was lost (at
> least on the LSM side of things) as a result.
>
> I would much rather see a standalone fix to address the unintended LSM
> interactions, then the static call performance improvements in a
> separate patchset.
Please allow me to revive this old thread. I learned about this effort
only recently and I'm interested into it.
Looking at patch 4/4 from this series, it *think* it's doable to
extract it from the series and make it work standalone. If so, would
that approach be ok from a LSM point of view?
One thing that I personally don't understand in said patch is how the
'__ro_after_init' annotation for the bpf_lsm_hooks fits the run-time
'default_state' changes?!?
Cheers,
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-07-26 11:07 ` Paolo Abeni
@ 2023-09-16 0:57 ` KP Singh
2023-09-16 8:06 ` Paolo Abeni
0 siblings, 1 reply; 27+ messages in thread
From: KP Singh @ 2023-09-16 0:57 UTC (permalink / raw)
To: Paolo Abeni
Cc: Paul Moore, Kees Cook, linux-security-module, bpf, ast, daniel,
jackmanb, renauld, casey, song, revest
On Wed, Jul 26, 2023 at 1:07 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi all,
>
> On Tue, 2023-06-20 at 19:40 -0400, Paul Moore wrote:
> > On Tue, Jun 13, 2023 at 6:03 PM KP Singh <kpsingh@kernel.org> wrote:
> > > I tried proposing an idea in
> > > https://patchwork.kernel.org/project/netdevbpf/patch/20220609234601.2026362-1-kpsingh@kernel.org/
> > > as an LSM_HOOK_NO_EFFECT but that did not seemed to have stuck.
> >
> > It looks like this was posted about a month before I became
> > responsible for the LSM layer as a whole, and likely was lost (at
> > least on the LSM side of things) as a result.
> >
> > I would much rather see a standalone fix to address the unintended LSM
> > interactions, then the static call performance improvements in a
> > separate patchset.
>
> Please allow me to revive this old thread. I learned about this effort
> only recently and I'm interested into it.
>
> Looking at patch 4/4 from this series, it *think* it's doable to
> extract it from the series and make it work standalone. If so, would
> that approach be ok from a LSM point of view?
I will rev up the series again. I think it's worth fixing both issues
(performance and this side-effect). There are more users who have been
asking me for performance improvements for LSMs
>
> One thing that I personally don't understand in said patch is how the
> '__ro_after_init' annotation for the bpf_lsm_hooks fits the run-time
> 'default_state' changes?!?
>
> Cheers,
>
> Paolo
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls
2023-09-16 0:57 ` KP Singh
@ 2023-09-16 8:06 ` Paolo Abeni
0 siblings, 0 replies; 27+ messages in thread
From: Paolo Abeni @ 2023-09-16 8:06 UTC (permalink / raw)
To: KP Singh
Cc: Paul Moore, Kees Cook, linux-security-module, bpf, ast, daniel,
jackmanb, renauld, casey, song, revest
Hi,
I'm sorry for the duplicate, I did a quick reply via the gmail UI and
that unintentionally inserted html. Retrying with a real email client.
On Sat, 2023-09-16 at 02:57 +0200, KP Singh wrote:
> On Wed, Jul 26, 2023 at 1:07 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > Looking at patch 4/4 from this series, it *think* it's doable to
> > extract it from the series and make it work standalone. If so, would
> > that approach be ok from a LSM point of view?
>
> I will rev up the series again. I think it's worth fixing both issues
> (performance and this side-effect). There are more users who have been
> asking me for performance improvements for LSMs
FTR, I'm also very interested in the performance side of the thing.
My understanding is that Paul asks the 'side-effect' issue being
addressed before/separately.
To that extent I shared a slightly different approach here:
https://lore.kernel.org/linux-security-module/cover.1691082677.git.pabeni@redhat.com/
with the hope it could be 'cleaner' and allow building the indirect
call avoidance on top.
I would appreciate it if you could take a look there, too!
Thanks,
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-09-16 8:06 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-19 23:10 [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 1/4] kernel: Add helper macros for loop unrolling KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 2/4] security: Generate a header with the count of enabled LSMs KP Singh
2023-01-20 1:32 ` Casey Schaufler
2023-01-20 2:15 ` KP Singh
2023-01-20 18:35 ` Kui-Feng Lee
2023-01-20 19:40 ` Kees Cook
2023-01-19 23:10 ` [PATCH bpf-next 3/4] security: Replace indirect LSM hook calls with static calls KP Singh
2023-01-20 1:43 ` Casey Schaufler
2023-01-20 2:13 ` KP Singh
2023-01-19 23:10 ` [PATCH bpf-next 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
2023-01-20 1:13 ` [PATCH bpf-next 0/4] Reduce overhead of LSMs with static calls Casey Schaufler
2023-01-20 2:17 ` KP Singh
2023-01-20 18:40 ` Casey Schaufler
2023-01-27 19:22 ` Song Liu
2023-01-27 20:16 ` Paul Moore
2023-02-09 16:56 ` Kees Cook
2023-02-10 20:03 ` Paul Moore
2023-02-11 2:32 ` Casey Schaufler
2023-02-12 22:00 ` Paul Moore
2023-02-13 18:04 ` Casey Schaufler
2023-02-13 18:29 ` Casey Schaufler
2023-06-13 22:02 ` KP Singh
2023-06-20 23:40 ` Paul Moore
2023-07-26 11:07 ` Paolo Abeni
2023-09-16 0:57 ` KP Singh
2023-09-16 8:06 ` Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.