All of lore.kernel.org
 help / color / mirror / Atom feed
From: KP Singh <kpsingh@kernel.org>
To: linux-security-module@vger.kernel.org, bpf@vger.kernel.org
Cc: ast@kernel.org, daniel@iogearbox.net, jackmanb@google.com,
	renauld@google.com, paul@paul-moore.com, casey@schaufler-ca.com,
	song@kernel.org, revest@chromium.org, keescook@chromium.org,
	KP Singh <kpsingh@kernel.org>
Subject: [PATCH RESEND bpf-next 4/4] bpf: Only enable BPF LSM hooks when an LSM program is attached
Date: Fri, 20 Jan 2023 01:08:18 +0100	[thread overview]
Message-ID: <20230120000818.1324170-5-kpsingh@kernel.org> (raw)
In-Reply-To: <20230120000818.1324170-1-kpsingh@kernel.org>

BPF LSM hooks have side-effects (even when a default value is returned),
as some hooks end up behaving differently due to the very presence of
the hook.

The static keys guarding the BPF LSM hooks are disabled by default and
enabled only when a BPF program is attached implementing the hook
logic. This avoids the issue of the side-effects and also the minor
overhead associated with the empty callback.

security_file_ioctl:
   0xffffffff814f0e90 <+0>:	endbr64
   0xffffffff814f0e94 <+4>:	push   %rbp
   0xffffffff814f0e95 <+5>:	push   %r14
   0xffffffff814f0e97 <+7>:	push   %rbx
   0xffffffff814f0e98 <+8>:	mov    %rdx,%rbx
   0xffffffff814f0e9b <+11>:	mov    %esi,%ebp
   0xffffffff814f0e9d <+13>:	mov    %rdi,%r14

>>> 0xffffffff814f0ea0 <+16>:	jmp    0xffffffff814f0eb1 <security_file_ioctl+33>

   Static key enabled for SELinux

   0xffffffff814f0ea2 <+18>:	xchg   %ax,%ax

   Static key disabled for BPF. This gets patched to a jmp when a BPF
   LSM program is attached.

   0xffffffff814f0ea4 <+20>:	xor    %eax,%eax
   0xffffffff814f0ea6 <+22>:	xchg   %ax,%ax
   0xffffffff814f0ea8 <+24>:	pop    %rbx
   0xffffffff814f0ea9 <+25>:	pop    %r14
   0xffffffff814f0eab <+27>:	pop    %rbp
   0xffffffff814f0eac <+28>:	jmp    0xffffffff81f767c4 <__x86_return_thunk>
   0xffffffff814f0eb1 <+33>:	endbr64
   0xffffffff814f0eb5 <+37>:	mov    %r14,%rdi
   0xffffffff814f0eb8 <+40>:	mov    %ebp,%esi
   0xffffffff814f0eba <+42>:	mov    %rbx,%rdx
   0xffffffff814f0ebd <+45>:	call   0xffffffff814fe8e0 <selinux_file_ioctl>
   0xffffffff814f0ec2 <+50>:	test   %eax,%eax
   0xffffffff814f0ec4 <+52>:	jne    0xffffffff814f0ea8 <security_file_ioctl+24>
   0xffffffff814f0ec6 <+54>:	jmp    0xffffffff814f0ea2 <security_file_ioctl+18>
   0xffffffff814f0ec8 <+56>:	endbr64
   0xffffffff814f0ecc <+60>:	mov    %r14,%rdi
   0xffffffff814f0ecf <+63>:	mov    %ebp,%esi
   0xffffffff814f0ed1 <+65>:	mov    %rbx,%rdx
   0xffffffff814f0ed4 <+68>:	call   0xffffffff8123b890 <bpf_lsm_file_ioctl>
   0xffffffff814f0ed9 <+73>:	test   %eax,%eax
   0xffffffff814f0edb <+75>:	jne    0xffffffff814f0ea8 <security_file_ioctl+24>
   0xffffffff814f0edd <+77>:	jmp    0xffffffff814f0ea4 <security_file_ioctl+20>
   0xffffffff814f0edf <+79>:	endbr64
   0xffffffff814f0ee3 <+83>:	mov    %r14,%rdi
   0xffffffff814f0ee6 <+86>:	mov    %ebp,%esi
   0xffffffff814f0ee8 <+88>:	mov    %rbx,%rdx
   0xffffffff814f0eeb <+91>:	pop    %rbx
   0xffffffff814f0eec <+92>:	pop    %r14
   0xffffffff814f0eee <+94>:	pop    %rbp
   0xffffffff814f0eef <+95>:	ret
   0xffffffff814f0ef0 <+96>:	int3
   0xffffffff814f0ef1 <+97>:	int3
   0xffffffff814f0ef2 <+98>:	int3
   0xffffffff814f0ef3 <+99>:	int3

Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/bpf.h       |  1 +
 include/linux/bpf_lsm.h   |  1 +
 include/linux/lsm_hooks.h | 13 ++++++++++++-
 kernel/bpf/trampoline.c   | 29 +++++++++++++++++++++++++++--
 security/bpf/hooks.c      | 26 +++++++++++++++++++++++++-
 security/security.c       |  3 ++-
 6 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index ae7771c7d750..4008f4a00851 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1049,6 +1049,7 @@ struct bpf_attach_target_info {
 	long tgt_addr;
 	const char *tgt_name;
 	const struct btf_type *tgt_type;
+	bool is_lsm_target;
 };
 
 #define BPF_DISPATCHER_MAX 48 /* Fits in 2048B */
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 1de7ece5d36d..cce615af93d5 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -29,6 +29,7 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
 
 bool bpf_lsm_is_sleepable_hook(u32 btf_id);
 bool bpf_lsm_is_trusted(const struct bpf_prog *prog);
