* [PATCH v13 0/5] Reduce overhead of LSMs with static calls
@ 2024-06-29 8:43 KP Singh
2024-06-29 8:43 ` [PATCH v13 1/5] kernel: Add helper macros for loop unrolling KP Singh
` (4 more replies)
0 siblings, 5 replies; 34+ messages in thread
From: KP Singh @ 2024-06-29 8:43 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song
# 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% and I/O and Pipe based system calls benefitting
the most.
Here are the results of the relevant Unixbench system benchmarks with BPF LSM
and SELinux enabled with default policies enabled with and without these
patches.
Benchmark Delta(%): (+ is better)
===============================================================================
Execl Throughput +1.9356
File Write 1024 bufsize 2000 maxblocks +6.5953
Pipe Throughput +9.5499
Pipe-based Context Switching +3.0209
Process Creation +2.3246
Shell Scripts (1 concurrent) +1.4975
System Call Overhead +2.7815
System Benchmarks Index Score (Partial Only): +3.4859
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/
# BPF LSM Side effects
Patch 4 of the series also addresses the issues with the side effects of the
default value return values of the BPF LSM callbacks and also removes the
overheads associated with them making it deployable at hyperscale.
# v12 to v13
* Fixed the GCC note when struct randomization is enabled
* Added missing link to BPF LSM side effects discussion in the commit message
# v11 to v12
* Casey's feedback on readability of patch 4
* Tetsuo's catch on the bug in Patch 5
* Changed LSM_HOOK_TOGGLEABLE to LSM_HOOK_RUNTIME, the bikeshed is blue now.
* Also, as security_toggle_hook relies on iterating over a
flat lsm_static_calls_table, I added the __packed attribute.
# v10 to v11
* bpf_lsm_toggle_hook to security_toggle_hook with LSM_HOOK_TOGGLEABLE
limiting the hooks as the ones that can be toggled.
# v9 to v10
* Addressed Paul's comments for Patch 3. I did not remove the acks from this one
as changes were minor.
* Moved BPF LSM specific hook toggling logic bpf_lsm_toggle_hook to s
security_toggle_hook as a generic API. I removed the Ack's from this patch
as it's worth another look.
* Refactored the non-standard hooks to use static calls.
# v8 to v9
Paul, I removed the 5th patch about CONFIG_SECURITY_HOOK_LIKELY and went through
all the feedback. I believe it all should be addressed now.
But, please let me know if I missed anything.
The patches are based on https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git
(next branch as of 2024-02-07) and resolved a bunch of conflicts.
I also added Andrii's series ack to indidividual patches.
# v7 to v8
* Addressed Andrii's feedback
* Rebased (this seems to have removed the syscall changes). v7 has the required
conflict resolution incase the conflicts need to be resolved again.
# v6 -> v7
* Rebased with latest LSM id changes merged
NOTE: The warning shown by the kernel test bot is spurious, there is no flex array
and it seems to come from an older tool chain.
https://lore.kernel.org/bpf/202310111711.wLbijitj-lkp@intel.com/
# v5 -> v6
* Fix a bug in BPF LSM hook toggle logic.
# v4 -> v5
* Rebase to linux-next/master
* Fixed the case where MAX_LSM_COUNT comes to zero when just CONFIG_SECURITY
is compiled in without any other LSM enabled as reported here:
https://lore.kernel.org/bpf/202309271206.d7fb60f9-oliver.sang@intel.com
# v3 -> v4
* Refactor LSM count macros to use COUNT_ARGS
* Change CONFIG_SECURITY_HOOK_LIKELY likely's default value to be based on
the LSM enabled and have it depend on CONFIG_EXPERT. There are a lot of subtle
options behind CONFIG_EXPERT and this should, hopefully alleviate concerns
about yet another knob.
* __randomize_layout for struct lsm_static_call and, in addition to the cover
letter add performance numbers to 3rd patch and some minor commit message
updates.
* Rebase to linux-next.
# v2 -> v3
* Fixed a build issue on archs which don't have static calls and enable
CONFIG_SECURITY.
* Updated the LSM_COUNT macros based on Andrii's suggestions.
* Changed the security_ prefix to lsm_prefix based on Casey's suggestion.
* Inlined static_branch_maybe into lsm_for_each_hook on Kees' feedback.
# v1 -> v2 (based on linux-next, next-20230614)
* Incorporated suggestions from Kees
* Changed the way MAX_LSMs are counted from a binary based generator to a clever header.
* Add CONFIG_SECURITY_HOOK_LIKELY to configure the likelihood of LSM hooks.
KP Singh (5):
kernel: Add helper macros for loop unrolling
security: Count the LSMs enabled at compile time
security: Replace indirect LSM hook calls with static calls
security: Update non standard hooks to use static calls
bpf: Only enable BPF LSM hooks when an LSM program is attached
include/linux/args.h | 6 +-
include/linux/lsm_count.h | 128 ++++++++++++
include/linux/lsm_hooks.h | 100 +++++++++-
include/linux/unroll.h | 36 ++++
kernel/bpf/trampoline.c | 40 +++-
security/bpf/hooks.c | 2 +-
security/security.c | 404 ++++++++++++++++++++++++++------------
7 files changed, 569 insertions(+), 147 deletions(-)
create mode 100644 include/linux/lsm_count.h
create mode 100644 include/linux/unroll.h
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v13 1/5] kernel: Add helper macros for loop unrolling
2024-06-29 8:43 [PATCH v13 0/5] Reduce overhead of LSMs with static calls KP Singh
@ 2024-06-29 8:43 ` KP Singh
2024-06-29 8:43 ` [PATCH v13 2/5] security: Count the LSMs enabled at compile time KP Singh
` (3 subsequent siblings)
4 siblings, 0 replies; 34+ messages in thread
From: KP Singh @ 2024-06-29 8:43 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song,
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;
}
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
Acked-by: Song Liu <song@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/unroll.h | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 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..d42fd6366373
--- /dev/null
+++ b/include/linux/unroll.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2023 Google LLC.
+ */
+
+#ifndef __UNROLL_H
+#define __UNROLL_H
+
+#include <linux/args.h>
+
+#define UNROLL(N, MACRO, args...) CONCATENATE(__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.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v13 2/5] security: Count the LSMs enabled at compile time
2024-06-29 8:43 [PATCH v13 0/5] Reduce overhead of LSMs with static calls KP Singh
2024-06-29 8:43 ` [PATCH v13 1/5] kernel: Add helper macros for loop unrolling KP Singh
@ 2024-06-29 8:43 ` KP Singh
2024-07-03 9:44 ` Rasmus Villemoes
2024-06-29 8:43 ` [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls KP Singh
` (2 subsequent siblings)
4 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-06-29 8:43 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song,
Kui-Feng Lee, KP Singh
These macros are a clever trick to determine a count of the number of
LSMs that are enabled in the config to ascertain the maximum number of
static calls that need to be configured per LSM hook.
Without this one would need to generate static calls for the total
number of LSMs in the kernel (even if they are not compiled) times the
number of LSM hooks which ends up being quite wasteful.
Suggested-by: Kui-Feng Lee <sinquersw@gmail.com>
Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <song@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/args.h | 6 +-
include/linux/lsm_count.h | 128 ++++++++++++++++++++++++++++++++++++++
2 files changed, 131 insertions(+), 3 deletions(-)
create mode 100644 include/linux/lsm_count.h
diff --git a/include/linux/args.h b/include/linux/args.h
index 8ff60a54eb7d..2e8e65d975c7 100644
--- a/include/linux/args.h
+++ b/include/linux/args.h
@@ -17,9 +17,9 @@
* that as _n.
*/
-/* This counts to 12. Any more, it will return 13th argument. */
-#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n
-#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+/* This counts to 15. Any more, it will return 16th argument. */
+#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _n, X...) _n
+#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
/* Concatenate two parameters, but allow them to be expanded beforehand. */
#define __CONCAT(a, b) a ## b
diff --git a/include/linux/lsm_count.h b/include/linux/lsm_count.h
new file mode 100644
index 000000000000..73c7cc81349b
--- /dev/null
+++ b/include/linux/lsm_count.h
@@ -0,0 +1,128 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Copyright (C) 2023 Google LLC.
+ */
+
+#ifndef __LINUX_LSM_COUNT_H
+#define __LINUX_LSM_COUNT_H
+
+#include <linux/args.h>
+
+#ifdef CONFIG_SECURITY
+
+/*
+ * Macros to count the number of LSMs enabled in the kernel at compile time.
+ */
+
+/*
+ * Capabilities is enabled when CONFIG_SECURITY is enabled.
+ */
+#if IS_ENABLED(CONFIG_SECURITY)
+#define CAPABILITIES_ENABLED 1,
+#else
+#define CAPABILITIES_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_SELINUX)
+#define SELINUX_ENABLED 1,
+#else
+#define SELINUX_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_SMACK)
+#define SMACK_ENABLED 1,
+#else
+#define SMACK_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_APPARMOR)
+#define APPARMOR_ENABLED 1,
+#else
+#define APPARMOR_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_TOMOYO)
+#define TOMOYO_ENABLED 1,
+#else
+#define TOMOYO_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_YAMA)
+#define YAMA_ENABLED 1,
+#else
+#define YAMA_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_LOADPIN)
+#define LOADPIN_ENABLED 1,
+#else
+#define LOADPIN_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_LOCKDOWN_LSM)
+#define LOCKDOWN_ENABLED 1,
+#else
+#define LOCKDOWN_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_SAFESETID)
+#define SAFESETID_ENABLED 1,
+#else
+#define SAFESETID_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_BPF_LSM)
+#define BPF_LSM_ENABLED 1,
+#else
+#define BPF_LSM_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_SECURITY_LANDLOCK)
+#define LANDLOCK_ENABLED 1,
+#else
+#define LANDLOCK_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_IMA)
+#define IMA_ENABLED 1,
+#else
+#define IMA_ENABLED
+#endif
+
+#if IS_ENABLED(CONFIG_EVM)
+#define EVM_ENABLED 1,
+#else
+#define EVM_ENABLED
+#endif
+
+/*
+ * There is a trailing comma that we need to be accounted for. This is done by
+ * using a skipped argument in __COUNT_LSMS
+ */
+#define __COUNT_LSMS(skipped_arg, args...) COUNT_ARGS(args...)
+#define COUNT_LSMS(args...) __COUNT_LSMS(args)
+
+#define MAX_LSM_COUNT \
+ COUNT_LSMS( \
+ CAPABILITIES_ENABLED \
+ SELINUX_ENABLED \
+ SMACK_ENABLED \
+ APPARMOR_ENABLED \
+ TOMOYO_ENABLED \
+ YAMA_ENABLED \
+ LOADPIN_ENABLED \
+ LOCKDOWN_ENABLED \
+ SAFESETID_ENABLED \
+ BPF_LSM_ENABLED \
+ LANDLOCK_ENABLED \
+ IMA_ENABLED \
+ EVM_ENABLED)
+
+#else
+
+#define MAX_LSM_COUNT 0
+
+#endif /* CONFIG_SECURITY */
+
+#endif /* __LINUX_LSM_COUNT_H */
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-06-29 8:43 [PATCH v13 0/5] Reduce overhead of LSMs with static calls KP Singh
2024-06-29 8:43 ` [PATCH v13 1/5] kernel: Add helper macros for loop unrolling KP Singh
2024-06-29 8:43 ` [PATCH v13 2/5] security: Count the LSMs enabled at compile time KP Singh
@ 2024-06-29 8:43 ` KP Singh
2024-07-03 0:07 ` Paul Moore
2024-06-29 8:43 ` [PATCH v13 4/5] security: Update non standard hooks to use " KP Singh
2024-06-29 8:43 ` [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
4 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-06-29 8:43 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song,
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:
0xff...0320 <+0>: endbr64
0xff...0324 <+4>: push %rbp
0xff...0325 <+5>: push %r15
0xff...0327 <+7>: push %r14
0xff...0329 <+9>: push %rbx
0xff...032a <+10>: mov %rdx,%rbx
0xff...032d <+13>: mov %esi,%ebp
0xff...032f <+15>: mov %rdi,%r14
0xff...0332 <+18>: mov $0xff...7030,%r15
0xff...0339 <+25>: mov (%r15),%r15
0xff...033c <+28>: test %r15,%r15
0xff...033f <+31>: je 0xff...0358 <security_file_ioctl+56>
0xff...0341 <+33>: mov 0x18(%r15),%r11
0xff...0345 <+37>: mov %r14,%rdi
0xff...0348 <+40>: mov %ebp,%esi
0xff...034a <+42>: mov %rbx,%rdx
0xff...034d <+45>: call 0xff...2e0 <__x86_indirect_thunk_array+352>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Indirect calls that use retpolines leading to overhead, not just due
to extra instruction but also branch misses.
0xff...0352 <+50>: test %eax,%eax
0xff...0354 <+52>: je 0xff...0339 <security_file_ioctl+25>
0xff...0356 <+54>: jmp 0xff...035a <security_file_ioctl+58>
0xff...0358 <+56>: xor %eax,%eax
0xff...035a <+58>: pop %rbx
0xff...035b <+59>: pop %r14
0xff...035d <+61>: pop %r15
0xff...035f <+63>: pop %rbp
0xff...0360 <+64>: jmp 0xff...47c4 <__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:
0xff...0ca0 <+0>: endbr64
0xff...0ca4 <+4>: nopl 0x0(%rax,%rax,1)
0xff...0ca9 <+9>: push %rbp
0xff...0caa <+10>: push %r14
0xff...0cac <+12>: push %rbx
0xff...0cad <+13>: mov %rdx,%rbx
0xff...0cb0 <+16>: mov %esi,%ebp
0xff...0cb2 <+18>: mov %rdi,%r14
0xff...0cb5 <+21>: jmp 0xff...0cc7 <security_file_ioctl+39>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Static key enabled for SELinux
0xffffffff818f0cb7 <+23>: jmp 0xff...0cde <security_file_ioctl+62>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Static key enabled for BPF LSM. This is something that is changed to
default to false to avoid the existing side effect issues of BPF LSM
[1] in a subsequent patch.
0xff...0cb9 <+25>: xor %eax,%eax
0xff...0cbb <+27>: xchg %ax,%ax
0xff...0cbd <+29>: pop %rbx
0xff...0cbe <+30>: pop %r14
0xff...0cc0 <+32>: pop %rbp
0xff...0cc1 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
0xff...0cc7 <+39>: endbr64
0xff...0ccb <+43>: mov %r14,%rdi
0xff...0cce <+46>: mov %ebp,%esi
0xff...0cd0 <+48>: mov %rbx,%rdx
0xff...0cd3 <+51>: call 0xff...3230 <selinux_file_ioctl>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Direct call to SELinux.
0xff...0cd8 <+56>: test %eax,%eax
0xff...0cda <+58>: jne 0xff...0cbd <security_file_ioctl+29>
0xff...0cdc <+60>: jmp 0xff...0cb7 <security_file_ioctl+23>
0xff...0cde <+62>: endbr64
0xff...0ce2 <+66>: mov %r14,%rdi
0xff...0ce5 <+69>: mov %ebp,%esi
0xff...0ce7 <+71>: mov %rbx,%rdx
0xff...0cea <+74>: call 0xff...e220 <bpf_lsm_file_ioctl>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Direct call to BPF LSM.
0xff...0cef <+79>: test %eax,%eax
0xff...0cf1 <+81>: jne 0xff...0cbd <security_file_ioctl+29>
0xff...0cf3 <+83>: jmp 0xff...0cb9 <security_file_ioctl+25>
0xff...0cf5 <+85>: endbr64
0xff...0cf9 <+89>: mov %r14,%rdi
0xff...0cfc <+92>: mov %ebp,%esi
0xff...0cfe <+94>: mov %rbx,%rdx
0xff...0d01 <+97>: pop %rbx
0xff...0d02 <+98>: pop %r14
0xff...0d04 <+100>: pop %rbp
0xff...0d05 <+101>: ret
0xff...0d06 <+102>: int3
0xff...0d07 <+103>: int3
0xff...0d08 <+104>: int3
0xff...0d09 <+105>: int3
While this patch uses static_branch_unlikely indicating that an LSM hook
is likely to be not present. In most cases this is still a better choice
as even when an LSM with one hook is added, empty slots are created for
all LSM hooks (especially when many LSMs that do not initialize most
hooks are present on the system).
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
lsm_for_each_hook where the lsm_callback is directly invoked as an
indirect call. These are updated in a subsequent patch to also use
static calls.
Below are results of the relevant Unixbench system benchmarks with BPF LSM
and SELinux enabled with default policies enabled with and without these
patches.
Benchmark Delta(%): (+ is better)
===============================================================================
Execl Throughput +1.9356
File Write 1024 bufsize 2000 maxblocks +6.5953
Pipe Throughput +9.5499
Pipe-based Context Switching +3.0209
Process Creation +2.3246
Shell Scripts (1 concurrent) +1.4975
System Call Overhead +2.7815
System Benchmarks Index Score (Partial Only): +3.4859
In the best case, some syscalls like eventfd_create benefitted to about ~10%.
[1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Song Liu <song@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/lsm_hooks.h | 72 ++++++++++++--
security/security.c | 198 ++++++++++++++++++++++++++------------
2 files changed, 197 insertions(+), 73 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index efd4a0655159..a66ca68485a2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -30,19 +30,66 @@
#include <linux/init.h>
#include <linux/rculist.h>
#include <linux/xattr.h>
+#include <linux/static_call.h>
+#include <linux/unroll.h>
+#include <linux/jump_label.h>
+#include <linux/lsm_count.h>
+
+#define SECURITY_HOOK_ACTIVE_KEY(HOOK, IDX) security_hook_active_##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_LOOP_UNROLL(M, ...) \
+do { \
+ UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) \
+} while (0)
+
+#define LSM_DEFINE_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__)
union security_list_options {
#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
#include "lsm_hook_defs.h"
#undef LSM_HOOK
+ void *lsm_func_addr;
};
-struct security_hook_heads {
- #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME;
- #include "lsm_hook_defs.h"
- #undef LSM_HOOK
+/*
+ * @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.
+ * @active: 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;
+ /* this needs to be true or false based on what the key defaults to */
+ struct static_key_false *active;
} __randomize_layout;
+/*
+ * 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
+} __packed __randomize_layout;
+
/**
* struct lsm_id - Identify a Linux Security Module.
* @lsm: name of the LSM, must be approved by the LSM maintainers
@@ -58,10 +105,14 @@ struct lsm_id {
/*
* 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 struct lsm_id *lsmid;
} __randomize_layout;
@@ -111,10 +162,12 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
* 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, HOOK) \
+ { \
+ .scalls = static_calls_table.NAME, \
+ .hook = { .NAME = HOOK } \
+ }
-extern struct security_hook_heads security_hook_heads;
extern char *lsm_names;
extern void security_add_hooks(struct security_hook_list *hooks, int count,
@@ -152,5 +205,6 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
__aligned(sizeof(unsigned long))
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 9c3fb2f60e2a..e0ec185cf125 100644
--- a/security/security.c
+++ b/security/security.c
@@ -30,6 +30,8 @@
#include <linux/overflow.h>
#include <net/flow.h>
#include <net/sock.h>
+#include <linux/static_call.h>
+#include <linux/jump_label.h>
/* How many LSMs were built into the kernel? */
#define LSM_COUNT (__end_lsm_info - __start_lsm_info)
@@ -93,7 +95,6 @@ const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX + 1] = {
[LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
};
-struct security_hook_heads security_hook_heads __ro_after_init;
static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
static struct kmem_cache *lsm_file_cache;
@@ -112,6 +113,51 @@ static __initconst const char *const builtin_lsm_order = CONFIG_LSM;
static __initdata struct lsm_info **ordered_lsms;
static __initdata struct lsm_info *exclusive;
+
+#ifdef CONFIG_HAVE_STATIC_CALL
+#define LSM_HOOK_TRAMP(NAME, NUM) \
+ &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM))
+#else
+#define LSM_HOOK_TRAMP(NAME, NUM) NULL
+#endif
+
+/*
+ * 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_ACTIVE_KEY(NAME, NUM));
+
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+ LSM_DEFINE_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 __ro_after_init = {
+#define INIT_LSM_STATIC_CALL(NUM, NAME) \
+ (struct lsm_static_call) { \
+ .key = &STATIC_CALL_KEY(LSM_STATIC_CALL(NAME, NUM)), \
+ .trampoline = LSM_HOOK_TRAMP(NAME, NUM), \
+ .active = &SECURITY_HOOK_ACTIVE_KEY(NAME, NUM), \
+ },
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+ .NAME = { \
+ LSM_DEFINE_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 { \
@@ -172,7 +218,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. */
@@ -352,6 +398,25 @@ 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_func_addr);
+ scall->hl = hl;
+ static_branch_enable(scall->active);
+ return;
+ }
+ scall++;
+ }
+ panic("%s - Ran out of static slots.\n", __func__);
+}
+
static void __init lsm_early_cred(struct cred *cred);
static void __init lsm_early_task(struct task_struct *task);
@@ -432,11 +497,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;
@@ -564,7 +624,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
for (i = 0; i < count; i++) {
hooks[i].lsmid = lsmid;
- hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+ lsm_static_call_init(&hooks[i]);
}
/*
@@ -856,29 +916,43 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
* call_int_hook:
* This is a hook that returns a value.
*/
+#define __CALL_STATIC_VOID(NUM, HOOK, ...) \
+do { \
+ if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
+ static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
+ } \
+} while (0);
-#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(HOOK, ...) \
+ do { \
+ LSM_LOOP_UNROLL(__CALL_STATIC_VOID, HOOK, __VA_ARGS__); \
} while (0)
-#define call_int_hook(FUNC, ...) ({ \
- int RC = LSM_RET_DEFAULT(FUNC); \
- do { \
- struct security_hook_list *P; \
- \
- hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
- RC = P->hook.FUNC(__VA_ARGS__); \
- if (RC != LSM_RET_DEFAULT(FUNC)) \
- break; \
- } \
- } while (0); \
- RC; \
+
+#define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...) \
+do { \
+ if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
+ R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
+ if (R != LSM_RET_DEFAULT(HOOK)) \
+ goto LABEL; \
+ } \
+} while (0);
+
+#define call_int_hook(HOOK, ...) \
+({ \
+ __label__ out; \
+ int RC = LSM_RET_DEFAULT(HOOK); \
+ \
+ LSM_LOOP_UNROLL(__CALL_STATIC_INT, RC, HOOK, out, __VA_ARGS__); \
+out: \
+ RC; \
})
+#define lsm_for_each_hook(scall, NAME) \
+ for (scall = static_calls_table.NAME; \
+ scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
+ if (static_key_enabled(&scall->active->key))
+
/* Security operations */
/**
@@ -1113,7 +1187,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;
@@ -1124,8 +1198,8 @@ 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);
+ lsm_for_each_hook(scall, vm_enough_memory) {
+ rc = scall->hl->hook.vm_enough_memory(mm, pages);
if (rc <= 0) {
cap_sys_admin = 0;
break;
@@ -1272,13 +1346,12 @@ 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);
+ lsm_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)
@@ -1508,12 +1581,11 @@ int security_sb_set_mnt_opts(struct super_block *sb,
unsigned long kern_flags,
unsigned long *set_kern_flags)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
- hlist_for_each_entry(hp, &security_hook_heads.sb_set_mnt_opts,
- list) {
- rc = hp->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
+ lsm_for_each_hook(scall, sb_set_mnt_opts) {
+ rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
set_kern_flags);
if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
break;
@@ -1708,7 +1780,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
const initxattrs initxattrs, void *fs_data)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
struct xattr *new_xattrs = NULL;
int ret = -EOPNOTSUPP, xattr_count = 0;
@@ -1726,9 +1798,8 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
return -ENOMEM;
}
- hlist_for_each_entry(hp, &security_hook_heads.inode_init_security,
- list) {
- ret = hp->hook.inode_init_security(inode, dir, qstr, new_xattrs,
+ lsm_for_each_hook(scall, inode_init_security) {
+ ret = scall->hl->hook.inode_init_security(inode, dir, qstr, new_xattrs,
&xattr_count);
if (ret && ret != -EOPNOTSUPP)
goto out;
@@ -3560,10 +3631,10 @@ 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);
+ lsm_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)
@@ -3969,7 +4040,7 @@ EXPORT_SYMBOL(security_d_instantiate);
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
u32 __user *size, u32 flags)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
u8 __user *base = (u8 __user *)uctx;
u32 entrysize;
@@ -4007,13 +4078,13 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
* In the usual case gather all the data from the LSMs.
* In the single case only get the data from the LSM specified.
*/
- hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
- if (single && lctx.id != hp->lsmid->id)
+ lsm_for_each_hook(scall, getselfattr) {
+ if (single && lctx.id != scall->hl->lsmid->id)
continue;
entrysize = left;
if (base)
uctx = (struct lsm_ctx __user *)(base + total);
- rc = hp->hook.getselfattr(attr, uctx, &entrysize, flags);
+ rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
if (rc == -EOPNOTSUPP) {
rc = 0;
continue;
@@ -4062,7 +4133,7 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
u32 size, u32 flags)
{
- struct security_hook_list *hp;
+ struct lsm_static_call *scall;
struct lsm_ctx *lctx;
int rc = LSM_RET_DEFAULT(setselfattr);
u64 required_len;
@@ -4085,9 +4156,9 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
goto free_out;
}
- hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
- if ((hp->lsmid->id) == lctx->id) {
- rc = hp->hook.setselfattr(attr, lctx, size, flags);
+ lsm_for_each_hook(scall, setselfattr)
+ if ((scall->hl->lsmid->id) == lctx->id) {
+ rc = scall->hl->hook.setselfattr(attr, lctx, size, flags);
break;
}
@@ -4110,12 +4181,12 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
int security_getprocattr(struct task_struct *p, int lsmid, 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 (lsmid != 0 && lsmid != hp->lsmid->id)
+ lsm_for_each_hook(scall, getprocattr) {
+ if (lsmid != 0 && lsmid != scall->hl->lsmid->id)
continue;
- return hp->hook.getprocattr(p, name, value);
+ return scall->hl->hook.getprocattr(p, name, value);
}
return LSM_RET_DEFAULT(getprocattr);
}
@@ -4134,12 +4205,12 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
*/
int security_setprocattr(int lsmid, 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 (lsmid != 0 && lsmid != hp->lsmid->id)
+ lsm_for_each_hook(scall, setprocattr) {
+ if (lsmid != 0 && lsmid != scall->hl->lsmid->id)
continue;
- return hp->hook.setprocattr(name, value, size);
+ return scall->hl->hook.setprocattr(name, value, size);
}
return LSM_RET_DEFAULT(setprocattr);
}
@@ -5257,7 +5328,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);
/*
@@ -5269,9 +5340,8 @@ 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);
+ lsm_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.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v13 4/5] security: Update non standard hooks to use static calls
2024-06-29 8:43 [PATCH v13 0/5] Reduce overhead of LSMs with static calls KP Singh
` (2 preceding siblings ...)
2024-06-29 8:43 ` [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls KP Singh
@ 2024-06-29 8:43 ` KP Singh
2024-07-03 0:07 ` Paul Moore
2024-06-29 8:43 ` [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
4 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-06-29 8:43 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song,
KP Singh
There are some LSM hooks which do not use the common pattern followed
by other LSM hooks and thus cannot use call_{int, void}_hook macros and
instead use lsm_for_each_hook macro which still results in indirect
call.
There is one additional generalizable pattern where a hook matching an
lsmid is called and the indirect calls for these are addressed with the
newly added call_hook_with_lsmid macro which internally uses an
implementation similar to call_int_hook but has an additional check that
matches the lsmid.
For the generic case the lsm_for_each_hook macro is updated to accept
logic before and after the invocation of the LSM hook (static call) in
the unrolled loop.
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
security/security.c | 248 +++++++++++++++++++++++++-------------------
1 file changed, 144 insertions(+), 104 deletions(-)
diff --git a/security/security.c b/security/security.c
index e0ec185cf125..4f0f35857217 100644
--- a/security/security.c
+++ b/security/security.c
@@ -948,10 +948,48 @@ out: \
RC; \
})
-#define lsm_for_each_hook(scall, NAME) \
- for (scall = static_calls_table.NAME; \
- scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
- if (static_key_enabled(&scall->active->key))
+/*
+ * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
+ * current hook
+ */
+#define current_lsmid() _hook_lsmid
+
+#define __CALL_HOOK(NUM, HOOK, RC, BLOCK_BEFORE, BLOCK_AFTER, ...) \
+do { \
+ int __maybe_unused _hook_lsmid; \
+ \
+ if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
+ _hook_lsmid = static_calls_table.HOOK[NUM].hl->lsmid->id; \
+ BLOCK_BEFORE \
+ RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
+ BLOCK_AFTER \
+ } \
+} while (0);
+
+#define lsm_for_each_hook(HOOK, RC, BLOCK_AFTER, ...) \
+ LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BLOCK_AFTER, __VA_ARGS__)
+
+#define call_hook_with_lsmid(HOOK, LSMID, ...) \
+({ \
+ __label__ out; \
+ int RC = LSM_RET_DEFAULT(HOOK); \
+ \
+ LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, \
+ /* BLOCK BEFORE INVOCATION */ \
+ { \
+ if (current_lsmid() != LSMID) \
+ continue; \
+ }, \
+ /* END BLOCK BEFORE INVOCATION */ \
+ /* BLOCK AFTER INVOCATION */ \
+ { \
+ goto out; \
+ }, \
+ /* END BLOCK AFTER INVOCATION */ \
+ __VA_ARGS__); \
+out: \
+ RC; \
+})
/* Security operations */
@@ -1187,7 +1225,6 @@ int security_settime64(const struct timespec64 *ts, const struct timezone *tz)
*/
int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
{
- struct lsm_static_call *scall;
int cap_sys_admin = 1;
int rc;
@@ -1198,13 +1235,20 @@ 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.
*/
- lsm_for_each_hook(scall, vm_enough_memory) {
- rc = scall->hl->hook.vm_enough_memory(mm, pages);
- if (rc <= 0) {
- cap_sys_admin = 0;
- break;
- }
- }
+
+ lsm_for_each_hook(
+ vm_enough_memory, rc,
+ /* BLOCK AFTER INVOCATION */
+ {
+ if (rc <= 0) {
+ cap_sys_admin = 0;
+ goto out;
+ }
+ },
+ /* END BLOCK AFTER INVOCATION */
+ mm, pages);
+
+out:
return __vm_enough_memory(mm, pages, cap_sys_admin);
}
@@ -1346,17 +1390,21 @@ 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 lsm_static_call *scall;
- int trc;
+ int trc = LSM_RET_DEFAULT(fs_context_parse_param);
int rc = -ENOPARAM;
- lsm_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;
- }
+ lsm_for_each_hook(
+ fs_context_parse_param, trc,
+ /* BLOCK AFTER INVOCATION */
+ {
+ if (trc == 0)
+ rc = 0;
+ else if (trc != -ENOPARAM)
+ return trc;
+ },
+ /* END BLOCK AFTER INVOCATION */
+ fc, param);
+
return rc;
}
@@ -1581,15 +1629,19 @@ int security_sb_set_mnt_opts(struct super_block *sb,
unsigned long kern_flags,
unsigned long *set_kern_flags)
{
- struct lsm_static_call *scall;
int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
- lsm_for_each_hook(scall, sb_set_mnt_opts) {
- rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
- set_kern_flags);
- if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
- break;
- }
+ lsm_for_each_hook(
+ sb_set_mnt_opts, rc,
+ /* BLOCK AFTER INVOCATION */
+ {
+ if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
+ goto out;
+ },
+ /* END BLOCK AFTER INVOCATION */
+ sb, mnt_opts, kern_flags, set_kern_flags);
+
+out:
return rc;
}
EXPORT_SYMBOL(security_sb_set_mnt_opts);
@@ -1780,7 +1832,6 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
const initxattrs initxattrs, void *fs_data)
{
- struct lsm_static_call *scall;
struct xattr *new_xattrs = NULL;
int ret = -EOPNOTSUPP, xattr_count = 0;
@@ -1798,18 +1849,21 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
return -ENOMEM;
}
- lsm_for_each_hook(scall, inode_init_security) {
- ret = scall->hl->hook.inode_init_security(inode, dir, qstr, new_xattrs,
- &xattr_count);
- if (ret && ret != -EOPNOTSUPP)
- goto out;
+ lsm_for_each_hook(
+ inode_init_security, ret,
+ /* BLOCK AFTER INVOCATION */
+ {
/*
* As documented in lsm_hooks.h, -EOPNOTSUPP in this context
* means that the LSM is not willing to provide an xattr, not
* that it wants to signal an error. Thus, continue to invoke
* the remaining LSMs.
*/
- }
+ if (ret && ret != -EOPNOTSUPP)
+ goto out;
+ },
+ /* END BLOCK AFTER INVOCATION */
+ inode, dir, qstr, new_xattrs, &xattr_count);
/* If initxattrs() is NULL, xattr_count is zero, skip the call. */
if (!xattr_count)
@@ -3631,16 +3685,21 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
{
int thisrc;
int rc = LSM_RET_DEFAULT(task_prctl);
- struct lsm_static_call *scall;
-
- lsm_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;
- }
- }
+
+ lsm_for_each_hook(
+ task_prctl, thisrc,
+ /* BLOCK AFTER INVOCATION */
+ {
+ if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
+ rc = thisrc;
+ if (thisrc != 0)
+ goto out;
+ }
+ },
+ /* END BLOCK AFTER INVOCATION */
+ option, arg2, arg3, arg4, arg5);
+
+out:
return rc;
}
@@ -4040,7 +4099,6 @@ EXPORT_SYMBOL(security_d_instantiate);
int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
u32 __user *size, u32 flags)
{
- struct lsm_static_call *scall;
struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
u8 __user *base = (u8 __user *)uctx;
u32 entrysize;
@@ -4078,31 +4136,42 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
* In the usual case gather all the data from the LSMs.
* In the single case only get the data from the LSM specified.
*/
- lsm_for_each_hook(scall, getselfattr) {
- if (single && lctx.id != scall->hl->lsmid->id)
- continue;
- entrysize = left;
- if (base)
- uctx = (struct lsm_ctx __user *)(base + total);
- rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
- if (rc == -EOPNOTSUPP) {
- rc = 0;
- continue;
- }
- if (rc == -E2BIG) {
- rc = 0;
- left = 0;
- toobig = true;
- } else if (rc < 0)
- return rc;
- else
- left -= entrysize;
+ LSM_LOOP_UNROLL(
+ __CALL_HOOK, getselfattr, rc,
+ /* BLOCK BEFORE INVOCATION */
+ {
+ if (single && lctx.id != current_lsmid())
+ continue;
+ entrysize = left;
+ if (base)
+ uctx = (struct lsm_ctx __user *)(base + total);
+ },
+ /* END BLOCK BEFORE INVOCATION */
+ /* BLOCK AFTER INVOCATION */
+ {
+ if (rc == -EOPNOTSUPP) {
+ rc = 0;
+ } else {
+ if (rc == -E2BIG) {
+ rc = 0;
+ left = 0;
+ toobig = true;
+ } else if (rc < 0)
+ return rc;
+ else
+ left -= entrysize;
+
+ total += entrysize;
+ count += rc;
+ if (single)
+ goto out;
+ }
+ },
+ /* END BLOCK AFTER INVOCATION */
+ attr, uctx, &entrysize, flags);
+
+out:
- total += entrysize;
- count += rc;
- if (single)
- break;
- }
if (put_user(total, size))
return -EFAULT;
if (toobig)
@@ -4133,9 +4202,8 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
u32 size, u32 flags)
{
- struct lsm_static_call *scall;
struct lsm_ctx *lctx;
- int rc = LSM_RET_DEFAULT(setselfattr);
+ int rc;
u64 required_len;
if (flags)
@@ -4156,11 +4224,7 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
goto free_out;
}
- lsm_for_each_hook(scall, setselfattr)
- if ((scall->hl->lsmid->id) == lctx->id) {
- rc = scall->hl->hook.setselfattr(attr, lctx, size, flags);
- break;
- }
+ rc = call_hook_with_lsmid(setselfattr, lctx->id, attr, lctx, size, flags);
free_out:
kfree(lctx);
@@ -4181,14 +4245,7 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value)
{
- struct lsm_static_call *scall;
-
- lsm_for_each_hook(scall, getprocattr) {
- if (lsmid != 0 && lsmid != scall->hl->lsmid->id)
- continue;
- return scall->hl->hook.getprocattr(p, name, value);
- }
- return LSM_RET_DEFAULT(getprocattr);
+ return call_hook_with_lsmid(getprocattr, lsmid, p, name, value);
}
/**
@@ -4205,14 +4262,7 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
*/
int security_setprocattr(int lsmid, const char *name, void *value, size_t size)
{
- struct lsm_static_call *scall;
-
- lsm_for_each_hook(scall, setprocattr) {
- if (lsmid != 0 && lsmid != scall->hl->lsmid->id)
- continue;
- return scall->hl->hook.setprocattr(name, value, size);
- }
- return LSM_RET_DEFAULT(setprocattr);
+ return call_hook_with_lsmid(setprocattr, lsmid, name, value, size);
}
/**
@@ -5328,23 +5378,13 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
struct xfrm_policy *xp,
const struct flowi_common *flic)
{
- struct lsm_static_call *scall;
- int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
-
/*
* Since this function is expected to return 0 or 1, the judgment
* becomes difficult if multiple LSMs supply this call. Fortunately,
* we can use the first LSM's judgment because currently only SELinux
* supplies this call.
- *
- * For speed optimization, we explicitly break the loop rather than
- * using the macro
*/
- lsm_for_each_hook(scall, xfrm_state_pol_flow_match) {
- rc = scall->hl->hook.xfrm_state_pol_flow_match(x, xp, flic);
- break;
- }
- return rc;
+ return call_int_hook(xfrm_state_pol_flow_match, x, xp, flic);
}
/**
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached
2024-06-29 8:43 [PATCH v13 0/5] Reduce overhead of LSMs with static calls KP Singh
` (3 preceding siblings ...)
2024-06-29 8:43 ` [PATCH v13 4/5] security: Update non standard hooks to use " KP Singh
@ 2024-06-29 8:43 ` KP Singh
2024-07-03 0:07 ` Paul Moore
4 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-06-29 8:43 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song,
KP Singh
BPF LSM hooks have side-effects (even when a default value's 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:
0xff...0e30 <+0>: endbr64
0xff...0e34 <+4>: nopl 0x0(%rax,%rax,1)
0xff...0e39 <+9>: push %rbp
0xff...0e3a <+10>: push %r14
0xff...0e3c <+12>: push %rbx
0xff...0e3d <+13>: mov %rdx,%rbx
0xff...0e40 <+16>: mov %esi,%ebp
0xff...0e42 <+18>: mov %rdi,%r14
0xff...0e45 <+21>: jmp 0xff...0e57 <security_file_ioctl+39>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Static key enabled for SELinux
0xff...0e47 <+23>: xchg %ax,%ax
^^^^^^^^^^^^^^
Static key disabled for BPF. This gets patched when a BPF LSM
program is attached
0xff...0e49 <+25>: xor %eax,%eax
0xff...0e4b <+27>: xchg %ax,%ax
0xff...0e4d <+29>: pop %rbx
0xff...0e4e <+30>: pop %r14
0xff...0e50 <+32>: pop %rbp
0xff...0e51 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
0xff...0e57 <+39>: endbr64
0xff...0e5b <+43>: mov %r14,%rdi
0xff...0e5e <+46>: mov %ebp,%esi
0xff...0e60 <+48>: mov %rbx,%rdx
0xff...0e63 <+51>: call 0xff...33c0 <selinux_file_ioctl>
0xff...0e68 <+56>: test %eax,%eax
0xff...0e6a <+58>: jne 0xff...0e4d <security_file_ioctl+29>
0xff...0e6c <+60>: jmp 0xff...0e47 <security_file_ioctl+23>
0xff...0e6e <+62>: endbr64
0xff...0e72 <+66>: mov %r14,%rdi
0xff...0e75 <+69>: mov %ebp,%esi
0xff...0e77 <+71>: mov %rbx,%rdx
0xff...0e7a <+74>: call 0xff...e3b0 <bpf_lsm_file_ioctl>
0xff...0e7f <+79>: test %eax,%eax
0xff...0e81 <+81>: jne 0xff...0e4d <security_file_ioctl+29>
0xff...0e83 <+83>: jmp 0xff...0e49 <security_file_ioctl+25>
0xff...0e85 <+85>: endbr64
0xff...0e89 <+89>: mov %r14,%rdi
0xff...0e8c <+92>: mov %ebp,%esi
0xff...0e8e <+94>: mov %rbx,%rdx
0xff...0e91 <+97>: pop %rbx
0xff...0e92 <+98>: pop %r14
0xff...0e94 <+100>: pop %rbp
0xff...0e95 <+101>: ret
This patch enables this by providing a LSM_HOOK_INIT_RUNTIME variant
that allows the LSMs to opt-in to hooks which can be toggled at runtime
which with security_toogle_hook.
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++-
kernel/bpf/trampoline.c | 40 +++++++++++++++++++++++++++++++++++----
security/bpf/hooks.c | 2 +-
security/security.c | 36 ++++++++++++++++++++++++++++++++++-
4 files changed, 101 insertions(+), 7 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a66ca68485a2..dbe0f40f7f67 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -110,11 +110,14 @@ struct lsm_id {
* @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 struct lsm_id *lsmid;
+ bool runtime;
} __randomize_layout;
/*
@@ -165,7 +168,19 @@ static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
#define LSM_HOOK_INIT(NAME, HOOK) \
{ \
.scalls = static_calls_table.NAME, \
- .hook = { .NAME = HOOK } \
+ .hook = { .NAME = HOOK }, \
+ .runtime = false \
+ }
+
+/*
+ * Initialize hooks that are inactive by default and
+ * enabled at runtime with security_toggle_hook.
+ */
+#define LSM_HOOK_INIT_RUNTIME(NAME, HOOK) \
+ { \
+ .scalls = static_calls_table.NAME, \
+ .hook = { .NAME = HOOK }, \
+ .runtime = true \
}
extern char *lsm_names;
@@ -207,4 +222,17 @@ extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
extern int lsm_inode_alloc(struct inode *inode);
extern struct lsm_static_calls_table static_calls_table __ro_after_init;
+#ifdef CONFIG_SECURITY
+
+int security_toggle_hook(void *addr, bool value);
+
+#else
+
+static inline int security_toggle_hook(void *addr, bool value)
+{
+ return -EINVAL;
+}
+
+#endif /* CONFIG_SECURITY */
+
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f8302a5ca400..69d3eb490a1b 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -523,6 +523,21 @@ static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog)
}
}
+static int bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr,
+ enum bpf_tramp_prog_type kind)
+{
+ struct bpf_tramp_link *link;
+ bool found = false;
+
+ hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) {
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+ found = true;
+ break;
+ }
+ }
+ return security_toggle_hook(tr->func.addr, found);
+}
+
static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
{
enum bpf_tramp_prog_type kind;
@@ -562,11 +577,22 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
tr->progs_cnt[kind]++;
- err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
- if (err) {
- hlist_del_init(&link->tramp_hlist);
- tr->progs_cnt[kind]--;
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+ err = bpf_trampoline_toggle_lsm(tr, kind);
+ if (err)
+ goto cleanup;
}
+
+ err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
+ if (err)
+ goto cleanup;
+
+ return 0;
+
+cleanup:
+ hlist_del_init(&link->tramp_hlist);
+ tr->progs_cnt[kind]--;
return err;
}
@@ -595,6 +621,12 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
}
hlist_del_init(&link->tramp_hlist);
tr->progs_cnt[kind]--;
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+ err = bpf_trampoline_toggle_lsm(tr, kind);
+ WARN(err, "BUG: unable to toggle BPF LSM hook");
+ }
+
return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
}
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 57b9ffd53c98..8452e0835f56 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -9,7 +9,7 @@
static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = {
#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
- LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
+ LSM_HOOK_INIT_RUNTIME(NAME, bpf_lsm_##NAME),
#include <linux/lsm_hook_defs.h>
#undef LSM_HOOK
LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
diff --git a/security/security.c b/security/security.c
index 4f0f35857217..1c448fe529f9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -409,7 +409,9 @@ static void __init lsm_static_call_init(struct security_hook_list *hl)
__static_call_update(scall->key, scall->trampoline,
hl->hook.lsm_func_addr);
scall->hl = hl;
- static_branch_enable(scall->active);
+ /* Runtime hooks are inactive by default */
+ if (!hl->runtime)
+ static_branch_enable(scall->active);
return;
}
scall++;
@@ -888,6 +890,38 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
return rc;
}
+/**
+ * security_toggle_hook - Toggle the state of the LSM hook.
+ * @hook_addr: The address of the hook to be toggled.
+ * @state: Whether to enable for disable the hook.
+ *
+ * Returns 0 on success, -EINVAL if the address is not found.
+ */
+int security_toggle_hook(void *hook_addr, bool state)
+{
+ unsigned long num_entries =
+ (sizeof(static_calls_table) / sizeof(struct lsm_static_call));
+ void *scalls_table = ((void *)&static_calls_table);
+ struct lsm_static_call *scall;
+ int i;
+
+ for (i = 0; i < num_entries; i++) {
+ scall = scalls_table + (i * sizeof(struct lsm_static_call));
+ if (!scall->hl || !scall->hl->runtime)
+ continue;
+
+ if (scall->hl->hook.lsm_func_addr != hook_addr)
+ continue;
+
+ if (state)
+ static_branch_enable(scall->active);
+ else
+ static_branch_disable(scall->active);
+ return 0;
+ }
+ return -EINVAL;
+}
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
--
2.45.2.803.g4e1b14247a-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-06-29 8:43 ` [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls KP Singh
@ 2024-07-03 0:07 ` Paul Moore
2024-07-03 16:54 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-03 0:07 UTC (permalink / raw)
To: KP Singh, linux-security-module, bpf
Cc: ast, casey, andrii, keescook, daniel, renauld, revest, song,
KP Singh
On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> 0xff...0320 <+0>: endbr64
> 0xff...0324 <+4>: push %rbp
> 0xff...0325 <+5>: push %r15
> 0xff...0327 <+7>: push %r14
> 0xff...0329 <+9>: push %rbx
> 0xff...032a <+10>: mov %rdx,%rbx
> 0xff...032d <+13>: mov %esi,%ebp
> 0xff...032f <+15>: mov %rdi,%r14
> 0xff...0332 <+18>: mov $0xff...7030,%r15
> 0xff...0339 <+25>: mov (%r15),%r15
> 0xff...033c <+28>: test %r15,%r15
> 0xff...033f <+31>: je 0xff...0358 <security_file_ioctl+56>
> 0xff...0341 <+33>: mov 0x18(%r15),%r11
> 0xff...0345 <+37>: mov %r14,%rdi
> 0xff...0348 <+40>: mov %ebp,%esi
> 0xff...034a <+42>: mov %rbx,%rdx
>
> 0xff...034d <+45>: call 0xff...2e0 <__x86_indirect_thunk_array+352>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Indirect calls that use retpolines leading to overhead, not just due
> to extra instruction but also branch misses.
>
> 0xff...0352 <+50>: test %eax,%eax
> 0xff...0354 <+52>: je 0xff...0339 <security_file_ioctl+25>
> 0xff...0356 <+54>: jmp 0xff...035a <security_file_ioctl+58>
> 0xff...0358 <+56>: xor %eax,%eax
> 0xff...035a <+58>: pop %rbx
> 0xff...035b <+59>: pop %r14
> 0xff...035d <+61>: pop %r15
> 0xff...035f <+63>: pop %rbp
> 0xff...0360 <+64>: jmp 0xff...47c4 <__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].
I believe there have been improvements in the LSM framework since the
original problems were reported and I'm not entirely sure if this is
still a problem. In particular I believe that commit
260017f31a8c ("lsm: use default hook return value in call_int_hook()")
and 61df7b828204 ("lsm: fixup the inode xattr capability handling")
should fix the more obvious problems. I'd like to know if you are
aware of any others? If not, the text above should be adjusted and
we should reconsider patch 5/5. If there are other problems I'd
like to better understand them as there may be an independent
solution for those particular problems.
> 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:
> 0xff...0ca0 <+0>: endbr64
> 0xff...0ca4 <+4>: nopl 0x0(%rax,%rax,1)
> 0xff...0ca9 <+9>: push %rbp
> 0xff...0caa <+10>: push %r14
> 0xff...0cac <+12>: push %rbx
> 0xff...0cad <+13>: mov %rdx,%rbx
> 0xff...0cb0 <+16>: mov %esi,%ebp
> 0xff...0cb2 <+18>: mov %rdi,%r14
> 0xff...0cb5 <+21>: jmp 0xff...0cc7 <security_file_ioctl+39>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Static key enabled for SELinux
>
> 0xffffffff818f0cb7 <+23>: jmp 0xff...0cde <security_file_ioctl+62>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Static key enabled for BPF LSM. This is something that is changed to
> default to false to avoid the existing side effect issues of BPF LSM
> [1] in a subsequent patch.
>
> 0xff...0cb9 <+25>: xor %eax,%eax
> 0xff...0cbb <+27>: xchg %ax,%ax
> 0xff...0cbd <+29>: pop %rbx
> 0xff...0cbe <+30>: pop %r14
> 0xff...0cc0 <+32>: pop %rbp
> 0xff...0cc1 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
> 0xff...0cc7 <+39>: endbr64
> 0xff...0ccb <+43>: mov %r14,%rdi
> 0xff...0cce <+46>: mov %ebp,%esi
> 0xff...0cd0 <+48>: mov %rbx,%rdx
> 0xff...0cd3 <+51>: call 0xff...3230 <selinux_file_ioctl>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Direct call to SELinux.
>
> 0xff...0cd8 <+56>: test %eax,%eax
> 0xff...0cda <+58>: jne 0xff...0cbd <security_file_ioctl+29>
> 0xff...0cdc <+60>: jmp 0xff...0cb7 <security_file_ioctl+23>
> 0xff...0cde <+62>: endbr64
> 0xff...0ce2 <+66>: mov %r14,%rdi
> 0xff...0ce5 <+69>: mov %ebp,%esi
> 0xff...0ce7 <+71>: mov %rbx,%rdx
> 0xff...0cea <+74>: call 0xff...e220 <bpf_lsm_file_ioctl>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Direct call to BPF LSM.
>
> 0xff...0cef <+79>: test %eax,%eax
> 0xff...0cf1 <+81>: jne 0xff...0cbd <security_file_ioctl+29>
> 0xff...0cf3 <+83>: jmp 0xff...0cb9 <security_file_ioctl+25>
> 0xff...0cf5 <+85>: endbr64
> 0xff...0cf9 <+89>: mov %r14,%rdi
> 0xff...0cfc <+92>: mov %ebp,%esi
> 0xff...0cfe <+94>: mov %rbx,%rdx
> 0xff...0d01 <+97>: pop %rbx
> 0xff...0d02 <+98>: pop %r14
> 0xff...0d04 <+100>: pop %rbp
> 0xff...0d05 <+101>: ret
> 0xff...0d06 <+102>: int3
> 0xff...0d07 <+103>: int3
> 0xff...0d08 <+104>: int3
> 0xff...0d09 <+105>: int3
>
> While this patch uses static_branch_unlikely indicating that an LSM hook
> is likely to be not present. In most cases this is still a better choice
> as even when an LSM with one hook is added, empty slots are created for
> all LSM hooks (especially when many LSMs that do not initialize most
> hooks are present on the system).
>
> There are some hooks that don't use the call_int_hook and
> call_void_hook.
I think you mean "or" and not "and", yes?
> These hooks are updated to use a new macro called
> lsm_for_each_hook where the lsm_callback is directly invoked as an
> indirect call. These are updated in a subsequent patch to also use
> static calls.
>
> Below are results of the relevant Unixbench system benchmarks with BPF LSM
> and SELinux enabled with default policies enabled with and without these
> patches.
>
> Benchmark Delta(%): (+ is better)
> ===============================================================================
> Execl Throughput +1.9356
> File Write 1024 bufsize 2000 maxblocks +6.5953
> Pipe Throughput +9.5499
> Pipe-based Context Switching +3.0209
> Process Creation +2.3246
> Shell Scripts (1 concurrent) +1.4975
> System Call Overhead +2.7815
> System Benchmarks Index Score (Partial Only): +3.4859
>
> In the best case, some syscalls like eventfd_create benefitted to about ~10%.
>
> [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/
>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Song Liu <song@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
> include/linux/lsm_hooks.h | 72 ++++++++++++--
> security/security.c | 198 ++++++++++++++++++++++++++------------
> 2 files changed, 197 insertions(+), 73 deletions(-)
...
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index efd4a0655159..a66ca68485a2 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -30,19 +30,66 @@
> #include <linux/init.h>
> #include <linux/rculist.h>
> #include <linux/xattr.h>
> +#include <linux/static_call.h>
> +#include <linux/unroll.h>
> +#include <linux/jump_label.h>
> +#include <linux/lsm_count.h>
> +
> +#define SECURITY_HOOK_ACTIVE_KEY(HOOK, IDX) security_hook_active_##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_LOOP_UNROLL(M, ...) \
> +do { \
> + UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) \
> +} while (0)
> +
> +#define LSM_DEFINE_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__)
All of the macros above (SECURITY_HOOK_ACTIVE_KEY, LSM_STATIC_CALL,
LSM_LOOP_UNROLL, and LSM_DEFINE_UNROLL) are only used in
security/security.c, yes? If so, is there a reason why they are
defined here and not in security/security.c?
> diff --git a/security/security.c b/security/security.c
> index 9c3fb2f60e2a..e0ec185cf125 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -112,6 +113,51 @@ static __initconst const char *const builtin_lsm_order = CONFIG_LSM;
> static __initdata struct lsm_info **ordered_lsms;
> static __initdata struct lsm_info *exclusive;
>
> +
> +#ifdef CONFIG_HAVE_STATIC_CALL
> +#define LSM_HOOK_TRAMP(NAME, NUM) \
> + &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM))
> +#else
> +#define LSM_HOOK_TRAMP(NAME, NUM) NULL
> +#endif
> +
> +/*
> + * 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_ACTIVE_KEY(NAME, NUM));
> +
> +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> + LSM_DEFINE_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__)
> +#include <linux/lsm_hook_defs.h>
> +#undef LSM_HOOK
> +#undef DEFINE_LSM_STATIC_CALL
If you end up respinning the patchset, drop the two blank lines
before the DEFINE_LSM_STATIC_CALL and LSM_HOOK definitions.
> @@ -856,29 +916,43 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
> * call_int_hook:
> * This is a hook that returns a value.
> */
> +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \
> +do { \
> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + } \
> +} while (0);
>
> -#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(HOOK, ...) \
> + do { \
> + LSM_LOOP_UNROLL(__CALL_STATIC_VOID, HOOK, __VA_ARGS__); \
> } while (0)
>
> -#define call_int_hook(FUNC, ...) ({ \
> - int RC = LSM_RET_DEFAULT(FUNC); \
> - do { \
> - struct security_hook_list *P; \
> - \
> - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> - RC = P->hook.FUNC(__VA_ARGS__); \
> - if (RC != LSM_RET_DEFAULT(FUNC)) \
> - break; \
> - } \
> - } while (0); \
> - RC; \
> +
> +#define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...) \
> +do { \
> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + if (R != LSM_RET_DEFAULT(HOOK)) \
> + goto LABEL; \
> + } \
> +} while (0);
> +
> +#define call_int_hook(HOOK, ...) \
> +({ \
> + __label__ out; \
This is another only-if-you-respin, consider /out/OUT/ so it is more
consistent.
> + int RC = LSM_RET_DEFAULT(HOOK); \
> + \
> + LSM_LOOP_UNROLL(__CALL_STATIC_INT, RC, HOOK, out, __VA_ARGS__); \
> +out: \
> + RC; \
> })
>
> +#define lsm_for_each_hook(scall, NAME) \
> + for (scall = static_calls_table.NAME; \
> + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> + if (static_key_enabled(&scall->active->key))
This is probably a stupid question, but why use static_key_enabled()
here instead of static_branch_unlikely() as in the call_XXX_macros?
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 4/5] security: Update non standard hooks to use static calls
2024-06-29 8:43 ` [PATCH v13 4/5] security: Update non standard hooks to use " KP Singh
@ 2024-07-03 0:07 ` Paul Moore
2024-07-09 12:36 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-03 0:07 UTC (permalink / raw)
To: KP Singh, linux-security-module, bpf
Cc: ast, casey, andrii, keescook, daniel, renauld, revest, song,
KP Singh
On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote:
>
> There are some LSM hooks which do not use the common pattern followed
> by other LSM hooks and thus cannot use call_{int, void}_hook macros and
> instead use lsm_for_each_hook macro which still results in indirect
> call.
>
> There is one additional generalizable pattern where a hook matching an
> lsmid is called and the indirect calls for these are addressed with the
> newly added call_hook_with_lsmid macro which internally uses an
> implementation similar to call_int_hook but has an additional check that
> matches the lsmid.
>
> For the generic case the lsm_for_each_hook macro is updated to accept
> logic before and after the invocation of the LSM hook (static call) in
> the unrolled loop.
>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> security/security.c | 248 +++++++++++++++++++++++++-------------------
> 1 file changed, 144 insertions(+), 104 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index e0ec185cf125..4f0f35857217 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -948,10 +948,48 @@ out: \
> RC; \
> })
>
> -#define lsm_for_each_hook(scall, NAME) \
> - for (scall = static_calls_table.NAME; \
> - scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> - if (static_key_enabled(&scall->active->key))
> +/*
> + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> + * current hook
> + */
> +#define current_lsmid() _hook_lsmid
See my comments below about security_getselfattr(), I think we can drop
the current_lsmid() macro. If we really must keep it, we need to rename
it to something else as it clashes too much with the other current_XXX()
macros/functions which are useful outside of our wacky macros.
> +#define __CALL_HOOK(NUM, HOOK, RC, BLOCK_BEFORE, BLOCK_AFTER, ...) \
> +do { \
> + int __maybe_unused _hook_lsmid; \
> + \
> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> + _hook_lsmid = static_calls_table.HOOK[NUM].hl->lsmid->id; \
> + BLOCK_BEFORE \
> + RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> + BLOCK_AFTER \
> + } \
> +} while (0);
> +
> +#define lsm_for_each_hook(HOOK, RC, BLOCK_AFTER, ...) \
> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BLOCK_AFTER, __VA_ARGS__)
> +
> +#define call_hook_with_lsmid(HOOK, LSMID, ...) \
> +({ \
> + __label__ out; \
> + int RC = LSM_RET_DEFAULT(HOOK); \
> + \
> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, \
> + /* BLOCK BEFORE INVOCATION */ \
> + { \
> + if (current_lsmid() != LSMID) \
> + continue; \
> + }, \
> + /* END BLOCK BEFORE INVOCATION */ \
> + /* BLOCK AFTER INVOCATION */ \
> + { \
> + goto out; \
> + }, \
> + /* END BLOCK AFTER INVOCATION */ \
> + __VA_ARGS__); \
> +out: \
> + RC; \
> +})
>
> /* Security operations */
...
> @@ -1581,15 +1629,19 @@ int security_sb_set_mnt_opts(struct super_block *sb,
> unsigned long kern_flags,
> unsigned long *set_kern_flags)
> {
> - struct lsm_static_call *scall;
> int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
>
> - lsm_for_each_hook(scall, sb_set_mnt_opts) {
> - rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
> - set_kern_flags);
> - if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> - break;
> - }
> + lsm_for_each_hook(
> + sb_set_mnt_opts, rc,
> + /* BLOCK AFTER INVOCATION */
> + {
> + if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> + goto out;
> + },
> + /* END BLOCK AFTER INVOCATION */
> + sb, mnt_opts, kern_flags, set_kern_flags);
> +
> +out:
> return rc;
> }
> EXPORT_SYMBOL(security_sb_set_mnt_opts);
I know I was the one who asked to implement the static_calls for *all*
of the LSM functions - thank you for doing that - but I think we can
all agree that some of the resulting code is pretty awful. I'm probably
missing something important, but would an apporach similar to the pseudo
code below work?
#define call_int_hook_special(HOOK, RC, LABEL, ...) \
LSM_LOOP_UNROLL(HOOK##_SPECIAL, RC, HOOK, LABEL, __VA_ARGS__)
int security_sb_set_mnt_opts(...)
{
int rc = LSM_RET_DEFAULT(sb_set_mnt_opts);
#define sb_set_mnt_opts_SPECIAL \
do { \
if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts)) \
goto out; \
} while (0)
rc = call_int_hook_special(sb_set_mnt_opts, rc, out, ...);
out:
return rc;
}
> @@ -4040,7 +4099,6 @@ EXPORT_SYMBOL(security_d_instantiate);
> int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> u32 __user *size, u32 flags)
> {
> - struct lsm_static_call *scall;
> struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
> u8 __user *base = (u8 __user *)uctx;
> u32 entrysize;
> @@ -4078,31 +4136,42 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> * In the usual case gather all the data from the LSMs.
> * In the single case only get the data from the LSM specified.
> */
> - lsm_for_each_hook(scall, getselfattr) {
> - if (single && lctx.id != scall->hl->lsmid->id)
> - continue;
> - entrysize = left;
> - if (base)
> - uctx = (struct lsm_ctx __user *)(base + total);
> - rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
> - if (rc == -EOPNOTSUPP) {
> - rc = 0;
> - continue;
> - }
> - if (rc == -E2BIG) {
> - rc = 0;
> - left = 0;
> - toobig = true;
> - } else if (rc < 0)
> - return rc;
> - else
> - left -= entrysize;
> + LSM_LOOP_UNROLL(
> + __CALL_HOOK, getselfattr, rc,
> + /* BLOCK BEFORE INVOCATION */
> + {
> + if (single && lctx.id != current_lsmid())
> + continue;
> + entrysize = left;
> + if (base)
> + uctx = (struct lsm_ctx __user *)(base + total);
> + },
> + /* END BLOCK BEFORE INVOCATION */
> + /* BLOCK AFTER INVOCATION */
> + {
> + if (rc == -EOPNOTSUPP) {
> + rc = 0;
> + } else {
> + if (rc == -E2BIG) {
> + rc = 0;
> + left = 0;
> + toobig = true;
> + } else if (rc < 0)
> + return rc;
> + else
> + left -= entrysize;
> +
> + total += entrysize;
> + count += rc;
> + if (single)
> + goto out;
> + }
> + },
> + /* END BLOCK AFTER INVOCATION */
> + attr, uctx, &entrysize, flags);
> +
> +out:
>
> - total += entrysize;
> - count += rc;
> - if (single)
> - break;
> - }
> if (put_user(total, size))
> return -EFAULT;
> if (toobig)
I think we may need to admit defeat with security_getselfattr() and
leave it as-is, the above is just too ugly to live. I'd suggest
adding a comment explaining that it wasn't converted due to complexity
and the resulting awfulness.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached
2024-06-29 8:43 ` [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
@ 2024-07-03 0:07 ` Paul Moore
2024-07-03 16:55 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-03 0:07 UTC (permalink / raw)
To: KP Singh, linux-security-module, bpf
Cc: ast, casey, andrii, keescook, daniel, renauld, revest, song,
KP Singh
On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote:
>
> BPF LSM hooks have side-effects (even when a default value's 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:
> 0xff...0e30 <+0>: endbr64
> 0xff...0e34 <+4>: nopl 0x0(%rax,%rax,1)
> 0xff...0e39 <+9>: push %rbp
> 0xff...0e3a <+10>: push %r14
> 0xff...0e3c <+12>: push %rbx
> 0xff...0e3d <+13>: mov %rdx,%rbx
> 0xff...0e40 <+16>: mov %esi,%ebp
> 0xff...0e42 <+18>: mov %rdi,%r14
> 0xff...0e45 <+21>: jmp 0xff...0e57 <security_file_ioctl+39>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Static key enabled for SELinux
>
> 0xff...0e47 <+23>: xchg %ax,%ax
> ^^^^^^^^^^^^^^
>
> Static key disabled for BPF. This gets patched when a BPF LSM
> program is attached
>
> 0xff...0e49 <+25>: xor %eax,%eax
> 0xff...0e4b <+27>: xchg %ax,%ax
> 0xff...0e4d <+29>: pop %rbx
> 0xff...0e4e <+30>: pop %r14
> 0xff...0e50 <+32>: pop %rbp
> 0xff...0e51 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
> 0xff...0e57 <+39>: endbr64
> 0xff...0e5b <+43>: mov %r14,%rdi
> 0xff...0e5e <+46>: mov %ebp,%esi
> 0xff...0e60 <+48>: mov %rbx,%rdx
> 0xff...0e63 <+51>: call 0xff...33c0 <selinux_file_ioctl>
> 0xff...0e68 <+56>: test %eax,%eax
> 0xff...0e6a <+58>: jne 0xff...0e4d <security_file_ioctl+29>
> 0xff...0e6c <+60>: jmp 0xff...0e47 <security_file_ioctl+23>
> 0xff...0e6e <+62>: endbr64
> 0xff...0e72 <+66>: mov %r14,%rdi
> 0xff...0e75 <+69>: mov %ebp,%esi
> 0xff...0e77 <+71>: mov %rbx,%rdx
> 0xff...0e7a <+74>: call 0xff...e3b0 <bpf_lsm_file_ioctl>
> 0xff...0e7f <+79>: test %eax,%eax
> 0xff...0e81 <+81>: jne 0xff...0e4d <security_file_ioctl+29>
> 0xff...0e83 <+83>: jmp 0xff...0e49 <security_file_ioctl+25>
> 0xff...0e85 <+85>: endbr64
> 0xff...0e89 <+89>: mov %r14,%rdi
> 0xff...0e8c <+92>: mov %ebp,%esi
> 0xff...0e8e <+94>: mov %rbx,%rdx
> 0xff...0e91 <+97>: pop %rbx
> 0xff...0e92 <+98>: pop %r14
> 0xff...0e94 <+100>: pop %rbp
> 0xff...0e95 <+101>: ret
>
> This patch enables this by providing a LSM_HOOK_INIT_RUNTIME variant
> that allows the LSMs to opt-in to hooks which can be toggled at runtime
> which with security_toogle_hook.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: KP Singh <kpsingh@kernel.org>
> ---
> include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++-
> kernel/bpf/trampoline.c | 40 +++++++++++++++++++++++++++++++++++----
> security/bpf/hooks.c | 2 +-
> security/security.c | 36 ++++++++++++++++++++++++++++++++++-
> 4 files changed, 101 insertions(+), 7 deletions(-)
I didn't look at this one too closely, see my previous comments in
patch 3/5, but I did catch one typo, see below ...
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index a66ca68485a2..dbe0f40f7f67 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -110,11 +110,14 @@ struct lsm_id {
> * @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 struct lsm_id *lsmid;
> + bool runtime;
> } __randomize_layout;
The comment header doesn't match the struct fields, "default_state" vs
"runtime".
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 2/5] security: Count the LSMs enabled at compile time
2024-06-29 8:43 ` [PATCH v13 2/5] security: Count the LSMs enabled at compile time KP Singh
@ 2024-07-03 9:44 ` Rasmus Villemoes
2024-07-03 13:12 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Rasmus Villemoes @ 2024-07-03 9:44 UTC (permalink / raw)
To: linux-security-module, bpf
Cc: ast, paul, casey, andrii, keescook, daniel, renauld, revest, song,
Kui-Feng Lee, KP Singh
KP Singh <kpsingh@kernel.org> writes:
> These macros are a clever trick to determine a count of the number of
> LSMs that are enabled in the config to ascertain the maximum number of
> static calls that need to be configured per LSM hook.
>
> Without this one would need to generate static calls for the total
> number of LSMs in the kernel (even if they are not compiled) times the
> number of LSM hooks which ends up being quite wasteful.
[snip]
> diff --git a/include/linux/lsm_count.h b/include/linux/lsm_count.h
> new file mode 100644
> index 000000000000..73c7cc81349b
> --- /dev/null
> +++ b/include/linux/lsm_count.h
> @@ -0,0 +1,128 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Copyright (C) 2023 Google LLC.
> + */
> +
> +#ifndef __LINUX_LSM_COUNT_H
> +#define __LINUX_LSM_COUNT_H
> +
> +#include <linux/args.h>
> +
> +#ifdef CONFIG_SECURITY
> +
> +/*
> + * Macros to count the number of LSMs enabled in the kernel at compile time.
> + */
> +
> +/*
> + * Capabilities is enabled when CONFIG_SECURITY is enabled.
> + */
> +#if IS_ENABLED(CONFIG_SECURITY)
> +#define CAPABILITIES_ENABLED 1,
> +#else
> +#define CAPABILITIES_ENABLED
> +#endif
> +
> +#if IS_ENABLED(CONFIG_SECURITY_SELINUX)
> +#define SELINUX_ENABLED 1,
> +#else
> +#define SELINUX_ENABLED
> +#endif
> +
[snip]
> +
> +#if IS_ENABLED(CONFIG_EVM)
> +#define EVM_ENABLED 1,
> +#else
> +#define EVM_ENABLED
> +#endif
> +
> +/*
> + * There is a trailing comma that we need to be accounted for. This is done by
> + * using a skipped argument in __COUNT_LSMS
> + */
> +#define __COUNT_LSMS(skipped_arg, args...) COUNT_ARGS(args...)
> +#define COUNT_LSMS(args...) __COUNT_LSMS(args)
> +
> +#define MAX_LSM_COUNT \
> + COUNT_LSMS( \
> + CAPABILITIES_ENABLED \
> + SELINUX_ENABLED \
> + SMACK_ENABLED \
> + APPARMOR_ENABLED \
> + TOMOYO_ENABLED \
> + YAMA_ENABLED \
> + LOADPIN_ENABLED \
> + LOCKDOWN_ENABLED \
> + SAFESETID_ENABLED \
> + BPF_LSM_ENABLED \
> + LANDLOCK_ENABLED \
> + IMA_ENABLED \
> + EVM_ENABLED)
> +
> +#else
> +
> +#define MAX_LSM_COUNT 0
> +
> +#endif /* CONFIG_SECURITY */
> +
> +#endif /* __LINUX_LSM_COUNT_H */
OK, so I can tell from the other patches that this isn't just about
getting MAX_LSM_COUNT to be a compile-time constant, it really has to be
a single preprocessor token representing the right decimal value. That
information could have been in some comment or the commit log. So
#define MAX_LSM_COUNT (IS_ENABLED(CONFIG_SECURITY) + IS_ENABLED(CONFIG_SECURITY_SELINUX) + ...)
doesn't work immediately. But this does provide not just a compile-time
constant, but a preprocessor constant, so:
Instead of all this trickery with defining temporary, never used again,
macros expanding to something with trailing comma or not, what about
this simpler (at least in terms of LOC, but IMO also readability)
approach:
/*
* The sum of the IS_ENABLED() values provides the right value, but we
* need MAX_LSM_COUNT to be a single preprocessor token representing
* that value, because it will be passed to the UNROLL macro which
* does token concatenation.
*/
#define __MAX_LSM_COUNT (\
IS_ENABLED(CONFIG_SECURITY) /* capabilities */ + \
IS_ENABLED(CONFIG_SECURITY_SELINUX) + \
... \
IS_ENABLED(CONFIG_EVM) \
)
#if __MAX_LSM_COUNT == 0
#define MAX_LSM_COUNT 0
#elif __MAX_LSM_COUNT == 1
#define MAX_LSM_COUNT 1
#elif
...
#elif __MAX_LSM_COUNT == 15
#define MAX_LSM_COUNT 15
#else
#error "Too many LSMs, add an #elif case"
#endif
Rasmus
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 2/5] security: Count the LSMs enabled at compile time
2024-07-03 9:44 ` Rasmus Villemoes
@ 2024-07-03 13:12 ` KP Singh
2024-07-03 14:54 ` Paul Moore
0 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-07-03 13:12 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: linux-security-module, bpf, Alexei Starovoitov, Paul Moore,
Casey Schaufler, andrii, keescook, daniel, renauld, revest, song,
Kui-Feng Lee
> On 3 Jul 2024, at 11:44, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
>
> KP Singh <kpsingh@kernel.org> writes:
>
>> These macros are a clever trick to determine a count of the number of
>> LSMs that are enabled in the config to ascertain the maximum number of
>> static calls that need to be configured per LSM hook.
>>
>> Without this one would need to generate static calls for the total
>> number of LSMs in the kernel (even if they are not compiled) times the
>> number of LSM hooks which ends up being quite wasteful.
>
> [snip]
>
>> diff --git a/include/linux/lsm_count.h b/include/linux/lsm_count.h
>> new file mode 100644
>> index 000000000000..73c7cc81349b
>> --- /dev/null
>> +++ b/include/linux/lsm_count.h
>> @@ -0,0 +1,128 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +/*
>> + * Copyright (C) 2023 Google LLC.
>> + */
>> +
>> +#ifndef __LINUX_LSM_COUNT_H
>> +#define __LINUX_LSM_COUNT_H
>> +
>> +#include <linux/args.h>
>> +
>> +#ifdef CONFIG_SECURITY
>> +
>> +/*
>> + * Macros to count the number of LSMs enabled in the kernel at compile time.
>> + */
>> +
>> +/*
>> + * Capabilities is enabled when CONFIG_SECURITY is enabled.
>> + */
>> +#if IS_ENABLED(CONFIG_SECURITY)
>> +#define CAPABILITIES_ENABLED 1,
>> +#else
>> +#define CAPABILITIES_ENABLED
>> +#endif
>> +
>> +#if IS_ENABLED(CONFIG_SECURITY_SELINUX)
>> +#define SELINUX_ENABLED 1,
>> +#else
>> +#define SELINUX_ENABLED
>> +#endif
>> +
> [snip]
>> +
>> +#if IS_ENABLED(CONFIG_EVM)
>> +#define EVM_ENABLED 1,
>> +#else
>> +#define EVM_ENABLED
>> +#endif
>> +
>> +/*
>> + * There is a trailing comma that we need to be accounted for. This is done by
>> + * using a skipped argument in __COUNT_LSMS
>> + */
>> +#define __COUNT_LSMS(skipped_arg, args...) COUNT_ARGS(args...)
>> +#define COUNT_LSMS(args...) __COUNT_LSMS(args)
>> +
>> +#define MAX_LSM_COUNT \
>> + COUNT_LSMS( \
>> + CAPABILITIES_ENABLED \
>> + SELINUX_ENABLED \
>> + SMACK_ENABLED \
>> + APPARMOR_ENABLED \
>> + TOMOYO_ENABLED \
>> + YAMA_ENABLED \
>> + LOADPIN_ENABLED \
>> + LOCKDOWN_ENABLED \
>> + SAFESETID_ENABLED \
>> + BPF_LSM_ENABLED \
>> + LANDLOCK_ENABLED \
>> + IMA_ENABLED \
>> + EVM_ENABLED)
>> +
>> +#else
>> +
>> +#define MAX_LSM_COUNT 0
>> +
>> +#endif /* CONFIG_SECURITY */
>> +
>> +#endif /* __LINUX_LSM_COUNT_H */
>
> OK, so I can tell from the other patches that this isn't just about
> getting MAX_LSM_COUNT to be a compile-time constant, it really has to be
> a single preprocessor token representing the right decimal value. That
> information could have been in some comment or the commit log. So
>
> #define MAX_LSM_COUNT (IS_ENABLED(CONFIG_SECURITY) + IS_ENABLED(CONFIG_SECURITY_SELINUX) + ...)
>
> doesn't work immediately. But this does provide not just a compile-time
> constant, but a preprocessor constant, so:
>
> Instead of all this trickery with defining temporary, never used again,
> macros expanding to something with trailing comma or not, what about
> this simpler (at least in terms of LOC, but IMO also readability)
> approach:
>
> /*
> * The sum of the IS_ENABLED() values provides the right value, but we
> * need MAX_LSM_COUNT to be a single preprocessor token representing
> * that value, because it will be passed to the UNROLL macro which
> * does token concatenation.
> */
>
I actually prefer the version we have now from a readability perspective, it makes it more explicit (the check about the CONFIG_* being enabled and counting them). let's keep this as an incremental change that you can propose :) once the patches are merged.
- KP
> #define __MAX_LSM_COUNT (\
> IS_ENABLED(CONFIG_SECURITY) /* capabilities */ + \
> IS_ENABLED(CONFIG_SECURITY_SELINUX) + \
> ... \
> IS_ENABLED(CONFIG_EVM) \
> )
> #if __MAX_LSM_COUNT == 0
> #define MAX_LSM_COUNT 0
> #elif __MAX_LSM_COUNT == 1
> #define MAX_LSM_COUNT 1
> #elif
> ...
> #elif __MAX_LSM_COUNT == 15
> #define MAX_LSM_COUNT 15
> #else
> #error "Too many LSMs, add an #elif case"
> #endif
>
> Rasmus
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 2/5] security: Count the LSMs enabled at compile time
2024-07-03 13:12 ` KP Singh
@ 2024-07-03 14:54 ` Paul Moore
0 siblings, 0 replies; 34+ messages in thread
From: Paul Moore @ 2024-07-03 14:54 UTC (permalink / raw)
To: KP Singh
Cc: Rasmus Villemoes, linux-security-module, bpf, Alexei Starovoitov,
Casey Schaufler, andrii, keescook, daniel, renauld, revest, song,
Kui-Feng Lee
On Wed, Jul 3, 2024 at 9:12 AM KP Singh <kpsingh@kernel.org> wrote:
> > On 3 Jul 2024, at 11:44, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote:
> > KP Singh <kpsingh@kernel.org> writes:
> >
> >> These macros are a clever trick to determine a count of the number of
> >> LSMs that are enabled in the config to ascertain the maximum number of
> >> static calls that need to be configured per LSM hook.
...
> > Instead of all this trickery with defining temporary, never used again,
> > macros expanding to something with trailing comma or not, what about
> > this simpler (at least in terms of LOC, but IMO also readability)
> > approach:
...
> I actually prefer the version we have now from a readability perspective, it makes it more explicit (the check about the CONFIG_* being enabled and counting them). let's keep this as an incremental change that you can propose :) once the patches are merged.
I prefer the original approach by KP as well, let's leave it as-is.
IMO, it's far from the worst of the macro shenanigans in this patchset
(or existing LSM code for that matter).
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 0:07 ` Paul Moore
@ 2024-07-03 16:54 ` KP Singh
2024-07-03 20:56 ` Paul Moore
0 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-07-03 16:54 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
[...]
> should fix the more obvious problems. I'd like to know if you are
> aware of any others? If not, the text above should be adjusted and
> we should reconsider patch 5/5. If there are other problems I'd
> like to better understand them as there may be an independent
> solution for those particular problems.
We did have problems with some other hooks but I was unable to dig up
specific examples though, it's been a while. More broadly speaking, a
default decision is still a decision. Whereas the intent from the BPF
LSM is not to make a default decision unless a BPF program is loaded.
I am quite worried about the holes this leaves open, subtle bugs
(security or crashes) we have not caught yet and PATCH 5/5 engineers away
the problem of the "default decision".
>
> > 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:
> > 0xff...0ca0 <+0>: endbr64
> > 0xff...0ca4 <+4>: nopl 0x0(%rax,%rax,1)
> > 0xff...0ca9 <+9>: push %rbp
> > 0xff...0caa <+10>: push %r14
> > 0xff...0cac <+12>: push %rbx
> > 0xff...0cad <+13>: mov %rdx,%rbx
> > 0xff...0cb0 <+16>: mov %esi,%ebp
> > 0xff...0cb2 <+18>: mov %rdi,%r14
> > 0xff...0cb5 <+21>: jmp 0xff...0cc7 <security_file_ioctl+39>
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Static key enabled for SELinux
> >
> > 0xffffffff818f0cb7 <+23>: jmp 0xff...0cde <security_file_ioctl+62>
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > Static key enabled for BPF LSM. This is something that is changed to
> > default to false to avoid the existing side effect issues of BPF LSM
> > [1] in a subsequent patch.
> >
> > 0xff...0cb9 <+25>: xor %eax,%eax
> > 0xff...0cbb <+27>: xchg %ax,%ax
> > 0xff...0cbd <+29>: pop %rbx
> > 0xff...0cbe <+30>: pop %r14
> > 0xff...0cc0 <+32>: pop %rbp
> > 0xff...0cc1 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
> > 0xff...0cc7 <+39>: endbr64
> > 0xff...0ccb <+43>: mov %r14,%rdi
> > 0xff...0cce <+46>: mov %ebp,%esi
> > 0xff...0cd0 <+48>: mov %rbx,%rdx
> > 0xff...0cd3 <+51>: call 0xff...3230 <selinux_file_ioctl>
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Direct call to SELinux.
> >
> > 0xff...0cd8 <+56>: test %eax,%eax
> > 0xff...0cda <+58>: jne 0xff...0cbd <security_file_ioctl+29>
> > 0xff...0cdc <+60>: jmp 0xff...0cb7 <security_file_ioctl+23>
> > 0xff...0cde <+62>: endbr64
> > 0xff...0ce2 <+66>: mov %r14,%rdi
> > 0xff...0ce5 <+69>: mov %ebp,%esi
> > 0xff...0ce7 <+71>: mov %rbx,%rdx
> > 0xff...0cea <+74>: call 0xff...e220 <bpf_lsm_file_ioctl>
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Direct call to BPF LSM.
> >
> > 0xff...0cef <+79>: test %eax,%eax
> > 0xff...0cf1 <+81>: jne 0xff...0cbd <security_file_ioctl+29>
> > 0xff...0cf3 <+83>: jmp 0xff...0cb9 <security_file_ioctl+25>
> > 0xff...0cf5 <+85>: endbr64
> > 0xff...0cf9 <+89>: mov %r14,%rdi
> > 0xff...0cfc <+92>: mov %ebp,%esi
> > 0xff...0cfe <+94>: mov %rbx,%rdx
> > 0xff...0d01 <+97>: pop %rbx
> > 0xff...0d02 <+98>: pop %r14
> > 0xff...0d04 <+100>: pop %rbp
> > 0xff...0d05 <+101>: ret
> > 0xff...0d06 <+102>: int3
> > 0xff...0d07 <+103>: int3
> > 0xff...0d08 <+104>: int3
> > 0xff...0d09 <+105>: int3
> >
> > While this patch uses static_branch_unlikely indicating that an LSM hook
> > is likely to be not present. In most cases this is still a better choice
> > as even when an LSM with one hook is added, empty slots are created for
> > all LSM hooks (especially when many LSMs that do not initialize most
> > hooks are present on the system).
> >
> > There are some hooks that don't use the call_int_hook and
> > call_void_hook.
>
> I think you mean "or" and not "and", yes?
Yep, thanks, fixed.
>
> > These hooks are updated to use a new macro called
> > lsm_for_each_hook where the lsm_callback is directly invoked as an
> > indirect call. These are updated in a subsequent patch to also use
> > static calls.
> >
> > Below are results of the relevant Unixbench system benchmarks with BPF LSM
> > and SELinux enabled with default policies enabled with and without these
> > patches.
> >
> > Benchmark Delta(%): (+ is better)
> > ===============================================================================
> > Execl Throughput +1.9356
> > File Write 1024 bufsize 2000 maxblocks +6.5953
> > Pipe Throughput +9.5499
> > Pipe-based Context Switching +3.0209
> > Process Creation +2.3246
> > Shell Scripts (1 concurrent) +1.4975
> > System Call Overhead +2.7815
> > System Benchmarks Index Score (Partial Only): +3.4859
> >
> > In the best case, some syscalls like eventfd_create benefitted to about ~10%.
> >
> > [1] https://lore.kernel.org/linux-security-module/20220609234601.2026362-1-kpsingh@kernel.org/
> >
> > Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Acked-by: Song Liu <song@kernel.org>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> > include/linux/lsm_hooks.h | 72 ++++++++++++--
> > security/security.c | 198 ++++++++++++++++++++++++++------------
> > 2 files changed, 197 insertions(+), 73 deletions(-)
>
> ...
>
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index efd4a0655159..a66ca68485a2 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -30,19 +30,66 @@
> > #include <linux/init.h>
> > #include <linux/rculist.h>
> > #include <linux/xattr.h>
> > +#include <linux/static_call.h>
> > +#include <linux/unroll.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/lsm_count.h>
> > +
> > +#define SECURITY_HOOK_ACTIVE_KEY(HOOK, IDX) security_hook_active_##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_LOOP_UNROLL(M, ...) \
> > +do { \
> > + UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__) \
> > +} while (0)
> > +
> > +#define LSM_DEFINE_UNROLL(M, ...) UNROLL(MAX_LSM_COUNT, M, __VA_ARGS__)
>
> All of the macros above (SECURITY_HOOK_ACTIVE_KEY, LSM_STATIC_CALL,
> LSM_LOOP_UNROLL, and LSM_DEFINE_UNROLL) are only used in
> security/security.c, yes? If so, is there a reason why they are
> defined here and not in security/security.c?
Fair point, fixed.
>
> > diff --git a/security/security.c b/security/security.c
> > index 9c3fb2f60e2a..e0ec185cf125 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -112,6 +113,51 @@ static __initconst const char *const builtin_lsm_order = CONFIG_LSM;
> > static __initdata struct lsm_info **ordered_lsms;
> > static __initdata struct lsm_info *exclusive;
> >
> > +
> > +#ifdef CONFIG_HAVE_STATIC_CALL
> > +#define LSM_HOOK_TRAMP(NAME, NUM) \
> > + &STATIC_CALL_TRAMP(LSM_STATIC_CALL(NAME, NUM))
> > +#else
> > +#define LSM_HOOK_TRAMP(NAME, NUM) NULL
> > +#endif
> > +
> > +/*
> > + * 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_ACTIVE_KEY(NAME, NUM));
> > +
> > +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> > + LSM_DEFINE_UNROLL(DEFINE_LSM_STATIC_CALL, NAME, RET, __VA_ARGS__)
> > +#include <linux/lsm_hook_defs.h>
> > +#undef LSM_HOOK
> > +#undef DEFINE_LSM_STATIC_CALL
>
> If you end up respinning the patchset, drop the two blank lines
> before the DEFINE_LSM_STATIC_CALL and LSM_HOOK definitions.
Done.
>
> > @@ -856,29 +916,43 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, u32 *uctx_len,
> > * call_int_hook:
> > * This is a hook that returns a value.
> > */
> > +#define __CALL_STATIC_VOID(NUM, HOOK, ...) \
> > +do { \
> > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> > + static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> > + } \
> > +} while (0);
> >
> > -#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(HOOK, ...) \
> > + do { \
> > + LSM_LOOP_UNROLL(__CALL_STATIC_VOID, HOOK, __VA_ARGS__); \
> > } while (0)
> >
> > -#define call_int_hook(FUNC, ...) ({ \
> > - int RC = LSM_RET_DEFAULT(FUNC); \
> > - do { \
> > - struct security_hook_list *P; \
> > - \
> > - hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > - RC = P->hook.FUNC(__VA_ARGS__); \
> > - if (RC != LSM_RET_DEFAULT(FUNC)) \
> > - break; \
> > - } \
> > - } while (0); \
> > - RC; \
> > +
> > +#define __CALL_STATIC_INT(NUM, R, HOOK, LABEL, ...) \
> > +do { \
> > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> > + R = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> > + if (R != LSM_RET_DEFAULT(HOOK)) \
> > + goto LABEL; \
> > + } \
> > +} while (0);
> > +
> > +#define call_int_hook(HOOK, ...) \
> > +({ \
> > + __label__ out; \
>
> This is another only-if-you-respin, consider /out/OUT/ so it is more
> consistent.
Done. I will do a re-spin as we do have some fixes.
>
> > + int RC = LSM_RET_DEFAULT(HOOK); \
> > + \
> > + LSM_LOOP_UNROLL(__CALL_STATIC_INT, RC, HOOK, out, __VA_ARGS__); \
> > +out: \
> > + RC; \
> > })
> >
> > +#define lsm_for_each_hook(scall, NAME) \
> > + for (scall = static_calls_table.NAME; \
> > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> > + if (static_key_enabled(&scall->active->key))
>
> This is probably a stupid question, but why use static_key_enabled()
> here instead of static_branch_unlikely() as in the call_XXX_macros?
The static_key_enabled is a check for the key being enabled, whereas
the static_branch_likely is for laying out the right assembly code
(jump tables etc.) and based on the value of the static key, here we
are not using the static calls or even keys, rather we are following
back from direct calls to indirect calls and don't really need any
jump tables in the slow path.
We also get rid of this macro in the
subsequent patch, but we might need to keep it around if we leave
security_getselfattr alone.
- KP
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached
2024-07-03 0:07 ` Paul Moore
@ 2024-07-03 16:55 ` KP Singh
0 siblings, 0 replies; 34+ messages in thread
From: KP Singh @ 2024-07-03 16:55 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> wrote:
> >
> > BPF LSM hooks have side-effects (even when a default value's 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:
> > 0xff...0e30 <+0>: endbr64
> > 0xff...0e34 <+4>: nopl 0x0(%rax,%rax,1)
> > 0xff...0e39 <+9>: push %rbp
> > 0xff...0e3a <+10>: push %r14
> > 0xff...0e3c <+12>: push %rbx
> > 0xff...0e3d <+13>: mov %rdx,%rbx
> > 0xff...0e40 <+16>: mov %esi,%ebp
> > 0xff...0e42 <+18>: mov %rdi,%r14
> > 0xff...0e45 <+21>: jmp 0xff...0e57 <security_file_ioctl+39>
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > Static key enabled for SELinux
> >
> > 0xff...0e47 <+23>: xchg %ax,%ax
> > ^^^^^^^^^^^^^^
> >
> > Static key disabled for BPF. This gets patched when a BPF LSM
> > program is attached
> >
> > 0xff...0e49 <+25>: xor %eax,%eax
> > 0xff...0e4b <+27>: xchg %ax,%ax
> > 0xff...0e4d <+29>: pop %rbx
> > 0xff...0e4e <+30>: pop %r14
> > 0xff...0e50 <+32>: pop %rbp
> > 0xff...0e51 <+33>: cs jmp 0xff...0000 <__x86_return_thunk>
> > 0xff...0e57 <+39>: endbr64
> > 0xff...0e5b <+43>: mov %r14,%rdi
> > 0xff...0e5e <+46>: mov %ebp,%esi
> > 0xff...0e60 <+48>: mov %rbx,%rdx
> > 0xff...0e63 <+51>: call 0xff...33c0 <selinux_file_ioctl>
> > 0xff...0e68 <+56>: test %eax,%eax
> > 0xff...0e6a <+58>: jne 0xff...0e4d <security_file_ioctl+29>
> > 0xff...0e6c <+60>: jmp 0xff...0e47 <security_file_ioctl+23>
> > 0xff...0e6e <+62>: endbr64
> > 0xff...0e72 <+66>: mov %r14,%rdi
> > 0xff...0e75 <+69>: mov %ebp,%esi
> > 0xff...0e77 <+71>: mov %rbx,%rdx
> > 0xff...0e7a <+74>: call 0xff...e3b0 <bpf_lsm_file_ioctl>
> > 0xff...0e7f <+79>: test %eax,%eax
> > 0xff...0e81 <+81>: jne 0xff...0e4d <security_file_ioctl+29>
> > 0xff...0e83 <+83>: jmp 0xff...0e49 <security_file_ioctl+25>
> > 0xff...0e85 <+85>: endbr64
> > 0xff...0e89 <+89>: mov %r14,%rdi
> > 0xff...0e8c <+92>: mov %ebp,%esi
> > 0xff...0e8e <+94>: mov %rbx,%rdx
> > 0xff...0e91 <+97>: pop %rbx
> > 0xff...0e92 <+98>: pop %r14
> > 0xff...0e94 <+100>: pop %rbp
> > 0xff...0e95 <+101>: ret
> >
> > This patch enables this by providing a LSM_HOOK_INIT_RUNTIME variant
> > that allows the LSMs to opt-in to hooks which can be toggled at runtime
> > which with security_toogle_hook.
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> > include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++-
> > kernel/bpf/trampoline.c | 40 +++++++++++++++++++++++++++++++++++----
> > security/bpf/hooks.c | 2 +-
> > security/security.c | 36 ++++++++++++++++++++++++++++++++++-
> > 4 files changed, 101 insertions(+), 7 deletions(-)
>
> I didn't look at this one too closely, see my previous comments in
> patch 3/5, but I did catch one typo, see below ...
>
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index a66ca68485a2..dbe0f40f7f67 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -110,11 +110,14 @@ struct lsm_id {
> > * @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 struct lsm_id *lsmid;
> > + bool runtime;
> > } __randomize_layout;
>
> The comment header doesn't match the struct fields, "default_state" vs
> "runtime".
Good catch, apologies for the omission.
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 16:54 ` KP Singh
@ 2024-07-03 20:56 ` Paul Moore
2024-07-03 22:22 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-03 20:56 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
>
> [...]
>
> > should fix the more obvious problems. I'd like to know if you are
> > aware of any others? If not, the text above should be adjusted and
> > we should reconsider patch 5/5. If there are other problems I'd
> > like to better understand them as there may be an independent
> > solution for those particular problems.
>
> We did have problems with some other hooks but I was unable to dig up
> specific examples though, it's been a while. More broadly speaking, a
> default decision is still a decision. Whereas the intent from the BPF
> LSM is not to make a default decision unless a BPF program is loaded.
> I am quite worried about the holes this leaves open, subtle bugs
> (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> the problem of the "default decision".
The inode/xattr problem you originally mentioned wasn't really rooted
in a "bad" default return value, it was really an issue with how the
LSM hook was structured due to some legacy design assumptions made
well before the initial stacking patches were merged. That should be
fixed now[1] and given that the inode/xattr set/remove hooks were
unique in this regard (individual LSMs were responsible for performing
the capabilities checks) I don't expect this to be a general problem.
There were also some issues caused by the fact that we were defining
the default return value in multiple places and these values had gone
out of sync in a number of hooks. We've also fixed this problem by
only defining the default return value once for each hook, solving all
of those problems.
I'm not aware of any other existing problems relating to the LSM hook
default values, if there are any, we need to fix them independent of
this patchset. The LSM framework should function properly if the
"default" values are used.
[1] I just realized that commit 61df7b828204 doesn't properly update
the removexattr implementations for SELinux and Smack, expect a patch
for that soon. The current code is okay, it just does some
unnecessary work (see the setxattr changes to get an idea of the
changes needed).
> > > +#define lsm_for_each_hook(scall, NAME) \
> > > + for (scall = static_calls_table.NAME; \
> > > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> > > + if (static_key_enabled(&scall->active->key))
> >
> > This is probably a stupid question, but why use static_key_enabled()
> > here instead of static_branch_unlikely() as in the call_XXX_macros?
>
> The static_key_enabled is a check for the key being enabled, whereas
> the static_branch_likely is for laying out the right assembly code
> (jump tables etc.) and based on the value of the static key, here we
> are not using the static calls or even keys, rather we are following
> back from direct calls to indirect calls and don't really need any
> jump tables in the slow path.
Gotcha, thanks for the explanation.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 20:56 ` Paul Moore
@ 2024-07-03 22:22 ` KP Singh
2024-07-03 22:52 ` Paul Moore
0 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-07-03 22:22 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> >
> > [...]
> >
> > > should fix the more obvious problems. I'd like to know if you are
> > > aware of any others? If not, the text above should be adjusted and
> > > we should reconsider patch 5/5. If there are other problems I'd
> > > like to better understand them as there may be an independent
> > > solution for those particular problems.
> >
> > We did have problems with some other hooks but I was unable to dig up
> > specific examples though, it's been a while. More broadly speaking, a
> > default decision is still a decision. Whereas the intent from the BPF
> > LSM is not to make a default decision unless a BPF program is loaded.
> > I am quite worried about the holes this leaves open, subtle bugs
> > (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> > the problem of the "default decision".
>
> The inode/xattr problem you originally mentioned wasn't really rooted
> in a "bad" default return value, it was really an issue with how the
> LSM hook was structured due to some legacy design assumptions made
> well before the initial stacking patches were merged. That should be
> fixed now[1] and given that the inode/xattr set/remove hooks were
> unique in this regard (individual LSMs were responsible for performing
> the capabilities checks) I don't expect this to be a general problem.
>
> There were also some issues caused by the fact that we were defining
> the default return value in multiple places and these values had gone
> out of sync in a number of hooks. We've also fixed this problem by
> only defining the default return value once for each hook, solving all
> of those problems.
I don't see how this solves problems or prevents any future problems
with side-effects. I have always been uncomfortable with an extraneous
function being called with a side effect ever since we merged BPF LSM
with default callback. We have found one bug due to this, not all the
bugs.
>
> I'm not aware of any other existing problems relating to the LSM hook
> default values, if there are any, we need to fix them independent of
> this patchset. The LSM framework should function properly if the
> "default" values are used.
Patch 5 eliminates the possibilities of errors and subtle bugs all
together. The problem with subtle bugs is, well, they are subtle, if
you and I knew of the bugs, we would fix all of them, but we don't. I
really feel we ought to eliminate the class of issues and not just
whack-a-mole when we see the bugs.
The LSM framework ought to function with default values is a nice
goal, but a weaker position than what we have where we make these
subtle bugs impossible. If you feel this should not be a part of the
framework, I would still like to revert back to implementation where
we kept the toggling logic contained to the BPF LSM.
- KP
>
> [1] I just realized that commit 61df7b828204 doesn't properly update
> the removexattr implementations for SELinux and Smack, expect a patch
> for that soon. The current code is okay, it just does some
> unnecessary work (see the setxattr changes to get an idea of the
> changes needed).
>
> > > > +#define lsm_for_each_hook(scall, NAME) \
> > > > + for (scall = static_calls_table.NAME; \
> > > > + scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> > > > + if (static_key_enabled(&scall->active->key))
> > >
> > > This is probably a stupid question, but why use static_key_enabled()
> > > here instead of static_branch_unlikely() as in the call_XXX_macros?
> >
> > The static_key_enabled is a check for the key being enabled, whereas
> > the static_branch_likely is for laying out the right assembly code
> > (jump tables etc.) and based on the value of the static key, here we
> > are not using the static calls or even keys, rather we are following
> > back from direct calls to indirect calls and don't really need any
> > jump tables in the slow path.
>
> Gotcha, thanks for the explanation.
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 22:22 ` KP Singh
@ 2024-07-03 22:52 ` Paul Moore
2024-07-03 23:08 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-03 22:52 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> > >
> > > [...]
> > >
> > > > should fix the more obvious problems. I'd like to know if you are
> > > > aware of any others? If not, the text above should be adjusted and
> > > > we should reconsider patch 5/5. If there are other problems I'd
> > > > like to better understand them as there may be an independent
> > > > solution for those particular problems.
> > >
> > > We did have problems with some other hooks but I was unable to dig up
> > > specific examples though, it's been a while. More broadly speaking, a
> > > default decision is still a decision. Whereas the intent from the BPF
> > > LSM is not to make a default decision unless a BPF program is loaded.
> > > I am quite worried about the holes this leaves open, subtle bugs
> > > (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> > > the problem of the "default decision".
> >
> > The inode/xattr problem you originally mentioned wasn't really rooted
> > in a "bad" default return value, it was really an issue with how the
> > LSM hook was structured due to some legacy design assumptions made
> > well before the initial stacking patches were merged. That should be
> > fixed now[1] and given that the inode/xattr set/remove hooks were
> > unique in this regard (individual LSMs were responsible for performing
> > the capabilities checks) I don't expect this to be a general problem.
> >
> > There were also some issues caused by the fact that we were defining
> > the default return value in multiple places and these values had gone
> > out of sync in a number of hooks. We've also fixed this problem by
> > only defining the default return value once for each hook, solving all
> > of those problems.
>
> I don't see how this solves problems or prevents any future problems
> with side-effects. I have always been uncomfortable with an extraneous
> function being called with a side effect ever since we merged BPF LSM
> with default callback. We have found one bug due to this, not all the
> bugs.
You've got to give me something more concrete than that. If you can't
provide any concrete examples, start with providing a basic concept
with far more detail than just "side-effects".
> > I'm not aware of any other existing problems relating to the LSM hook
> > default values, if there are any, we need to fix them independent of
> > this patchset. The LSM framework should function properly if the
> > "default" values are used.
>
> Patch 5 eliminates the possibilities of errors and subtle bugs all
> together. The problem with subtle bugs is, well, they are subtle, if
> you and I knew of the bugs, we would fix all of them, but we don't. I
> really feel we ought to eliminate the class of issues and not just
> whack-a-mole when we see the bugs.
Here's the thing, I don't really like patch 5/5. To be honest, I
don't really like a lot of this patchset. From my perspective, the
complexity of the code is likely going to mean more maintenance
headaches down the road, but Linus hath spoken so we're doing this
(although "this" is still a bit undefined as far as I'm concerned).
If you want me to merge patch 5/5 you've got to give me something real
and convincing that can't be fixed by any other means. My current
opinion is that you're trying to use a previously fixed bug to scare
and/or coerce the merging of some changes I don't really want to
merge. If you want me to take patch 5/5, you've got to give me a
reason that is far more compelling that what you've written thus far.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 22:52 ` Paul Moore
@ 2024-07-03 23:08 ` KP Singh
2024-07-03 23:44 ` Casey Schaufler
2024-07-05 18:07 ` Paul Moore
0 siblings, 2 replies; 34+ messages in thread
From: KP Singh @ 2024-07-03 23:08 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > >
> > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> > > >
> > > > [...]
> > > >
> > > > > should fix the more obvious problems. I'd like to know if you are
> > > > > aware of any others? If not, the text above should be adjusted and
> > > > > we should reconsider patch 5/5. If there are other problems I'd
> > > > > like to better understand them as there may be an independent
> > > > > solution for those particular problems.
> > > >
> > > > We did have problems with some other hooks but I was unable to dig up
> > > > specific examples though, it's been a while. More broadly speaking, a
> > > > default decision is still a decision. Whereas the intent from the BPF
> > > > LSM is not to make a default decision unless a BPF program is loaded.
> > > > I am quite worried about the holes this leaves open, subtle bugs
> > > > (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> > > > the problem of the "default decision".
> > >
> > > The inode/xattr problem you originally mentioned wasn't really rooted
> > > in a "bad" default return value, it was really an issue with how the
> > > LSM hook was structured due to some legacy design assumptions made
> > > well before the initial stacking patches were merged. That should be
> > > fixed now[1] and given that the inode/xattr set/remove hooks were
> > > unique in this regard (individual LSMs were responsible for performing
> > > the capabilities checks) I don't expect this to be a general problem.
> > >
> > > There were also some issues caused by the fact that we were defining
> > > the default return value in multiple places and these values had gone
> > > out of sync in a number of hooks. We've also fixed this problem by
> > > only defining the default return value once for each hook, solving all
> > > of those problems.
> >
> > I don't see how this solves problems or prevents any future problems
> > with side-effects. I have always been uncomfortable with an extraneous
> > function being called with a side effect ever since we merged BPF LSM
> > with default callback. We have found one bug due to this, not all the
> > bugs.
>
> You've got to give me something more concrete than that. If you can't
> provide any concrete examples, start with providing a basic concept
> with far more detail than just "side-effects".
>
> > > I'm not aware of any other existing problems relating to the LSM hook
> > > default values, if there are any, we need to fix them independent of
> > > this patchset. The LSM framework should function properly if the
> > > "default" values are used.
> >
> > Patch 5 eliminates the possibilities of errors and subtle bugs all
> > together. The problem with subtle bugs is, well, they are subtle, if
> > you and I knew of the bugs, we would fix all of them, but we don't. I
> > really feel we ought to eliminate the class of issues and not just
> > whack-a-mole when we see the bugs.
>
> Here's the thing, I don't really like patch 5/5. To be honest, I
> don't really like a lot of this patchset. From my perspective, the
> complexity of the code is likely going to mean more maintenance
> headaches down the road, but Linus hath spoken so we're doing this
> (although "this" is still a bit undefined as far as I'm concerned).
> If you want me to merge patch 5/5 you've got to give me something real
> and convincing that can't be fixed by any other means. My current
> opinion is that you're trying to use a previously fixed bug to scare
> and/or coerce the merging of some changes I don't really want to
> merge. If you want me to take patch 5/5, you've got to give me a
> reason that is far more compelling that what you've written thus far.
Paul, I am not scaring you, I am providing a solution that saves us
from headaches with side-effects and bugs in the future. It's safer by
design.
You say you have not reviewed it carefully, but you did ask me to move
the function from the BPF LSM layer to an LSM API, and we had a bunch
of discussion around naming in the subsequent revisions.
https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
My reasons are:
1. It's safer, no side effects, guaranteed to be not buggy. Neither
you, nor me, can guarantee that a default value will be safe in the
LSM layer. I request others (Casey, Kees) for their opinion here too.
2. Performance, no extra function call.
If you still don't like it, it's your call, I would still like to keep
most of the logic local to the BPF LSM as proposed in
https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
- KP
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 23:08 ` KP Singh
@ 2024-07-03 23:44 ` Casey Schaufler
2024-07-04 0:24 ` KP Singh
2024-07-05 18:07 ` Paul Moore
1 sibling, 1 reply; 34+ messages in thread
From: Casey Schaufler @ 2024-07-03 23:44 UTC (permalink / raw)
To: KP Singh, Paul Moore
Cc: linux-security-module, bpf, ast, andrii, keescook, daniel,
renauld, revest, song, Casey Schaufler
On 7/3/2024 4:08 PM, KP Singh wrote:
> On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
>> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
>>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
>>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
>>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
>>>>>> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
>>>>> [...]
>>>>>
>>>>>> should fix the more obvious problems. I'd like to know if you are
>>>>>> aware of any others? If not, the text above should be adjusted and
>>>>>> we should reconsider patch 5/5. If there are other problems I'd
>>>>>> like to better understand them as there may be an independent
>>>>>> solution for those particular problems.
>>>>> We did have problems with some other hooks but I was unable to dig up
>>>>> specific examples though, it's been a while. More broadly speaking, a
>>>>> default decision is still a decision. Whereas the intent from the BPF
>>>>> LSM is not to make a default decision unless a BPF program is loaded.
>>>>> I am quite worried about the holes this leaves open, subtle bugs
>>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away
>>>>> the problem of the "default decision".
>>>> The inode/xattr problem you originally mentioned wasn't really rooted
>>>> in a "bad" default return value, it was really an issue with how the
>>>> LSM hook was structured due to some legacy design assumptions made
>>>> well before the initial stacking patches were merged. That should be
>>>> fixed now[1] and given that the inode/xattr set/remove hooks were
>>>> unique in this regard (individual LSMs were responsible for performing
>>>> the capabilities checks) I don't expect this to be a general problem.
>>>>
>>>> There were also some issues caused by the fact that we were defining
>>>> the default return value in multiple places and these values had gone
>>>> out of sync in a number of hooks. We've also fixed this problem by
>>>> only defining the default return value once for each hook, solving all
>>>> of those problems.
>>> I don't see how this solves problems or prevents any future problems
>>> with side-effects. I have always been uncomfortable with an extraneous
>>> function being called with a side effect ever since we merged BPF LSM
>>> with default callback. We have found one bug due to this, not all the
>>> bugs.
>> You've got to give me something more concrete than that. If you can't
>> provide any concrete examples, start with providing a basic concept
>> with far more detail than just "side-effects".
>>
>>>> I'm not aware of any other existing problems relating to the LSM hook
>>>> default values, if there are any, we need to fix them independent of
>>>> this patchset. The LSM framework should function properly if the
>>>> "default" values are used.
>>> Patch 5 eliminates the possibilities of errors and subtle bugs all
>>> together. The problem with subtle bugs is, well, they are subtle, if
>>> you and I knew of the bugs, we would fix all of them, but we don't. I
>>> really feel we ought to eliminate the class of issues and not just
>>> whack-a-mole when we see the bugs.
>> Here's the thing, I don't really like patch 5/5. To be honest, I
>> don't really like a lot of this patchset. From my perspective, the
>> complexity of the code is likely going to mean more maintenance
>> headaches down the road, but Linus hath spoken so we're doing this
>> (although "this" is still a bit undefined as far as I'm concerned).
>> If you want me to merge patch 5/5 you've got to give me something real
>> and convincing that can't be fixed by any other means. My current
>> opinion is that you're trying to use a previously fixed bug to scare
>> and/or coerce the merging of some changes I don't really want to
>> merge. If you want me to take patch 5/5, you've got to give me a
>> reason that is far more compelling that what you've written thus far.
> Paul, I am not scaring you, I am providing a solution that saves us
> from headaches with side-effects and bugs in the future. It's safer by
> design.
>
> You say you have not reviewed it carefully, but you did ask me to move
> the function from the BPF LSM layer to an LSM API, and we had a bunch
> of discussion around naming in the subsequent revisions.
>
> https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
>
> My reasons are:
>
> 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> you, nor me, can guarantee that a default value will be safe in the
> LSM layer. I request others (Casey, Kees) for their opinion here too.
> 2. Performance, no extra function call.
I want to be very careful about the comments I make on this patch set.
I can't say that I trust any fix for the BPF LSM layer. My natural
inclination is to isolate the fix to the area that has the problem,
that is, BPF. I have a hard time accepting the notion that the implementation
will really fix all possible bugs in the future. The pace at which eBPF
is evolving gives me the heebee geebees when I think of it as a mechanism
for implementing security modules.
My biggest concern is that we may be trying too hard for perfection.
I see a situation where we're not moving forward because there are two
reasonable solutions and rather than running with either we're desperately
looking for a compelling reason to pick one over the other.
>
> If you still don't like it, it's your call, I would still like to keep
> most of the logic local to the BPF LSM as proposed in
> https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
>
> - KP
>
>> --
>> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 23:44 ` Casey Schaufler
@ 2024-07-04 0:24 ` KP Singh
2024-07-04 1:15 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-07-04 0:24 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, linux-security-module, bpf, ast, andrii, keescook,
daniel, renauld, revest, song
On Thu, Jul 4, 2024 at 2:04 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 7/3/2024 4:08 PM, KP Singh wrote:
> > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> >>>>>> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> >>>>> [...]
> >>>>>
> >>>>>> should fix the more obvious problems. I'd like to know if you are
> >>>>>> aware of any others? If not, the text above should be adjusted and
> >>>>>> we should reconsider patch 5/5. If there are other problems I'd
> >>>>>> like to better understand them as there may be an independent
> >>>>>> solution for those particular problems.
> >>>>> We did have problems with some other hooks but I was unable to dig up
> >>>>> specific examples though, it's been a while. More broadly speaking, a
> >>>>> default decision is still a decision. Whereas the intent from the BPF
> >>>>> LSM is not to make a default decision unless a BPF program is loaded.
> >>>>> I am quite worried about the holes this leaves open, subtle bugs
> >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> >>>>> the problem of the "default decision".
> >>>> The inode/xattr problem you originally mentioned wasn't really rooted
> >>>> in a "bad" default return value, it was really an issue with how the
> >>>> LSM hook was structured due to some legacy design assumptions made
> >>>> well before the initial stacking patches were merged. That should be
> >>>> fixed now[1] and given that the inode/xattr set/remove hooks were
> >>>> unique in this regard (individual LSMs were responsible for performing
> >>>> the capabilities checks) I don't expect this to be a general problem.
> >>>>
> >>>> There were also some issues caused by the fact that we were defining
> >>>> the default return value in multiple places and these values had gone
> >>>> out of sync in a number of hooks. We've also fixed this problem by
> >>>> only defining the default return value once for each hook, solving all
> >>>> of those problems.
> >>> I don't see how this solves problems or prevents any future problems
> >>> with side-effects. I have always been uncomfortable with an extraneous
> >>> function being called with a side effect ever since we merged BPF LSM
> >>> with default callback. We have found one bug due to this, not all the
> >>> bugs.
> >> You've got to give me something more concrete than that. If you can't
> >> provide any concrete examples, start with providing a basic concept
> >> with far more detail than just "side-effects".
> >>
> >>>> I'm not aware of any other existing problems relating to the LSM hook
> >>>> default values, if there are any, we need to fix them independent of
> >>>> this patchset. The LSM framework should function properly if the
> >>>> "default" values are used.
> >>> Patch 5 eliminates the possibilities of errors and subtle bugs all
> >>> together. The problem with subtle bugs is, well, they are subtle, if
> >>> you and I knew of the bugs, we would fix all of them, but we don't. I
> >>> really feel we ought to eliminate the class of issues and not just
> >>> whack-a-mole when we see the bugs.
> >> Here's the thing, I don't really like patch 5/5. To be honest, I
> >> don't really like a lot of this patchset. From my perspective, the
> >> complexity of the code is likely going to mean more maintenance
> >> headaches down the road, but Linus hath spoken so we're doing this
> >> (although "this" is still a bit undefined as far as I'm concerned).
> >> If you want me to merge patch 5/5 you've got to give me something real
> >> and convincing that can't be fixed by any other means. My current
> >> opinion is that you're trying to use a previously fixed bug to scare
> >> and/or coerce the merging of some changes I don't really want to
> >> merge. If you want me to take patch 5/5, you've got to give me a
> >> reason that is far more compelling that what you've written thus far.
> > Paul, I am not scaring you, I am providing a solution that saves us
> > from headaches with side-effects and bugs in the future. It's safer by
> > design.
> >
> > You say you have not reviewed it carefully, but you did ask me to move
> > the function from the BPF LSM layer to an LSM API, and we had a bunch
> > of discussion around naming in the subsequent revisions.
> >
> > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> >
> > My reasons are:
> >
> > 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> > you, nor me, can guarantee that a default value will be safe in the
> > LSM layer. I request others (Casey, Kees) for their opinion here too.
> > 2. Performance, no extra function call.
>
> I want to be very careful about the comments I make on this patch set.
> I can't say that I trust any fix for the BPF LSM layer. My natural
> inclination is to isolate the fix to the area that has the problem,
> that is, BPF. I have a hard time accepting the notion that the implementation
> will really fix all possible bugs in the future. The pace at which eBPF
> is evolving gives me the heebee geebees when I think of it as a mechanism
> for implementing security modules.
>
> My biggest concern is that we may be trying too hard for perfection.
> I see a situation where we're not moving forward because there are two
> reasonable solutions and rather than running with either we're desperately
> looking for a compelling reason to pick one over the other.
I am fine with either, if you folks prefer security_toggle_hook to be
in BPF only / limited to BPF. I can revert back to what we had in v9,
the changes to the LSM layer are then very minimal.
- KP
>
> >
> > If you still don't like it, it's your call, I would still like to keep
> > most of the logic local to the BPF LSM as proposed in
> > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> >
> > - KP
> >
> >> --
> >> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-04 0:24 ` KP Singh
@ 2024-07-04 1:15 ` KP Singh
0 siblings, 0 replies; 34+ messages in thread
From: KP Singh @ 2024-07-04 1:15 UTC (permalink / raw)
To: Casey Schaufler
Cc: Paul Moore, linux-security-module, bpf, ast, andrii, keescook,
daniel, renauld, revest, song
On Thu, Jul 4, 2024 at 2:24 AM KP Singh <kpsingh@kernel.org> wrote:
>
> On Thu, Jul 4, 2024 at 2:04 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > On 7/3/2024 4:08 PM, KP Singh wrote:
> > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > >>>>>> On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> > >>>>> [...]
> > >>>>>
> > >>>>>> should fix the more obvious problems. I'd like to know if you are
> > >>>>>> aware of any others? If not, the text above should be adjusted and
> > >>>>>> we should reconsider patch 5/5. If there are other problems I'd
> > >>>>>> like to better understand them as there may be an independent
> > >>>>>> solution for those particular problems.
> > >>>>> We did have problems with some other hooks but I was unable to dig up
> > >>>>> specific examples though, it's been a while. More broadly speaking, a
> > >>>>> default decision is still a decision. Whereas the intent from the BPF
> > >>>>> LSM is not to make a default decision unless a BPF program is loaded.
> > >>>>> I am quite worried about the holes this leaves open, subtle bugs
> > >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away
> > >>>>> the problem of the "default decision".
> > >>>> The inode/xattr problem you originally mentioned wasn't really rooted
> > >>>> in a "bad" default return value, it was really an issue with how the
> > >>>> LSM hook was structured due to some legacy design assumptions made
> > >>>> well before the initial stacking patches were merged. That should be
> > >>>> fixed now[1] and given that the inode/xattr set/remove hooks were
> > >>>> unique in this regard (individual LSMs were responsible for performing
> > >>>> the capabilities checks) I don't expect this to be a general problem.
> > >>>>
> > >>>> There were also some issues caused by the fact that we were defining
> > >>>> the default return value in multiple places and these values had gone
> > >>>> out of sync in a number of hooks. We've also fixed this problem by
> > >>>> only defining the default return value once for each hook, solving all
> > >>>> of those problems.
> > >>> I don't see how this solves problems or prevents any future problems
> > >>> with side-effects. I have always been uncomfortable with an extraneous
> > >>> function being called with a side effect ever since we merged BPF LSM
> > >>> with default callback. We have found one bug due to this, not all the
> > >>> bugs.
> > >> You've got to give me something more concrete than that. If you can't
> > >> provide any concrete examples, start with providing a basic concept
> > >> with far more detail than just "side-effects".
> > >>
> > >>>> I'm not aware of any other existing problems relating to the LSM hook
> > >>>> default values, if there are any, we need to fix them independent of
> > >>>> this patchset. The LSM framework should function properly if the
> > >>>> "default" values are used.
> > >>> Patch 5 eliminates the possibilities of errors and subtle bugs all
> > >>> together. The problem with subtle bugs is, well, they are subtle, if
> > >>> you and I knew of the bugs, we would fix all of them, but we don't. I
> > >>> really feel we ought to eliminate the class of issues and not just
> > >>> whack-a-mole when we see the bugs.
> > >> Here's the thing, I don't really like patch 5/5. To be honest, I
> > >> don't really like a lot of this patchset. From my perspective, the
> > >> complexity of the code is likely going to mean more maintenance
> > >> headaches down the road, but Linus hath spoken so we're doing this
> > >> (although "this" is still a bit undefined as far as I'm concerned).
> > >> If you want me to merge patch 5/5 you've got to give me something real
> > >> and convincing that can't be fixed by any other means. My current
> > >> opinion is that you're trying to use a previously fixed bug to scare
> > >> and/or coerce the merging of some changes I don't really want to
> > >> merge. If you want me to take patch 5/5, you've got to give me a
> > >> reason that is far more compelling that what you've written thus far.
> > > Paul, I am not scaring you, I am providing a solution that saves us
> > > from headaches with side-effects and bugs in the future. It's safer by
> > > design.
> > >
> > > You say you have not reviewed it carefully, but you did ask me to move
> > > the function from the BPF LSM layer to an LSM API, and we had a bunch
> > > of discussion around naming in the subsequent revisions.
> > >
> > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> > >
> > > My reasons are:
> > >
> > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> > > you, nor me, can guarantee that a default value will be safe in the
> > > LSM layer. I request others (Casey, Kees) for their opinion here too.
> > > 2. Performance, no extra function call.
> >
> > I want to be very careful about the comments I make on this patch set.
> > I can't say that I trust any fix for the BPF LSM layer. My natural
> > inclination is to isolate the fix to the area that has the problem,
> > that is, BPF. I have a hard time accepting the notion that the implementation
> > will really fix all possible bugs in the future. The pace at which eBPF
> > is evolving gives me the heebee geebees when I think of it as a mechanism
> > for implementing security modules.
> >
> > My biggest concern is that we may be trying too hard for perfection.
> > I see a situation where we're not moving forward because there are two
> > reasonable solutions and rather than running with either we're desperately
> > looking for a compelling reason to pick one over the other.
>
> I am fine with either, if you folks prefer security_toggle_hook to be
> in BPF only / limited to BPF. I can revert back to what we had in v9,
> the changes to the LSM layer are then very minimal.
Now if you folks really don't want any changes to the core LSM layer,
that too is doable, the patch below accomplishes that.
So, here's where we are at, while the LSM framework is comfortable
with saying that default values and empty callbacks are fine, that's
not what BPF LSM is comfortable doing. Your concerns around changes to
the LSM layer should be addressed by the propose patch below:
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 1de7ece5d36d..5a2ab1067095 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)
@@ -52,6 +53,10 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
return false;
}
+static inline void bpf_lsm_toggle_hook(void *addr, bool value)
+{
+}
+
static inline bool bpf_lsm_is_trusted(const struct bpf_prog *prog)
{
return false;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f8302a5ca400..bc59025b3d46 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -523,6 +523,22 @@ static enum bpf_tramp_prog_type
bpf_attach_type_to_tramp(struct bpf_prog *prog)
}
}
+static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr,
+ enum bpf_tramp_prog_type kind)
+{
+ struct bpf_tramp_link *link;
+ bool found = false;
+
+ hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) {
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+ found = true;
+ break;
+ }
+ }
+
+ bpf_lsm_toggle_hook(tr->func.addr, found);
+}
+
static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
struct bpf_trampoline *tr)
{
enum bpf_tramp_prog_type kind;
@@ -562,6 +578,10 @@ static int __bpf_trampoline_link_prog(struct
bpf_tramp_link *link, struct bpf_tr
hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
tr->progs_cnt[kind]++;
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM)
+ bpf_trampoline_toggle_lsm(tr, kind);
+
err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
if (err) {
hlist_del_init(&link->tramp_hlist);
@@ -595,6 +615,10 @@ static int __bpf_trampoline_unlink_prog(struct
bpf_tramp_link *link, struct bpf_
}
hlist_del_init(&link->tramp_hlist);
tr->progs_cnt[kind]--;
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM)
+ bpf_trampoline_toggle_lsm(tr, kind);
+
return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
}
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 57b9ffd53c98..9ca3db6d2b07 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -16,6 +16,29 @@ static struct security_hook_list bpf_lsm_hooks[]
__ro_after_init = {
LSM_HOOK_INIT(task_free, bpf_task_storage_free),
};
+void bpf_lsm_toggle_hook(void *addr, bool enable)
+{
+ struct lsm_static_call *scalls;
+ struct security_hook_list *h;
+ int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) {
+ h = &bpf_lsm_hooks[i];
+ if (h->hook.lsm_func_addr != addr)
+ continue;
+
+ for (j = 0; j < MAX_LSM_COUNT; j++) {
+ scalls = &h->scalls[j];
+ if (scalls->hl != &bpf_lsm_hooks[i])
+ continue;
+ if (enable)
+ static_branch_enable(scalls->active);
+ else
+ static_branch_disable(scalls->active);
+ }
+ }
+}
+
static const struct lsm_id bpf_lsmid = {
.name = "bpf",
.id = LSM_ID_BPF,
@@ -23,8 +46,14 @@ static const struct lsm_id bpf_lsmid = {
static int __init bpf_lsm_init(void)
{
+ int i;
+
security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks),
&bpf_lsmid);
+
+ for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++)
+ bpf_lsm_toggle_hook(bpf_lsm_hooks[i].hook.lsm_func_addr, false);
+
pr_info("LSM support for eBPF active\n");
return 0;
}
- KP
>
> - KP
>
> >
> > >
> > > If you still don't like it, it's your call, I would still like to keep
> > > most of the logic local to the BPF LSM as proposed in
> > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> > >
> > > - KP
> > >
> > >> --
> > >> paul-moore.com
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-03 23:08 ` KP Singh
2024-07-03 23:44 ` Casey Schaufler
@ 2024-07-05 18:07 ` Paul Moore
2024-07-05 19:34 ` KP Singh
1 sibling, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-05 18:07 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
> On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
...
> > > > I'm not aware of any other existing problems relating to the LSM hook
> > > > default values, if there are any, we need to fix them independent of
> > > > this patchset. The LSM framework should function properly if the
> > > > "default" values are used.
> > >
> > > Patch 5 eliminates the possibilities of errors and subtle bugs all
> > > together. The problem with subtle bugs is, well, they are subtle, if
> > > you and I knew of the bugs, we would fix all of them, but we don't. I
> > > really feel we ought to eliminate the class of issues and not just
> > > whack-a-mole when we see the bugs.
> >
> > Here's the thing, I don't really like patch 5/5. To be honest, I
> > don't really like a lot of this patchset. From my perspective, the
> > complexity of the code is likely going to mean more maintenance
> > headaches down the road, but Linus hath spoken so we're doing this
> > (although "this" is still a bit undefined as far as I'm concerned).
> > If you want me to merge patch 5/5 you've got to give me something real
> > and convincing that can't be fixed by any other means. My current
> > opinion is that you're trying to use a previously fixed bug to scare
> > and/or coerce the merging of some changes I don't really want to
> > merge. If you want me to take patch 5/5, you've got to give me a
> > reason that is far more compelling that what you've written thus far.
>
> Paul, I am not scaring you, I am providing a solution that saves us
> from headaches with side-effects and bugs in the future. It's safer by
> design.
Perhaps I wasn't clear enough in my previous emails; instead of trying
to convince me that your solution is literally the best possible thing
to ever touch the kernel, convince me that there is a problem we need
to fix. Right now, I'm not convinced there is a bug that requires all
of the extra code in patch 5/5 (all of which have the potential to
introduce new bugs). As mentioned previously, the bugs that typically
have been used as examples of unwanted side effects with the LSM hooks
have been resolved, both in the specific and general case. If you
want me to add more code/functionality to fix a bug, you must first
demonstrate the bug exists and the risk is real; you have not done
that as far as I'm concerned.
> You say you have not reviewed it carefully ...
That may have been true of previous versions of this patchset, but I
did not say that about this current patchset.
> ... but you did ask me to move
> the function from the BPF LSM layer to an LSM API, and we had a bunch
> of discussion around naming in the subsequent revisions.
>
> https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
That discussion predates commit 61df7b828204 ("lsm: fixup the inode
xattr capability handling") which is currently in the lsm/dev branch,
marked for stable, and will go up to Linus during the upcoming merge
window.
> My reasons are:
>
> 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> you, nor me, can guarantee that a default value will be safe in the
> LSM layer.
In the first sentence above you "guarantee" that your code is not
buggy and then follow that up with a second sentence discussing how no
one can guarantee source code safety. Regardless of whatever point
you were trying to make here, I maintain that *all* patches have the
potential for bugs, even those that are attempting to fix bugs. With
that in mind, if you want me to merge more code to fix a bug (class),
a bug that I've mentioned several times now that I believe we've
already fixed, you first MUST convince me that the bug (class) still
exists. You have not done that.
> 2. Performance, no extra function call.
Convince me the bug still exists first and then we can discuss the
merits of whatever solutions are proposed.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-05 18:07 ` Paul Moore
@ 2024-07-05 19:34 ` KP Singh
2024-07-06 0:17 ` Kees Cook
2024-07-06 4:40 ` Paul Moore
0 siblings, 2 replies; 34+ messages in thread
From: KP Singh @ 2024-07-05 19:34 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
> > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
>
> ...
>
> > > > > I'm not aware of any other existing problems relating to the LSM hook
> > > > > default values, if there are any, we need to fix them independent of
> > > > > this patchset. The LSM framework should function properly if the
> > > > > "default" values are used.
> > > >
> > > > Patch 5 eliminates the possibilities of errors and subtle bugs all
> > > > together. The problem with subtle bugs is, well, they are subtle, if
> > > > you and I knew of the bugs, we would fix all of them, but we don't. I
> > > > really feel we ought to eliminate the class of issues and not just
> > > > whack-a-mole when we see the bugs.
> > >
> > > Here's the thing, I don't really like patch 5/5. To be honest, I
> > > don't really like a lot of this patchset. From my perspective, the
> > > complexity of the code is likely going to mean more maintenance
> > > headaches down the road, but Linus hath spoken so we're doing this
> > > (although "this" is still a bit undefined as far as I'm concerned).
> > > If you want me to merge patch 5/5 you've got to give me something real
> > > and convincing that can't be fixed by any other means. My current
> > > opinion is that you're trying to use a previously fixed bug to scare
> > > and/or coerce the merging of some changes I don't really want to
> > > merge. If you want me to take patch 5/5, you've got to give me a
> > > reason that is far more compelling that what you've written thus far.
> >
> > Paul, I am not scaring you, I am providing a solution that saves us
> > from headaches with side-effects and bugs in the future. It's safer by
> > design.
>
> Perhaps I wasn't clear enough in my previous emails; instead of trying
> to convince me that your solution is literally the best possible thing
> to ever touch the kernel, convince me that there is a problem we need
> to fix. Right now, I'm not convinced there is a bug that requires all
> of the extra code in patch 5/5 (all of which have the potential to
> introduce new bugs). As mentioned previously, the bugs that typically
> have been used as examples of unwanted side effects with the LSM hooks
> have been resolved, both in the specific and general case. If you
> want me to add more code/functionality to fix a bug, you must first
> demonstrate the bug exists and the risk is real; you have not done
> that as far as I'm concerned.
>
> > You say you have not reviewed it carefully ...
>
> That may have been true of previous versions of this patchset, but I
> did not say that about this current patchset.
>
> > ... but you did ask me to move
> > the function from the BPF LSM layer to an LSM API, and we had a bunch
> > of discussion around naming in the subsequent revisions.
> >
> > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
>
> That discussion predates commit 61df7b828204 ("lsm: fixup the inode
> xattr capability handling") which is currently in the lsm/dev branch,
> marked for stable, and will go up to Linus during the upcoming merge
> window.
>
> > My reasons are:
> >
> > 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> > you, nor me, can guarantee that a default value will be safe in the
> > LSM layer.
>
> In the first sentence above you "guarantee" that your code is not
> buggy and then follow that up with a second sentence discussing how no
> one can guarantee source code safety. Regardless of whatever point
> you were trying to make here, I maintain that *all* patches have the
> potential for bugs, even those that are attempting to fix bugs. WithD
> that in mind, if you want me to merge more code to fix a bug (class),
> a bug that I've mentioned several times now that I believe we've
> already fixed, you first MUST convince me that the bug (class) still
> exists. You have not done that.
>
Paul, I am talking about eliminating a class of bugs, but you don't
seem to get the point and you are fixated on the very instance of this
bug class.
> > 2. Performance, no extra function call.
>
> Convince me the bug still exists first and then we can discuss the
> merits of whatever solutions are proposed.
This is independent of the bug!
The extra function calls have performance overhead and as the BPF LSM
maintainer I am not okay with these extraneous calls when I have a
clear way of solving it.
As I said, If you don't want to modify the core LSM layer, it's okay,
I still want to go with changes local to the BPF LSM, If you really
don't agree with the changes local to the BPF LSM, we can have it go
via the BPF tree and seek Linus' help to resolve the conflict.
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 1de7ece5d36d..5a2ab1067095 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)
@@ -52,6 +53,10 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
return false;
}
+static inline void bpf_lsm_toggle_hook(void *addr, bool value)
+{
+}
+
static inline bool bpf_lsm_is_trusted(const struct bpf_prog *prog)
{
return false;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index f8302a5ca400..bc59025b3d46 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -523,6 +523,22 @@ static enum bpf_tramp_prog_type
bpf_attach_type_to_tramp(struct bpf_prog *prog)
}
}
+static void bpf_trampoline_toggle_lsm(struct bpf_trampoline *tr,
+ enum bpf_tramp_prog_type kind)
+{
+ struct bpf_tramp_link *link;
+ bool found = false;
+
+ hlist_for_each_entry(link, &tr->progs_hlist[kind], tramp_hlist) {
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+ found = true;
+ break;
+ }
+ }
+
+ bpf_lsm_toggle_hook(tr->func.addr, found);
+}
+
static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link,
struct bpf_trampoline *tr)
{
enum bpf_tramp_prog_type kind;
@@ -562,6 +578,10 @@ static int __bpf_trampoline_link_prog(struct
bpf_tramp_link *link, struct bpf_tr
hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
tr->progs_cnt[kind]++;
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM)
+ bpf_trampoline_toggle_lsm(tr, kind);
+
err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
if (err) {
hlist_del_init(&link->tramp_hlist);
@@ -595,6 +615,10 @@ static int __bpf_trampoline_unlink_prog(struct
bpf_tramp_link *link, struct bpf_
}
hlist_del_init(&link->tramp_hlist);
tr->progs_cnt[kind]--;
+
+ if (link->link.prog->type == BPF_PROG_TYPE_LSM)
+ bpf_trampoline_toggle_lsm(tr, kind);
+
return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
}
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index 57b9ffd53c98..9ca3db6d2b07 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -16,6 +16,29 @@ static struct security_hook_list bpf_lsm_hooks[]
__ro_after_init = {
LSM_HOOK_INIT(task_free, bpf_task_storage_free),
};
+void bpf_lsm_toggle_hook(void *addr, bool enable)
+{
+ struct lsm_static_call *scalls;
+ struct security_hook_list *h;
+ int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) {
+ h = &bpf_lsm_hooks[i];
+ if (h->hook.lsm_func_addr != addr)
+ continue;
+
+ for (j = 0; j < MAX_LSM_COUNT; j++) {
+ scalls = &h->scalls[j];
+ if (scalls->hl != &bpf_lsm_hooks[i])
+ continue;
+ if (enable)
+ static_branch_enable(scalls->active);
+ else
+ static_branch_disable(scalls->active);
+ }
+ }
+}
+
static const struct lsm_id bpf_lsmid = {
.name = "bpf",
.id = LSM_ID_BPF,
@@ -23,8 +46,14 @@ static const struct lsm_id bpf_lsmid = {
static int __init bpf_lsm_init(void)
{
+ int i;
+
security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks),
&bpf_lsmid);
+
+ for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++)
+ bpf_lsm_toggle_hook(bpf_lsm_hooks[i].hook.lsm_func_addr, false);
+
pr_info("LSM support for eBPF active\n");
return 0;
}
>
> --
> paul-moore.com
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-05 19:34 ` KP Singh
@ 2024-07-06 0:17 ` Kees Cook
2024-07-06 4:46 ` Paul Moore
2024-07-06 4:40 ` Paul Moore
1 sibling, 1 reply; 34+ messages in thread
From: Kees Cook @ 2024-07-06 0:17 UTC (permalink / raw)
To: KP Singh
Cc: Paul Moore, linux-security-module, bpf, ast, casey, andrii,
daniel, renauld, revest, song
On Fri, Jul 05, 2024 at 09:34:20PM +0200, KP Singh wrote:
> On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> >
> > ...
> >
> > > > > > I'm not aware of any other existing problems relating to the LSM hook
> > > > > > default values, if there are any, we need to fix them independent of
> > > > > > this patchset. The LSM framework should function properly if the
> > > > > > "default" values are used.
> > > > >
> > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all
> > > > > together. The problem with subtle bugs is, well, they are subtle, if
> > > > > you and I knew of the bugs, we would fix all of them, but we don't. I
> > > > > really feel we ought to eliminate the class of issues and not just
> > > > > whack-a-mole when we see the bugs.
> > > >
> > > > Here's the thing, I don't really like patch 5/5. To be honest, I
> > > > don't really like a lot of this patchset. From my perspective, the
> > > > complexity of the code is likely going to mean more maintenance
> > > > headaches down the road, but Linus hath spoken so we're doing this
> > > > (although "this" is still a bit undefined as far as I'm concerned).
> > > > If you want me to merge patch 5/5 you've got to give me something real
> > > > and convincing that can't be fixed by any other means. My current
> > > > opinion is that you're trying to use a previously fixed bug to scare
> > > > and/or coerce the merging of some changes I don't really want to
> > > > merge. If you want me to take patch 5/5, you've got to give me a
> > > > reason that is far more compelling that what you've written thus far.
> > >
> > > Paul, I am not scaring you, I am providing a solution that saves us
> > > from headaches with side-effects and bugs in the future. It's safer by
> > > design.
> >
> > Perhaps I wasn't clear enough in my previous emails; instead of trying
> > to convince me that your solution is literally the best possible thing
> > to ever touch the kernel, convince me that there is a problem we need
> > to fix. Right now, I'm not convinced there is a bug that requires all
> > of the extra code in patch 5/5 (all of which have the potential to
> > introduce new bugs). As mentioned previously, the bugs that typically
> > have been used as examples of unwanted side effects with the LSM hooks
> > have been resolved, both in the specific and general case. If you
> > want me to add more code/functionality to fix a bug, you must first
> > demonstrate the bug exists and the risk is real; you have not done
> > that as far as I'm concerned.
> >
> > > You say you have not reviewed it carefully ...
> >
> > That may have been true of previous versions of this patchset, but I
> > did not say that about this current patchset.
> >
> > > ... but you did ask me to move
> > > the function from the BPF LSM layer to an LSM API, and we had a bunch
> > > of discussion around naming in the subsequent revisions.
> > >
> > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> >
> > That discussion predates commit 61df7b828204 ("lsm: fixup the inode
> > xattr capability handling") which is currently in the lsm/dev branch,
> > marked for stable, and will go up to Linus during the upcoming merge
> > window.
> >
> > > My reasons are:
> > >
> > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> > > you, nor me, can guarantee that a default value will be safe in the
> > > LSM layer.
> >
> > In the first sentence above you "guarantee" that your code is not
> > buggy and then follow that up with a second sentence discussing how no
> > one can guarantee source code safety. Regardless of whatever point
> > you were trying to make here, I maintain that *all* patches have the
> > potential for bugs, even those that are attempting to fix bugs. WithD
> > that in mind, if you want me to merge more code to fix a bug (class),
> > a bug that I've mentioned several times now that I believe we've
> > already fixed, you first MUST convince me that the bug (class) still
> > exists. You have not done that.
> >
>
> Paul, I am talking about eliminating a class of bugs, but you don't
> seem to get the point and you are fixated on the very instance of this
> bug class.
Let's take this one step at a time. I think patches 1-4 are fine and
stand alone and solve a specific problem without creating any new
immediate problems.
After 1-4 is accepted, we can come back around to what patch 5 is trying
to do, and work on whatever issues may remain at that time.
--
Kees Cook
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-05 19:34 ` KP Singh
2024-07-06 0:17 ` Kees Cook
@ 2024-07-06 4:40 ` Paul Moore
2024-07-08 10:04 ` KP Singh
1 sibling, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-06 4:40 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote:
> On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > > > On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > > > On Jun 29, 2024 KP Singh <kpsingh@kernel.org> 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:
> >
> > ...
> >
> > > > > > I'm not aware of any other existing problems relating to the LSM hook
> > > > > > default values, if there are any, we need to fix them independent of
> > > > > > this patchset. The LSM framework should function properly if the
> > > > > > "default" values are used.
> > > > >
> > > > > Patch 5 eliminates the possibilities of errors and subtle bugs all
> > > > > together. The problem with subtle bugs is, well, they are subtle, if
> > > > > you and I knew of the bugs, we would fix all of them, but we don't. I
> > > > > really feel we ought to eliminate the class of issues and not just
> > > > > whack-a-mole when we see the bugs.
> > > >
> > > > Here's the thing, I don't really like patch 5/5. To be honest, I
> > > > don't really like a lot of this patchset. From my perspective, the
> > > > complexity of the code is likely going to mean more maintenance
> > > > headaches down the road, but Linus hath spoken so we're doing this
> > > > (although "this" is still a bit undefined as far as I'm concerned).
> > > > If you want me to merge patch 5/5 you've got to give me something real
> > > > and convincing that can't be fixed by any other means. My current
> > > > opinion is that you're trying to use a previously fixed bug to scare
> > > > and/or coerce the merging of some changes I don't really want to
> > > > merge. If you want me to take patch 5/5, you've got to give me a
> > > > reason that is far more compelling that what you've written thus far.
> > >
> > > Paul, I am not scaring you, I am providing a solution that saves us
> > > from headaches with side-effects and bugs in the future. It's safer by
> > > design.
> >
> > Perhaps I wasn't clear enough in my previous emails; instead of trying
> > to convince me that your solution is literally the best possible thing
> > to ever touch the kernel, convince me that there is a problem we need
> > to fix. Right now, I'm not convinced there is a bug that requires all
> > of the extra code in patch 5/5 (all of which have the potential to
> > introduce new bugs). As mentioned previously, the bugs that typically
> > have been used as examples of unwanted side effects with the LSM hooks
> > have been resolved, both in the specific and general case. If you
> > want me to add more code/functionality to fix a bug, you must first
> > demonstrate the bug exists and the risk is real; you have not done
> > that as far as I'm concerned.
> >
> > > You say you have not reviewed it carefully ...
> >
> > That may have been true of previous versions of this patchset, but I
> > did not say that about this current patchset.
> >
> > > ... but you did ask me to move
> > > the function from the BPF LSM layer to an LSM API, and we had a bunch
> > > of discussion around naming in the subsequent revisions.
> > >
> > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@paul-moore.com/
> >
> > That discussion predates commit 61df7b828204 ("lsm: fixup the inode
> > xattr capability handling") which is currently in the lsm/dev branch,
> > marked for stable, and will go up to Linus during the upcoming merge
> > window.
> >
> > > My reasons are:
> > >
> > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither
> > > you, nor me, can guarantee that a default value will be safe in the
> > > LSM layer.
> >
> > In the first sentence above you "guarantee" that your code is not
> > buggy and then follow that up with a second sentence discussing how no
> > one can guarantee source code safety. Regardless of whatever point
> > you were trying to make here, I maintain that *all* patches have the
> > potential for bugs, even those that are attempting to fix bugs. WithD
> > that in mind, if you want me to merge more code to fix a bug (class),
> > a bug that I've mentioned several times now that I believe we've
> > already fixed, you first MUST convince me that the bug (class) still
> > exists. You have not done that.
>
> Paul, I am talking about eliminating a class of bugs, but you don't
> seem to get the point and you are fixated on the very instance of this
> bug class.
I do understand that you are trying to eliminate a class of bugs, the
point I'm trying to make is that I believe we have addressed that
already with the patches I've previously cited.
> > > 2. Performance, no extra function call.
> >
> > Convince me the bug still exists first and then we can discuss the
> > merits of whatever solutions are proposed.
>
> This is independent of the bug!
Correctness first, maintainability second, performance third. That's
my current priority and I feel the maintainability hit doesn't justify
the performance win at this point in time. Besides, we're already
expecting a big performance boost simply by moving to static_calls.
> As I said, If you don't want to modify the core LSM layer, it's okay,
> I still want to go with changes local to the BPF LSM, If you really
> don't agree with the changes local to the BPF LSM, we can have it go
> via the BPF tree and seek Linus' help to resolve the conflict.
As the BPF maintainer you are always free to do whatever you like
within the scope of the LSM you maintain so long as it does not touch
or otherwise impact any of the other LSMs or the LSM framework. If
you do affect the other LSMs, or the LSM framework, you need to get an
ACK from the associated maintainer. That's pretty much how Linux
kernel development works.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-06 0:17 ` Kees Cook
@ 2024-07-06 4:46 ` Paul Moore
0 siblings, 0 replies; 34+ messages in thread
From: Paul Moore @ 2024-07-06 4:46 UTC (permalink / raw)
To: Kees Cook
Cc: KP Singh, linux-security-module, bpf, ast, casey, andrii, daniel,
renauld, revest, song
On Fri, Jul 5, 2024 at 8:17 PM Kees Cook <kees@kernel.org> wrote:
>
> Let's take this one step at a time. I think patches 1-4 are fine and
> stand alone and solve a specific problem without creating any new
> immediate problems.
>
> After 1-4 is accepted, we can come back around to what patch 5 is trying
> to do, and work on whatever issues may remain at that time.
Kees, whatever technical review you may have for any proposed patches
is always welcome, and if you want to provide process advice,
solicited or otherwise, to people posting patches that is up to you.
However, after your last comments regarding the management of the LSM
tree, I want to make it clear that I'm not interested in your opinions
on what should be merged into the LSM tree at this point in time.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-06 4:40 ` Paul Moore
@ 2024-07-08 10:04 ` KP Singh
2024-07-08 12:52 ` Paul Moore
0 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-07-08 10:04 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote:
> > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
[...]
> >
> > Paul, I am talking about eliminating a class of bugs, but you don't
> > seem to get the point and you are fixated on the very instance of this
> > bug class.
>
> I do understand that you are trying to eliminate a class of bugs, the
> point I'm trying to make is that I believe we have addressed that
> already with the patches I've previously cited.
The class I am referring to is useless hooks returning a default value
and imposing a denial / enforcement when they are not supposed to. If
you think this class of issues is not relevant to the overall LSM,
sure. I would still like BPF LSM to not add default callbacks as I
have always maintained since day 1:
https://lwn.net/ml/linux-kernel/20200224171305.GA21886@chromium.org/
BPF LSM does not want to provide a default decision until a BPF LSM
policy program is loaded,
>
> > > > 2. Performance, no extra function call.
> > >
> > > Convince me the bug still exists first and then we can discuss the
> > > merits of whatever solutions are proposed.
> >
> > This is independent of the bug!
>
> Correctness first, maintainability second, performance third. That's
> my current priority and I feel the maintainability hit doesn't justify
> the performance win at this point in time. Besides, we're already
> expecting a big performance boost simply by moving to static_calls.
>
> > As I said, If you don't want to modify the core LSM layer, it's okay,
> > I still want to go with changes local to the BPF LSM, If you really
> > don't agree with the changes local to the BPF LSM, we can have it go
> > via the BPF tree and seek Linus' help to resolve the conflict.
>
> As the BPF maintainer you are always free to do whatever you like
> within the scope of the LSM you maintain so long as it does not touch
> or otherwise impact any of the other LSMs or the LSM framework. If
> you do affect the other LSMs, or the LSM framework, you need to get an
> ACK from the associated maintainer. That's pretty much how Linux
> kernel development works.
Okay, then let's not make an LSM API, I will handle it within the BPF LSM.
The patch I proposed should not affect any other LSMs and is self
contained within BPF LSM:
https://lore.kernel.org/bpf/CACYkzJ6jADoGNuPP3-1wkk-kV7NOQh+eFkU5KEDEZgq9qNNEfg@mail.gmail.com/
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-08 10:04 ` KP Singh
@ 2024-07-08 12:52 ` Paul Moore
2024-07-08 13:52 ` KP Singh
0 siblings, 1 reply; 34+ messages in thread
From: Paul Moore @ 2024-07-08 12:52 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Mon, Jul 8, 2024 at 6:04 AM KP Singh <kpsingh@kernel.org> wrote:
> On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote:
> > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
>
> [...]
>
> > >
> > > Paul, I am talking about eliminating a class of bugs, but you don't
> > > seem to get the point and you are fixated on the very instance of this
> > > bug class.
> >
> > I do understand that you are trying to eliminate a class of bugs, the
> > point I'm trying to make is that I believe we have addressed that
> > already with the patches I've previously cited.
>
> The class I am referring to is useless hooks returning a default value
> and imposing a denial / enforcement when they are not supposed to.
If a LSM hook's default value were to result in an undesirable
behavior within the kernel that would be an issue regardless of what
LSMs were involved and we would either need to modify the hook and/or
the default value. I am not convinced that we can adequately solve
this entire class of problems simply by allowing one LSM, or even
arbitrary combinations of LSMs, to disable or otherwise disconnect
themselves from the framework.
> > As the BPF maintainer you are always free to do whatever you like
> > within the scope of the LSM you maintain so long as it does not touch
> > or otherwise impact any of the other LSMs or the LSM framework. If
> > you do affect the other LSMs, or the LSM framework, you need to get an
> > ACK from the associated maintainer. That's pretty much how Linux
> > kernel development works.
>
> Okay, then let's not make an LSM API, I will handle it within the BPF LSM.
>
> The patch I proposed should not affect any other LSMs and is self
> contained within BPF LSM:
>
> https://lore.kernel.org/bpf/CACYkzJ6jADoGNuPP3-1wkk-kV7NOQh+eFkU5KEDEZgq9qNNEfg@mail.gmail.com/
The code changes may be self-contained within the BPF LSM, but it does
appear that the bpf_lsm_toggle_hook() function directly manipulates
the LSM framework hook state which is not something we want to allow -
none of the individual LSMs should be directly manipulating the LSM
hook state/configuration. Although perhaps I'm missing or not
factoring in some context around the patch linked above and that isn't
the case?
While I had issues with Kees' comments, at a high level his suggestion
of dropping the last patch and moving forward is something you should
consider as I don't see this a good path forward with all of the
approaches that have been discussed thus far.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-08 12:52 ` Paul Moore
@ 2024-07-08 13:52 ` KP Singh
2024-07-08 14:23 ` Paul Moore
0 siblings, 1 reply; 34+ messages in thread
From: KP Singh @ 2024-07-08 13:52 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Mon, Jul 8, 2024 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Jul 8, 2024 at 6:04 AM KP Singh <kpsingh@kernel.org> wrote:
> > On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
> >
> > [...]
> >
> > > >
> > > > Paul, I am talking about eliminating a class of bugs, but you don't
> > > > seem to get the point and you are fixated on the very instance of this
> > > > bug class.
> > >
> > > I do understand that you are trying to eliminate a class of bugs, the
> > > point I'm trying to make is that I believe we have addressed that
> > > already with the patches I've previously cited.
> >
> > The class I am referring to is useless hooks returning a default value
> > and imposing a denial / enforcement when they are not supposed to.
>
> If a LSM hook's default value were to result in an undesirable
> behavior within the kernel that would be an issue regardless of what
> LSMs were involved and we would either need to modify the hook and/or
> the default value. I am not convinced that we can adequately solve
> this entire class of problems simply by allowing one LSM, or even
> arbitrary combinations of LSMs, to disable or otherwise disconnect
> themselves from the framework.
>
> > > As the BPF maintainer you are always free to do whatever you like
> > > within the scope of the LSM you maintain so long as it does not touch
> > > or otherwise impact any of the other LSMs or the LSM framework. If
> > > you do affect the other LSMs, or the LSM framework, you need to get an
> > > ACK from the associated maintainer. That's pretty much how Linux
> > > kernel development works.
> >
> > Okay, then let's not make an LSM API, I will handle it within the BPF LSM.
> >
> > The patch I proposed should not affect any other LSMs and is self
> > contained within BPF LSM:
> >
> > https://lore.kernel.org/bpf/CACYkzJ6jADoGNuPP3-1wkk-kV7NOQh+eFkU5KEDEZgq9qNNEfg@mail.gmail.com/
>
> The code changes may be self-contained within the BPF LSM, but it does
> appear that the bpf_lsm_toggle_hook() function directly manipulates
> the LSM framework hook state which is not something we want to allow -
> none of the individual LSMs should be directly manipulating the LSM
> hook state/configuration. Although perhaps I'm missing or not
> factoring in some context around the patch linked above and that isn't
> the case?
I think you are ignoring my point that BPF does not want to add
extraneous function calls which at the least result in extra overhead.
You have ignored the fact that BPF LSM never wanted these empty
callbacks and you still continue to ignore it. Sigh, I will drop it
now and will propose it as a separate patch so that we can at least
unblock the static call series.
- KP
> While I had issues with Kees' comments, at a high level his suggestion
> of dropping the last patch and moving forward is something you should
> consider as I don't see this a good path forward with all of the
> approaches that have been discussed thus far.
>
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls
2024-07-08 13:52 ` KP Singh
@ 2024-07-08 14:23 ` Paul Moore
0 siblings, 0 replies; 34+ messages in thread
From: Paul Moore @ 2024-07-08 14:23 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Mon, Jul 8, 2024 at 9:52 AM KP Singh <kpsingh@kernel.org> wrote:
> On Mon, Jul 8, 2024 at 2:52 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Jul 8, 2024 at 6:04 AM KP Singh <kpsingh@kernel.org> wrote:
> > > On Sat, Jul 6, 2024 at 6:40 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Fri, Jul 5, 2024 at 3:34 PM KP Singh <kpsingh@kernel.org> wrote:
> > > > > On Fri, Jul 5, 2024 at 8:07 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Wed, Jul 3, 2024 at 7:08 PM KP Singh <kpsingh@kernel.org> wrote:
...
> I think you are ignoring my point that BPF does not want to add
> extraneous function calls which at the least result in extra overhead.
I haven't been ignoring you on that point, see my previous comment:
"Correctness first, maintainability second, performance third. That's
my current priority and I feel the maintainability hit doesn't justify
the performance win at this point in time. Besides, we're already
expecting a big performance boost simply by moving to static_calls."
https://lore.kernel.org/linux-security-module/CAHC9VhQQkWxMT3KguOOK7W8cbY-cdeYTJSuh=tSDV4jsqp6s6g@mail.gmail.com/
> You have ignored the fact that BPF LSM never wanted these empty
> callbacks and you still continue to ignore it. Sigh, I will drop it
> now and will propose it as a separate patch so that we can at least
> unblock the static call series.
I didn't comment on that because it isn't very relevant at this point
in time, what matters is the current status quo and the proposed
change. In this particular case I'm not going to debate decisions
made by previous maintainers, my focus is on what we currently have
in-tree and what/how/why people want to change.
You've got a path forward with the bulk of this patchset, if you want
to scuttle it over the last patch in the series that is up to you, but
in my opinion that seems like a lost opportunity.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 4/5] security: Update non standard hooks to use static calls
2024-07-03 0:07 ` Paul Moore
@ 2024-07-09 12:36 ` KP Singh
2024-07-09 14:51 ` Paul Moore
2024-07-09 16:53 ` Casey Schaufler
0 siblings, 2 replies; 34+ messages in thread
From: KP Singh @ 2024-07-09 12:36 UTC (permalink / raw)
To: Paul Moore
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
[...]
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -948,10 +948,48 @@ out: \
> > RC; \
> > })
> >
> > -#define lsm_for_each_hook(scall, NAME) \
> > - for (scall = static_calls_table.NAME; \
> > - scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> > - if (static_key_enabled(&scall->active->key))
> > +/*
> > + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> > + * current hook
> > + */
> > +#define current_lsmid() _hook_lsmid
>
> See my comments below about security_getselfattr(), I think we can drop
> the current_lsmid() macro. If we really must keep it, we need to rename
> it to something else as it clashes too much with the other current_XXX()
> macros/functions which are useful outside of our wacky macros.
call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
update the name.
What do you think about __security_hook_lsm_id().
> > +#define __CALL_HOOK(NUM, HOOK, RC, BLOCK_BEFORE, BLOCK_AFTER, ...) \
> > +do { \
> > + int __maybe_unused _hook_lsmid; \
> > + \
> > + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
> > + _hook_lsmid = static_calls_table.HOOK[NUM].hl->lsmid->id; \
> > + BLOCK_BEFORE \
> > + RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
> > + BLOCK_AFTER \
> > + } \
> > +} while (0);
> > +
> > +#define lsm_for_each_hook(HOOK, RC, BLOCK_AFTER, ...) \
> > + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BLOCK_AFTER, __VA_ARGS__)
> > +
> > +#define call_hook_with_lsmid(HOOK, LSMID, ...) \
> > +({ \
> > + __label__ out; \
> > + int RC = LSM_RET_DEFAULT(HOOK); \
> > + \
> > + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, \
> > + /* BLOCK BEFORE INVOCATION */ \
> > + { \
> > + if (current_lsmid() != LSMID) \
> > + continue; \
> > + }, \
> > + /* END BLOCK BEFORE INVOCATION */ \
> > + /* BLOCK AFTER INVOCATION */ \
> > + { \
> > + goto out; \
> > + }, \
> > + /* END BLOCK AFTER INVOCATION */ \
> > + __VA_ARGS__); \
> > +out: \
> > + RC; \
> > +})
> >
> > /* Security operations */
>
> ...
>
> > @@ -1581,15 +1629,19 @@ int security_sb_set_mnt_opts(struct super_block *sb,
> > unsigned long kern_flags,
> > unsigned long *set_kern_flags)
> > {
> > - struct lsm_static_call *scall;
> > int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
> >
> > - lsm_for_each_hook(scall, sb_set_mnt_opts) {
> > - rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
> > - set_kern_flags);
> > - if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> > - break;
> > - }
> > + lsm_for_each_hook(
> > + sb_set_mnt_opts, rc,
> > + /* BLOCK AFTER INVOCATION */
> > + {
> > + if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
> > + goto out;
> > + },
> > + /* END BLOCK AFTER INVOCATION */
> > + sb, mnt_opts, kern_flags, set_kern_flags);
> > +
> > +out:
> > return rc;
> > }
> > EXPORT_SYMBOL(security_sb_set_mnt_opts);
>
> I know I was the one who asked to implement the static_calls for *all*
> of the LSM functions - thank you for doing that - but I think we can
> all agree that some of the resulting code is pretty awful. I'm probably
> missing something important, but would an apporach similar to the pseudo
> code below work?
This does not work.
The special macro you are defining does not have the static_call
invocation and if you add that bit it's basically the __CALL_HOOK
macro or __CALL_STATIC_INT, __CALL_STATIC_VOID macro inlined
everywhere, I tried implementing it but it gets very dirty.
>
> #define call_int_hook_special(HOOK, RC, LABEL, ...) \
> LSM_LOOP_UNROLL(HOOK##_SPECIAL, RC, HOOK, LABEL, __VA_ARGS__)
>
> int security_sb_set_mnt_opts(...)
> {
> int rc = LSM_RET_DEFAULT(sb_set_mnt_opts);
>
> #define sb_set_mnt_opts_SPECIAL \
> do { \
> if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts)) \
> goto out; \
> } while (0)
>
> rc = call_int_hook_special(sb_set_mnt_opts, rc, out, ...);
>
> out:
> return rc;
> }
>
> > @@ -4040,7 +4099,6 @@ EXPORT_SYMBOL(security_d_instantiate);
> > int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> > u32 __user *size, u32 flags)
> > {
> > - struct lsm_static_call *scall;
> > struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
> > u8 __user *base = (u8 __user *)uctx;
> > u32 entrysize;
> > @@ -4078,31 +4136,42 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
> > * In the usual case gather all the data from the LSMs.
> > * In the single case only get the data from the LSM specified.
> > */
> > - lsm_for_each_hook(scall, getselfattr) {
> > - if (single && lctx.id != scall->hl->lsmid->id)
> > - continue;
> > - entrysize = left;
> > - if (base)
> > - uctx = (struct lsm_ctx __user *)(base + total);
> > - rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
> > - if (rc == -EOPNOTSUPP) {
> > - rc = 0;
> > - continue;
> > - }
> > - if (rc == -E2BIG) {
> > - rc = 0;
> > - left = 0;
> > - toobig = true;
> > - } else if (rc < 0)
> > - return rc;
> > - else
> > - left -= entrysize;
> > + LSM_LOOP_UNROLL(
> > + __CALL_HOOK, getselfattr, rc,
> > + /* BLOCK BEFORE INVOCATION */
> > + {
> > + if (single && lctx.id != current_lsmid())
> > + continue;
> > + entrysize = left;
> > + if (base)
> > + uctx = (struct lsm_ctx __user *)(base + total);
> > + },
> > + /* END BLOCK BEFORE INVOCATION */
> > + /* BLOCK AFTER INVOCATION */
> > + {
> > + if (rc == -EOPNOTSUPP) {
> > + rc = 0;
> > + } else {
> > + if (rc == -E2BIG) {
> > + rc = 0;
> > + left = 0;
> > + toobig = true;
> > + } else if (rc < 0)
> > + return rc;
> > + else
> > + left -= entrysize;
> > +
> > + total += entrysize;
> > + count += rc;
> > + if (single)
> > + goto out;
> > + }
> > + },
> > + /* END BLOCK AFTER INVOCATION */
> > + attr, uctx, &entrysize, flags);
> > +
> > +out:
> >
> > - total += entrysize;
> > - count += rc;
> > - if (single)
> > - break;
> > - }
> > if (put_user(total, size))
> > return -EFAULT;
> > if (toobig)
>
> I think we may need to admit defeat with security_getselfattr() and
> leave it as-is, the above is just too ugly to live. I'd suggest
> adding a comment explaining that it wasn't converted due to complexity
> and the resulting awfulness.
>
I think your position on fixing everything is actually a valid one for
security, which is why I did not contest it.
The security_getselfattr is called very close to the syscall boundary
and the closer to the boundary the call is, the greater control the
attacker has over arguments and the easier it is to mount the attack.
This is why LSM indirect calls are a lucrative target because they
happen fairly early in the transition from user to kernel.
security_getselfattr is literally just in a SYSCALL_DEFINE
From a security perspective we should not leave this open.
- KP
> --
> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 4/5] security: Update non standard hooks to use static calls
2024-07-09 12:36 ` KP Singh
@ 2024-07-09 14:51 ` Paul Moore
2024-07-09 16:53 ` Casey Schaufler
1 sibling, 0 replies; 34+ messages in thread
From: Paul Moore @ 2024-07-09 14:51 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song
On Tue, Jul 9, 2024 at 8:36 AM KP Singh <kpsingh@kernel.org> wrote:
> > > --- a/security/security.c
> > > +++ b/security/security.c
...
> > > -#define lsm_for_each_hook(scall, NAME) \
> > > - for (scall = static_calls_table.NAME; \
> > > - scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> > > - if (static_key_enabled(&scall->active->key))
> > > +/*
> > > + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> > > + * current hook
> > > + */
> > > +#define current_lsmid() _hook_lsmid
> >
> > See my comments below about security_getselfattr(), I think we can drop
> > the current_lsmid() macro. If we really must keep it, we need to rename
> > it to something else as it clashes too much with the other current_XXX()
> > macros/functions which are useful outside of our wacky macros.
>
> call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
> update the name.
>
> What do you think about __security_hook_lsm_id().
I guess we can't get rid of it due to the crazy macro stuff with loop
unrolling, BEFORE/AFTER blocks, etc. Ooof. If you were looking for
another example of why I don't really like these patches, this would
be a good candidate.
More below ...
> > I know I was the one who asked to implement the static_calls for *all*
> > of the LSM functions - thank you for doing that - but I think we can
> > all agree that some of the resulting code is pretty awful. I'm probably
> > missing something important, but would an apporach similar to the pseudo
> > code below work?
>
> This does not work.
>
> The special macro you are defining does not have the static_call
> invocation and if you add that bit it's basically the __CALL_HOOK
> macro or __CALL_STATIC_INT, __CALL_STATIC_VOID macro inlined
> everywhere, I tried implementing it but it gets very dirty.
Thanks for testing it out. Perhaps trying to move all of these hooks
to use the static_call approach was a mistake. I realize you're doing
your best adapting the static_call API to support multiple LSMs, but
it just doesn't look like a good fit to me for the "unconventional"
hooks here in this patch.
> > I think we may need to admit defeat with security_getselfattr() and
> > leave it as-is, the above is just too ugly to live. I'd suggest
> > adding a comment explaining that it wasn't converted due to complexity
> > and the resulting awfulness.
>
> I think your position on fixing everything is actually a valid one for
> security, which is why I did not contest it.
>
> The security_getselfattr is called very close to the syscall boundary
> and the closer to the boundary the call is, the greater control the
> attacker has over arguments and the easier it is to mount the attack.
> This is why LSM indirect calls are a lucrative target because they
> happen fairly early in the transition from user to kernel.
> security_getselfattr is literally just in a SYSCALL_DEFINE
I recognize that your comments are in reference to that last flaw
rooted in the hardware that used indirect calls at an attack vector,
but wasn't that resolved through other means? I never saw the PoC or
had time to follow up on whatever mitigation was ultimately merged (if
any). However, my understanding is that the move to static_calls is
not strictly necessary to patch over that particular hardware flaw, it
is just a we-really-want-this for either a performance or a
non-specific security reason; pick your favorite of the two based on
your audience.
Regardless, since none of the previous suggestions/options proved to
be workable, I'm going to suggest we just kill this patch too and move
forward with the others. I had hoped we could get the changes in this
patch cleaned up, but it doesn't look like that is going to be the
case, or at least not within a week or two, so let's drop it and we
can always reconsider this in the future if a cleaner implementation
is presented.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 4/5] security: Update non standard hooks to use static calls
2024-07-09 12:36 ` KP Singh
2024-07-09 14:51 ` Paul Moore
@ 2024-07-09 16:53 ` Casey Schaufler
2024-07-09 19:05 ` Paul Moore
1 sibling, 1 reply; 34+ messages in thread
From: Casey Schaufler @ 2024-07-09 16:53 UTC (permalink / raw)
To: KP Singh, Paul Moore
Cc: linux-security-module, bpf, ast, andrii, keescook, daniel,
renauld, revest, song, Casey Schaufler
On 7/9/2024 5:36 AM, KP Singh wrote:
> [...]
>
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -948,10 +948,48 @@ out: \
>>> RC; \
>>> })
>>>
>>> -#define lsm_for_each_hook(scall, NAME) \
>>> - for (scall = static_calls_table.NAME; \
>>> - scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
>>> - if (static_key_enabled(&scall->active->key))
>>> +/*
>>> + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
>>> + * current hook
>>> + */
>>> +#define current_lsmid() _hook_lsmid
>> See my comments below about security_getselfattr(), I think we can drop
>> the current_lsmid() macro. If we really must keep it, we need to rename
>> it to something else as it clashes too much with the other current_XXX()
>> macros/functions which are useful outside of our wacky macros.
> call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
> update the name.
>
> What do you think about __security_hook_lsm_id().
I really dislike it. The security prefix (even with __) tells the
developer in security.c that the code is used elsewhere. How about
lsm_hook_current_id()?
>
>>> +#define __CALL_HOOK(NUM, HOOK, RC, BLOCK_BEFORE, BLOCK_AFTER, ...) \
>>> +do { \
>>> + int __maybe_unused _hook_lsmid; \
>>> + \
>>> + if (static_branch_unlikely(&SECURITY_HOOK_ACTIVE_KEY(HOOK, NUM))) { \
>>> + _hook_lsmid = static_calls_table.HOOK[NUM].hl->lsmid->id; \
>>> + BLOCK_BEFORE \
>>> + RC = static_call(LSM_STATIC_CALL(HOOK, NUM))(__VA_ARGS__); \
>>> + BLOCK_AFTER \
>>> + } \
>>> +} while (0);
>>> +
>>> +#define lsm_for_each_hook(HOOK, RC, BLOCK_AFTER, ...) \
>>> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, ;, BLOCK_AFTER, __VA_ARGS__)
>>> +
>>> +#define call_hook_with_lsmid(HOOK, LSMID, ...) \
>>> +({ \
>>> + __label__ out; \
>>> + int RC = LSM_RET_DEFAULT(HOOK); \
>>> + \
>>> + LSM_LOOP_UNROLL(__CALL_HOOK, HOOK, RC, \
>>> + /* BLOCK BEFORE INVOCATION */ \
>>> + { \
>>> + if (current_lsmid() != LSMID) \
>>> + continue; \
>>> + }, \
>>> + /* END BLOCK BEFORE INVOCATION */ \
>>> + /* BLOCK AFTER INVOCATION */ \
>>> + { \
>>> + goto out; \
>>> + }, \
>>> + /* END BLOCK AFTER INVOCATION */ \
>>> + __VA_ARGS__); \
>>> +out: \
>>> + RC; \
>>> +})
>>>
>>> /* Security operations */
>> ...
>>
>>> @@ -1581,15 +1629,19 @@ int security_sb_set_mnt_opts(struct super_block *sb,
>>> unsigned long kern_flags,
>>> unsigned long *set_kern_flags)
>>> {
>>> - struct lsm_static_call *scall;
>>> int rc = mnt_opts ? -EOPNOTSUPP : LSM_RET_DEFAULT(sb_set_mnt_opts);
>>>
>>> - lsm_for_each_hook(scall, sb_set_mnt_opts) {
>>> - rc = scall->hl->hook.sb_set_mnt_opts(sb, mnt_opts, kern_flags,
>>> - set_kern_flags);
>>> - if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
>>> - break;
>>> - }
>>> + lsm_for_each_hook(
>>> + sb_set_mnt_opts, rc,
>>> + /* BLOCK AFTER INVOCATION */
>>> + {
>>> + if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts))
>>> + goto out;
>>> + },
>>> + /* END BLOCK AFTER INVOCATION */
>>> + sb, mnt_opts, kern_flags, set_kern_flags);
>>> +
>>> +out:
>>> return rc;
>>> }
>>> EXPORT_SYMBOL(security_sb_set_mnt_opts);
>> I know I was the one who asked to implement the static_calls for *all*
>> of the LSM functions - thank you for doing that - but I think we can
>> all agree that some of the resulting code is pretty awful. I'm probably
>> missing something important, but would an apporach similar to the pseudo
>> code below work?
> This does not work.
>
> The special macro you are defining does not have the static_call
> invocation and if you add that bit it's basically the __CALL_HOOK
> macro or __CALL_STATIC_INT, __CALL_STATIC_VOID macro inlined
> everywhere, I tried implementing it but it gets very dirty.
>
>> #define call_int_hook_special(HOOK, RC, LABEL, ...) \
>> LSM_LOOP_UNROLL(HOOK##_SPECIAL, RC, HOOK, LABEL, __VA_ARGS__)
>>
>> int security_sb_set_mnt_opts(...)
>> {
>> int rc = LSM_RET_DEFAULT(sb_set_mnt_opts);
>>
>> #define sb_set_mnt_opts_SPECIAL \
>> do { \
>> if (rc != LSM_RET_DEFAULT(sb_set_mnt_opts)) \
>> goto out; \
>> } while (0)
>>
>> rc = call_int_hook_special(sb_set_mnt_opts, rc, out, ...);
>>
>> out:
>> return rc;
>> }
>>
>>> @@ -4040,7 +4099,6 @@ EXPORT_SYMBOL(security_d_instantiate);
>>> int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>>> u32 __user *size, u32 flags)
>>> {
>>> - struct lsm_static_call *scall;
>>> struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
>>> u8 __user *base = (u8 __user *)uctx;
>>> u32 entrysize;
>>> @@ -4078,31 +4136,42 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
>>> * In the usual case gather all the data from the LSMs.
>>> * In the single case only get the data from the LSM specified.
>>> */
>>> - lsm_for_each_hook(scall, getselfattr) {
>>> - if (single && lctx.id != scall->hl->lsmid->id)
>>> - continue;
>>> - entrysize = left;
>>> - if (base)
>>> - uctx = (struct lsm_ctx __user *)(base + total);
>>> - rc = scall->hl->hook.getselfattr(attr, uctx, &entrysize, flags);
>>> - if (rc == -EOPNOTSUPP) {
>>> - rc = 0;
>>> - continue;
>>> - }
>>> - if (rc == -E2BIG) {
>>> - rc = 0;
>>> - left = 0;
>>> - toobig = true;
>>> - } else if (rc < 0)
>>> - return rc;
>>> - else
>>> - left -= entrysize;
>>> + LSM_LOOP_UNROLL(
>>> + __CALL_HOOK, getselfattr, rc,
>>> + /* BLOCK BEFORE INVOCATION */
>>> + {
>>> + if (single && lctx.id != current_lsmid())
>>> + continue;
>>> + entrysize = left;
>>> + if (base)
>>> + uctx = (struct lsm_ctx __user *)(base + total);
>>> + },
>>> + /* END BLOCK BEFORE INVOCATION */
>>> + /* BLOCK AFTER INVOCATION */
>>> + {
>>> + if (rc == -EOPNOTSUPP) {
>>> + rc = 0;
>>> + } else {
>>> + if (rc == -E2BIG) {
>>> + rc = 0;
>>> + left = 0;
>>> + toobig = true;
>>> + } else if (rc < 0)
>>> + return rc;
>>> + else
>>> + left -= entrysize;
>>> +
>>> + total += entrysize;
>>> + count += rc;
>>> + if (single)
>>> + goto out;
>>> + }
>>> + },
>>> + /* END BLOCK AFTER INVOCATION */
>>> + attr, uctx, &entrysize, flags);
>>> +
>>> +out:
>>>
>>> - total += entrysize;
>>> - count += rc;
>>> - if (single)
>>> - break;
>>> - }
>>> if (put_user(total, size))
>>> return -EFAULT;
>>> if (toobig)
>> I think we may need to admit defeat with security_getselfattr() and
>> leave it as-is, the above is just too ugly to live. I'd suggest
>> adding a comment explaining that it wasn't converted due to complexity
>> and the resulting awfulness.
>>
> I think your position on fixing everything is actually a valid one for
> security, which is why I did not contest it.
>
> The security_getselfattr is called very close to the syscall boundary
> and the closer to the boundary the call is, the greater control the
> attacker has over arguments and the easier it is to mount the attack.
> This is why LSM indirect calls are a lucrative target because they
> happen fairly early in the transition from user to kernel.
> security_getselfattr is literally just in a SYSCALL_DEFINE
>
> >From a security perspective we should not leave this open.
>
> - KP
>
>> --
>> paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v13 4/5] security: Update non standard hooks to use static calls
2024-07-09 16:53 ` Casey Schaufler
@ 2024-07-09 19:05 ` Paul Moore
0 siblings, 0 replies; 34+ messages in thread
From: Paul Moore @ 2024-07-09 19:05 UTC (permalink / raw)
To: Casey Schaufler
Cc: KP Singh, linux-security-module, bpf, ast, andrii, keescook,
daniel, renauld, revest, song
On Tue, Jul 9, 2024 at 12:53 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/9/2024 5:36 AM, KP Singh wrote:
> > [...]
> >
> >>> --- a/security/security.c
> >>> +++ b/security/security.c
> >>> @@ -948,10 +948,48 @@ out: \
> >>> RC; \
> >>> })
> >>>
> >>> -#define lsm_for_each_hook(scall, NAME) \
> >>> - for (scall = static_calls_table.NAME; \
> >>> - scall - static_calls_table.NAME < MAX_LSM_COUNT; scall++) \
> >>> - if (static_key_enabled(&scall->active->key))
> >>> +/*
> >>> + * Can be used in the context passed to lsm_for_each_hook to get the lsmid of the
> >>> + * current hook
> >>> + */
> >>> +#define current_lsmid() _hook_lsmid
> >> See my comments below about security_getselfattr(), I think we can drop
> >> the current_lsmid() macro. If we really must keep it, we need to rename
> >> it to something else as it clashes too much with the other current_XXX()
> >> macros/functions which are useful outside of our wacky macros.
> > call_hook_with_lsmid is a pattern used by quite a few hooks, happy to
> > update the name.
> >
> > What do you think about __security_hook_lsm_id().
>
> I really dislike it. The security prefix (even with __) tells the
> developer in security.c that the code is used elsewhere. How about
> lsm_hook_current_id()?
See my reply. There is enough ugliness in converting the hooks in
this particular patch that I think we need to shelve this patch too.
--
paul-moore.com
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2024-07-09 19:05 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-29 8:43 [PATCH v13 0/5] Reduce overhead of LSMs with static calls KP Singh
2024-06-29 8:43 ` [PATCH v13 1/5] kernel: Add helper macros for loop unrolling KP Singh
2024-06-29 8:43 ` [PATCH v13 2/5] security: Count the LSMs enabled at compile time KP Singh
2024-07-03 9:44 ` Rasmus Villemoes
2024-07-03 13:12 ` KP Singh
2024-07-03 14:54 ` Paul Moore
2024-06-29 8:43 ` [PATCH v13 3/5] security: Replace indirect LSM hook calls with static calls KP Singh
2024-07-03 0:07 ` Paul Moore
2024-07-03 16:54 ` KP Singh
2024-07-03 20:56 ` Paul Moore
2024-07-03 22:22 ` KP Singh
2024-07-03 22:52 ` Paul Moore
2024-07-03 23:08 ` KP Singh
2024-07-03 23:44 ` Casey Schaufler
2024-07-04 0:24 ` KP Singh
2024-07-04 1:15 ` KP Singh
2024-07-05 18:07 ` Paul Moore
2024-07-05 19:34 ` KP Singh
2024-07-06 0:17 ` Kees Cook
2024-07-06 4:46 ` Paul Moore
2024-07-06 4:40 ` Paul Moore
2024-07-08 10:04 ` KP Singh
2024-07-08 12:52 ` Paul Moore
2024-07-08 13:52 ` KP Singh
2024-07-08 14:23 ` Paul Moore
2024-06-29 8:43 ` [PATCH v13 4/5] security: Update non standard hooks to use " KP Singh
2024-07-03 0:07 ` Paul Moore
2024-07-09 12:36 ` KP Singh
2024-07-09 14:51 ` Paul Moore
2024-07-09 16:53 ` Casey Schaufler
2024-07-09 19:05 ` Paul Moore
2024-06-29 8:43 ` [PATCH v13 5/5] bpf: Only enable BPF LSM hooks when an LSM program is attached KP Singh
2024-07-03 0:07 ` Paul Moore
2024-07-03 16:55 ` KP Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).