[bpf-next,v1,10/13] bpf: lsm: Handle attachment of the same program
diff mbox series

Message ID 20191220154208.15895-11-kpsingh@chromium.org
State New
Headers show
Series
  • MAC and Audit policy using eBPF (KRSI)
Related show

Commit Message

KP Singh Dec. 20, 2019, 3:42 p.m. UTC
From: KP Singh <kpsingh@google.com>

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

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

./loader Attaches "env_dumper__v1" followed by "env_dumper__v2"
to the bprm_check_security hook..

./loader
./loader

Before:

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

After:

  cat /sys/kernel/security/bpf/process_execution
  env_dumper__v2

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

Comments

Andrii Nakryiko Dec. 24, 2019, 6:38 a.m. UTC | #1
On Fri, Dec 20, 2019 at 7:42 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> Allow userspace to attach a newer version of a program without having
> duplicates of the same program.
>
> If BPF_F_ALLOW_OVERRIDE is passed, the attachment logic compares the
> name of the new program to the names of existing attached programs. The
> names are only compared till a "__" (or '\0', if there is no "__"). If
> a successful match is found, the existing program is replaced with the
> newer attachment.
>
> ./loader Attaches "env_dumper__v1" followed by "env_dumper__v2"
> to the bprm_check_security hook..
>
> ./loader
> ./loader
>
> Before:
>
>   cat /sys/kernel/security/bpf/process_execution
>   env_dumper__v1
>   env_dumper__v2
>
> After:
>
>   cat /sys/kernel/security/bpf/process_execution
>   env_dumper__v2
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---


Andrey Ignatov just posted patch set few days ago solving similar
problem for cgroup BPF programs. His approach was to actually also
specify FD of BPF program to be replaced. This seems like a more
reliable way than doing this based on name only. Please take a look at
that patch and see if same approach can work for your use case.

>  security/bpf/ops.c | 57 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
>

[...]
James Morris Jan. 8, 2020, 6:21 p.m. UTC | #2
On Fri, 20 Dec 2019, KP Singh wrote:

> From: KP Singh <kpsingh@google.com>
> 
> Allow userspace to attach a newer version of a program without having
> duplicates of the same program.
> 
> If BPF_F_ALLOW_OVERRIDE is passed, the attachment logic compares the
> name of the new program to the names of existing attached programs. The
> names are only compared till a "__" (or '\0', if there is no "__"). If
> a successful match is found, the existing program is replaced with the
> newer attachment.
> 
> ./loader Attaches "env_dumper__v1" followed by "env_dumper__v2"
> to the bprm_check_security hook..
> 
> ./loader
> ./loader
> 
> Before:
> 
>   cat /sys/kernel/security/bpf/process_execution
>   env_dumper__v1
>   env_dumper__v2
> 
> After:
> 
>   cat /sys/kernel/security/bpf/process_execution
>   env_dumper__v2
> 
> Signed-off-by: KP Singh <kpsingh@google.com>


Reviewed-by: James Morris <jamorris@linux.microsoft.com>

Patch
diff mbox series

diff --git a/security/bpf/ops.c b/security/bpf/ops.c
index e9aae2ce718c..481e6ee75f27 100644
--- a/security/bpf/ops.c
+++ b/security/bpf/ops.c
@@ -64,11 +64,52 @@  static struct bpf_lsm_hook *get_hook_from_fd(int fd)
 	return ERR_PTR(ret);
 }
 
+/*
+ * match_prog_name matches the name of the program till "__"
+ * or the end of the string is encountered. This allows
+ * the matched program to be replaced by a newer version.
+ *
+ * For example:
+ *
+ *	env_dumper__v1 is matched with env_dumper__v2
+ *
+ */
+static bool match_prog_name(const char *a, const char *b)
+{
+	size_t m, n;
+	char *end;
+
+	end = strstr(a, "__");
+	n = end ? end - a : strlen(a);
+
+	end = strstr(b, "__");
+	m = end ? end - b : strlen(b);
+
+	if (m != n)
+		return false;
+
+	return strncmp(a, b, n) == 0;
+}
+
+static struct bpf_prog *find_attached_prog(struct bpf_prog_array *array,
+					   struct bpf_prog *prog)
+{
+	struct bpf_prog_array_item *item = array->items;
+
+	for (; item->prog; item++) {
+		if (match_prog_name(item->prog->aux->name, prog->aux->name))
+			return item->prog;
+	}
+
+	return NULL;
+}
+
 int bpf_lsm_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	struct bpf_prog_array *old_array;
 	struct bpf_prog_array *new_array;
 	struct bpf_lsm_hook *h;
+	struct bpf_prog *old_prog = NULL;
 	int ret = 0;
 
 	h = get_hook_from_fd(attr->target_fd);
@@ -78,13 +119,27 @@  int bpf_lsm_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 	mutex_lock(&h->mutex);
 	old_array = rcu_dereference_protected(h->progs,
 					      lockdep_is_held(&h->mutex));
+	/*
+	 * Check if a matching program already exists and replace
+	 * the existing program if BPF_F_ALLOW_OVERRIDE is specified in
+	 * the attach flags.
+	 */
+	if (old_array) {
+		old_prog = find_attached_prog(old_array, prog);
+		if (old_prog && !(attr->attach_flags & BPF_F_ALLOW_OVERRIDE)) {
+			ret = -EEXIST;
+			goto unlock;
+		}
+	}
 
-	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
+	ret = bpf_prog_array_copy(old_array, old_prog, prog, &new_array);
 	if (ret < 0)
 		goto unlock;
 
 	rcu_assign_pointer(h->progs, new_array);
 	bpf_prog_array_free(old_array);
+	if (old_prog)
+		bpf_prog_put(old_prog);
 
 unlock:
 	mutex_unlock(&h->mutex);