+void bpf_lsm_toggle_hook(void *addr, bool value);
 
 static inline struct bpf_storage_blob *bpf_inode(
 	const struct inode *inode)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c82d15a4ef50..5e85d3340a07 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1716,11 +1716,14 @@ struct lsm_static_calls_table {
  * @scalls: The beginning of the array of static calls assigned to this hook.
  * @hook: The callback for the hook.
  * @lsm: The name of the lsm that owns this hook.
+ * @default_state: The state of the LSM hook when initialized. If set to false,
+ * the static key guarding the hook will be set to disabled.
  */
 struct security_hook_list {
 	struct lsm_static_call	*scalls;
 	union security_list_options	hook;
 	const char			*lsm;
+	bool				default_state;
 } __randomize_layout;
 
 /*
@@ -1751,7 +1754,15 @@ struct lsm_blob_sizes {
 #define LSM_HOOK_INIT(NAME, CALLBACK)			\
 	{						\
 		.scalls = static_calls_table.NAME,	\
-		.hook = { .NAME = CALLBACK }		\
+		.hook = { .NAME = CALLBACK },		\
+		.default_state = true			\
+	}
+
+#define LSM_HOOK_INIT_DISABLED(NAME, CALLBACK)		\
+	{						\
+		.scalls = static_calls_table.NAME,	\
+		.hook = { .NAME = CALLBACK },		\
+		.default_state = false			\
 	}
 
 extern char *lsm_names;
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index d0ed7d6f5eec..9789ecf6f29c 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -14,6 +14,7 @@
 #include <linux/bpf_verifier.h>
 #include <linux/bpf_lsm.h>
 #include <linux/delay.h>
+#include <linux/bpf_lsm.h>
 
 /* dummy _ops. The verifier will operate on target program's ops. */
 const struct bpf_verifier_ops bpf_extension_verifier_ops = {
@@ -536,7 +537,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
 {
 	enum bpf_tramp_prog_type kind;
 	struct bpf_tramp_link *link_exiting;
-	int err = 0;
+	int err = 0, num_lsm_progs = 0;
 	int cnt = 0, i;
 
 	kind = bpf_attach_type_to_tramp(link->link.prog);
@@ -567,8 +568,14 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
 			continue;
 		/* prog already linked */
 		return -EBUSY;
+
+		if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM)
+			num_lsm_progs++;
 	}
 
+	if (!num_lsm_progs && link->link.prog->type == BPF_PROG_TYPE_LSM)
+		bpf_lsm_toggle_hook(tr->func.addr, true);
+
 	hlist_add_head(&link->tramp_hlist, &tr->progs_hlist[kind]);
 	tr->progs_cnt[kind]++;
 	err = bpf_trampoline_update(tr, true /* lock_direct_mutex */);
@@ -591,8 +598,10 @@ int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline
 
 static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr)
 {
+	struct bpf_tramp_link *link_exiting;
 	enum bpf_tramp_prog_type kind;
-	int err;
+	bool lsm_link_found = false;
+	int err, num_lsm_progs = 0;
 
 	kind = bpf_attach_type_to_tramp(link->link.prog);
 	if (kind == BPF_TRAMP_REPLACE) {
@@ -602,8 +611,24 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
 		tr->extension_prog = NULL;
 		return err;
 	}
+
+	if (link->link.prog->type == BPF_PROG_TYPE_LSM) {
+		hlist_for_each_entry(link_exiting, &tr->progs_hlist[kind],
+				     tramp_hlist) {
+			if (link_exiting->link.prog->type == BPF_PROG_TYPE_LSM)
+				num_lsm_progs++;
+
+			if (link_exiting->link.prog == link->link.prog)
+				lsm_link_found = true;
+		}
+	}
+
 	hlist_del_init(&link->tramp_hlist);
 	tr->progs_cnt[kind]--;
+
+	if (lsm_link_found && num_lsm_progs == 1)
+		bpf_lsm_toggle_hook(tr->func.addr, false);
+
 	return bpf_trampoline_update(tr, true /* lock_direct_mutex */);
 }
 
diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
index e5971fa74fd7..a39799e9f648 100644
--- a/security/bpf/hooks.c
+++ b/security/bpf/hooks.c
@@ -8,7 +8,7 @@
 
 static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
 	#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
-	LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
+	LSM_HOOK_INIT_DISABLED(NAME, bpf_lsm_##NAME),
 	#include <linux/lsm_hook_defs.h>
 	#undef LSM_HOOK
 	LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
@@ -32,3 +32,27 @@ DEFINE_LSM(bpf) = {
 	.init = bpf_lsm_init,
 	.blobs = &bpf_lsm_blob_sizes
 };
+
+void bpf_lsm_toggle_hook(void *addr, bool value)
+{
+	struct security_hook_list *h;
+	struct lsm_static_call *scalls;
+	int i, j;
+
+	for (i = 0; i < ARRAY_SIZE(bpf_lsm_hooks); i++) {
+		h = &bpf_lsm_hooks[i];
+		scalls = h->scalls;
+		if (h->hook.lsm_callback == addr)
+			continue;
+
+		for (j = 0; j < MAX_LSM_COUNT; j++) {
+			if (scalls[j].hl != h)
+				continue;
+
+			if (value)
+				static_key_enable(scalls[j].enabled_key);
+			else
+				static_key_disable(scalls[j].enabled_key);
+		}
+	}
+}
diff --git a/security/security.c b/security/security.c
index e54d5ba187d1..f74135349429 100644
--- a/security/security.c
+++ b/security/security.c
@@ -366,7 +366,8 @@ static void __init lsm_static_call_init(struct security_hook_list *hl)
 		if (!scall->hl) {
 			__static_call_update(scall->key, scall->trampoline, hl->hook.lsm_callback);
 			scall->hl = hl;
-			static_key_enable(scall->enabled_key);
+			if (hl->default_state)
+				static_key_enable(scall->enabled_key);
 			return;
 		}
 		scall++;
-- 
2.39.0.246.g2a6d74b583-goog


      parent reply	other threads:[~2023-01-20  0:09 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-20  0:08 [PATCH RESEND bpf-next 0/4] Reduce overhead of LSMs with static calls KP Singh
2023-01-20  0:08 ` [PATCH RESEND bpf-next 1/4] kernel: Add helper macros for loop unrolling KP Singh
2023-01-20  3:48   ` Kees Cook
2023-01-20  0:08 ` [PATCH RESEND bpf-next 2/4] security: Generate a header with the count of enabled LSMs KP Singh
2023-01-20  4:04   ` Kees Cook
2023-01-20  7:33   ` kernel test robot
2023-01-20  9:50   ` kernel test robot
2023-01-20  9:50   ` kernel test robot
2023-01-20  0:08 ` [PATCH RESEND bpf-next 3/4] security: Replace indirect LSM hook calls with static calls KP Singh
2023-01-20  4:36   ` Kees Cook
2023-01-20 18:26     ` Casey Schaufler
2023-02-06 13:04     ` KP Singh
2023-02-06 16:29       ` Casey Schaufler
2023-02-06 17:48         ` Song Liu
2023-02-06 18:19           ` KP Singh
2023-02-06 18:29           ` Casey Schaufler
2023-02-06 18:41             ` KP Singh
2023-02-06 18:50               ` Kees Cook
2023-06-08  2:48                 ` KP Singh
2023-06-13 21:43                   ` Paul Moore
2023-06-13 22:03                     ` KP Singh
2023-02-06 19:05             ` Song Liu
2023-02-06 20:11               ` Casey Schaufler
2023-01-20 10:10   ` kernel test robot
2023-01-20 10:41   ` kernel test robot
2023-01-20  0:08 ` KP Singh [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230120000818.1324170-5-kpsingh@kernel.org \
    --to=kpsingh@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=daniel@iogearbox.net \
    --cc=jackmanb@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=renauld@google.com \
    --cc=revest@chromium.org \
    --cc=song@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.