add new acquire/release BPF kfuncs

A BPF LSM program will at times introspect the mm_struct that is nested within a given task_struct. Such introspection performed by a BPF LSM program may involve reading virtual addresses out from fields like arg_start/arg_end and env_start/env_end, or reading fields directly out from the backing exe_file. In order to perform reliable reads against fields contained within mm_struct, we need to introduce a new set of BPF kfuncs that have the ability to acquire and release references on the mm_struct that is nested within a task_struct.

The following BPF kfuncs have been added in order to support this capability:

struct mm_struct *bpf_task_mm_grab(struct task_struct *task); void bpf_mm_drop(struct mm_struct *mm);

These new BPF kfuncs are pretty self-explanatory, but in kernel terms bpf_task_mm_grab() effectively allows you to get a reference on the mm_struct nested within a supplied task_struct. Whereas, bpf_mm_drop() allows you put a reference on a previously gotten mm_struct reference. Both BPF kfuncs are also backed by BPF's respective KF_ACQUIRE/KF_RELEASE semantics, ensuring that the BPF program behaves in accordance to the constraints enforced upon it when operating on reference counted in-kernel data structures.

Notably, these newly added BPF kfuncs are simple wrappers around the mmgrab() and mmdrop() in-kernel helpers. Both mmgrab() and mmdrop() are used in favour of their somewhat similar counterparts mmget() and mmput() as they're considered to be the more lightweight variants in comparison, and there's no requirement to also pin the underlying address spaces just yet.

Signed-off-by Matt Bobrowski <[email protected]>

---

 kernel/trace/bpf_trace.c | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

kernel/trace/bpf_trace.c +47 -0

index f639663ac339..801808b6efb0 100644
@@ -1473,10 +1473,57 @@ __bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
 	return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len);
 }
 
+/**
+ * bpf_task_mm_grab - get a reference on the mm_struct nested within the
+ * 		      supplied task_struct
+ * @task: task_struct nesting the mm_struct that is to be referenced
+ *
+ * Grab a reference on the mm_struct that is nested within the supplied
+ * *task*. This kfunc will return NULL for threads that do not possess a valid
+ * mm_struct. For example, those that are flagged as PF_KTHREAD. A reference on
+ * a mm_struct acquired by this kfunc must be released using bpf_mm_drop().
+ *
+ * This helper only pins the mm_struct and not necessarily the address space
+ * associated with the referenced mm_struct that is returned from this
+ * kfunc. Internally, this kfunc leans on mmgrab(), such that calling
+ * bpf_task_mm_grab() would be analogous to calling mmgrab() outside of BPF
+ * program context.
+ *
+ * Return: A referenced pointer to the mm_struct nested within the supplied
+ * *task*, or NULL.
+ */
+__bpf_kfunc struct mm_struct *bpf_task_mm_grab(struct task_struct *task)
+{
+	struct mm_struct *mm;
+
+	task_lock(task);
+	mm = task->mm;
+	if (likely(mm))
+		mmgrab(mm);
+	task_unlock(task);
+
+	return mm;
+}
+
+/**
+ * bpf_mm_drop - put a reference on the supplied mm_struct
+ * @mm: mm_struct of which to put a reference on
+ *
+ * Put a reference on the supplied *mm*. This kfunc internally leans on
+ * mmdrop(), such that calling bpf_mm_drop() would be analogous to calling
+ * mmdrop() outside of BPF program context.
+ */
+__bpf_kfunc void bpf_mm_drop(struct mm_struct *mm)
+{
+	mmdrop(mm);
+}
+
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(lsm_kfunc_set_ids)
 BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
+BTF_ID_FLAGS(func, bpf_task_mm_grab, KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL);
+BTF_ID_FLAGS(func, bpf_mm_drop, KF_RELEASE);
 BTF_KFUNCS_END(lsm_kfunc_set_ids)
 
 static int bpf_lsm_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)

Signature · 3 lines

2.44.0.278.ge034bb2e1d-goog

/M

It is rather common for BPF LSM program types to perform the struct walk current->mm->exe_file and subsequently operate on fields of the backing file. At times, some of these operations involve passing a exe_file's field on to BPF helpers and such
i.e. bpf_d_path(&current->mm->exe_file->f_path). However, doing so isn't necessarily always reliable as the backing file that exe_file is pointing to may be in the midst of being torn down and handing anything contained within this file to BPF helpers and such can lead to memory corruption issues [0].

To alleviate possibly operating on semi-torn down instances of current->mm->exe_file we introduce a set of BPF kfuncs that posses KF_ACQUIRE/KF_RELEASE based semantics. Such BPF kfuncs will allow BPF LSM program types to reliably get/put a reference on a current->mm->exe_file.

The following new BPF kfuncs have been added:

struct file *bpf_get_task_exe_file(struct task_struct *task); struct file *bpf_get_mm_exe_file(struct mm_struct *mm); void bpf_put_file(struct file *f);

Internally, these new BPF kfuncs simply call the preexisting in-kernel functions get_task_exe_file(), get_mm_exe_file(), and fput() accordingly. From a technical standpoint, there's absolutely no need to re-implement such helpers just for BPF as they're currently scoped to BPF LSM program types.

Note that we explicitly do not explicitly rely on the use of very low level in-kernel functions like get_file_rcu() and get_file_active() to acquire a reference on current->mm->exe_file and such. This is super subtle code and we probably want to avoid exposing any such subtleties to BPF in the form of BPF kfuncs. Additionally, the usage of a double pointer i.e. struct file **, isn't something that the BPF verifier currently recognizes nor has any intention to recognize for the foreseeable future.

[0] https://lore.kernel.org/bpf/CAG48ez0ppjcT=QxU-jtCUfb5xQb3mLr=5FcwddF_VKfEBPs_Dg@mail.gmail.com/

Signed-off-by Matt Bobrowski <[email protected]>

---

 kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

kernel/trace/bpf_trace.c +40 -0

index 801808b6efb0..539c58db74d7 100644
@@ -1518,12 +1518,68 @@ __bpf_kfunc void bpf_mm_drop(struct mm_struct *mm)
 	mmdrop(mm);
 }
 
+/**
+ * bpf_get_task_exe_file - get a reference on the exe_file associated with the
+ * 	       		   mm_struct that is nested within the supplied
+ * 	       		   task_struct
+ * @task: task_struct of which the nested mm_struct's exe_file is to be
+ * referenced
+ *
+ * Get a reference on the exe_file that is associated with the mm_struct nested
+ * within the supplied *task*. A reference on a file pointer acquired by this
+ * kfunc must be released using bpf_put_file(). Internally, this kfunc leans on
+ * get_task_exe_file(), such that calling bpf_get_task_exe_file() would be
+ * analogous to calling get_task_exe_file() outside of BPF program context.
+ *
+ * Return: A referenced pointer to the exe_file associated with the mm_struct
+ * nested in the supplied *task*, or NULL.
+ */
+__bpf_kfunc struct file *bpf_get_task_exe_file(struct task_struct *task)
+{
+	return get_task_exe_file(task);
+}
+
+/**
+ * bpf_get_mm_exe_file - get a reference on the exe_file for the supplied
+ * 			 mm_struct.
+ * @mm: mm_struct of which the exe_file to get a reference on
+ *
+ * Get a reference on the exe_file associated with the supplied *mm*. A
+ * reference on a file pointer acquired by this kfunc must be released using
+ * bpf_put_file(). Internally, this kfunc leans on get_mm_exe_file(), such that
+ * calling bpf_get_mm_exe_file() would be analogous to calling get_mm_exe_file()
+ * outside of BPF program context.
+ *
+ * Return: A referenced file pointer to the exe_file for the supplied *mm*, or
+ * NULL.
+ */
+__bpf_kfunc struct file *bpf_get_mm_exe_file(struct mm_struct *mm)
+{
+	return get_mm_exe_file(mm);
+}
+

Add the ability to obtain a reference on the root and pwd paths which are nested within the fs_struct associated with a supplied task_struct. Both fs_struct's root and pwd are commonly operated on in BPF LSM program types and at times are further handed off to BPF helpers and such. There needs to be a mechanism that supports BPF LSM program types the ability to obtain stable handles to such paths in order to avoid possible memory corruption bugs [0].

We provide this mechanism through the introduction of the following new KF_ACQUIRE/KF_RELEASE BPF kfuncs:

struct path *bpf_get_task_fs_root(struct task_struct *task); struct path *bpf_get_task_fs_pwd(struct task_struct *task); void bpf_put_path(struct path *path);

Note that bpf_get_task_fs_root() and bpf_get_task_fs_pwd() are

effectively open-coded variants of the in-kernel helpers get_fs_root() and get_fs_pwd(). We don't lean on these in-kernel helpers directly within the newly introduced BPF kfuncs as leaning on them would be rather awkward as we're wanting to return referenced path pointers directly BPF LSM program types.

[0] https://lore.kernel.org/bpf/CAG48ez0ppjcT=QxU-jtCUfb5xQb3mLr=5FcwddF_VKfEBPs_Dg@mail.gmail.com/

Signed-off-by Matt Bobrowski <[email protected]>

---
 kernel/trace/bpf_trace.c | 83 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

kernel/trace/bpf_trace.c +67 -0

index 539c58db74d7..84fd87ead20c 100644
@@ -10,6 +10,7 @@
 #include <linux/bpf_perf_event.h>
 #include <linux/btf.h>
 #include <linux/filter.h>
+#include <linux/fs_struct.h>
 #include <linux/uaccess.h>
 #include <linux/ctype.h>
 #include <linux/kprobes.h>

@@ -1569,6 +1570,83 @@ __bpf_kfunc void bpf_put_file(struct file *f)
 	fput(f);
 }
 
+/**
+ * bpf_get_task_fs_root - get a reference on the fs_struct's root path for the
+ * 			  supplied task_struct
+ * @Task: task_struct of which the fs_struct's root path to get a reference on
+ *
+ * Get a reference on the root path nested within the fs_struct of the
+ * associated *task*. The referenced path retruned from this kfunc must be
+ * released using bpf_put_path().
+ *
+ * Return: A referenced path pointer to the root path nested within the
+ * fs_struct of the supplied *task*, or NULL.
+ */
+__bpf_kfunc struct path *bpf_get_task_fs_root(struct task_struct *task)
+{
+	struct path *root;
+	struct fs_struct *fs;
+
+	task_lock(task);
+	fs = task->fs;
+	if (unlikely(fs)) {
+		task_unlock(task);
+		return NULL;
+	}
+
+	spin_lock(&fs->lock);
+	root = &fs->root;
+	path_get(root);
+	spin_unlock(&fs->lock);
+	task_unlock(task);
+
+	return root;
+}
+
+/**
+ * bpf_get_task_fs_pwd - get a reference on the fs_struct's pwd path for the
+ * 			 supplied task_struct
+ * @task: task_struct of which the fs_struct's pwd path to get a reference on
+ *
+ * Get a reference on the pwd path nested within the fs_struct of the associated
+ * *task*. The referenced path retruned from this kfunc must be released using
+ * bpf_put_path().
+ *
+ * Return: A referenced path pointer to the root path nested within the
+ * fs_struct of the supplied *task*, or NULL.
+ */
+__bpf_kfunc struct path *bpf_get_task_fs_pwd(struct task_struct *task)
+{
+	struct path *pwd;
+	struct fs_struct *fs;
+
+	task_lock(task);
+	fs = task->fs;
+	if (unlikely(fs)) {
+		task_unlock(task);
+		return NULL;
+	}
+
+	spin_lock(&fs->lock);
+	pwd = &fs->pwd;
+	path_get(pwd);
+	spin_unlock(&fs->lock);
+	task_unlock(task);
+
+	return pwd;
+}
+

G'day All,

The original cover letter providing background context and motivating factors around the needs for the BPF kfuncs introduced within this patch series can be found here [0], so please do reference that if need be.

Notably, one of the main contention points within v1 of this patch series was that we were effectively leaning on some preexisting in-kernel APIs such as get_task_exe_file() and get_mm_exe_file() within some of the newly introduced BPF kfuncs. As noted in my response here [1] though, I struggle to understand the technical reasoning behind why exposing such in-kernel helpers, specifically only to BPF LSM program types in the form of BPF kfuncs, is inherently a terrible idea. So, until someone provides me with a sound technical explanation as to why this cannot or should not be done, I'll continue to lean on them. The alternative is to reimplement the necessary in-kernel APIs within the BPF kfuncs, but that's just nonsensical IMO.

Quoted 7 lines · Matt Bobrowski

G'day All,

The original cover letter providing background context and motivating factors around the needs for the BPF kfuncs introduced within this patch series can be found here [0], so please do reference that if need be.

Quoted 11 lines

Notably, one of the main contention points within v1 of this patch series was that we were effectively leaning on some preexisting in-kernel APIs such as get_task_exe_file() and get_mm_exe_file() within some of the newly introduced BPF kfuncs. As noted in my response here [1] though, I struggle to understand the technical reasoning behind why exposing such in-kernel helpers, specifically only to BPF LSM program types in the form of BPF kfuncs, is inherently a terrible idea. So, until someone provides me with a sound technical explanation as to why this cannot or should not be done, I'll continue to lean on them. The alternative is to reimplement the necessary in-kernel APIs within the BPF kfuncs, but that's just nonsensical IMO.

You may lean as much as you like. What I've reacted to is that you've (not you specifically, I'm sure) messed up. You've exposed d_path() to users without understanding that it wasn't safe apparently.

And now we get patches that use the self-inflicted brokeness as an argument to expose a bunch of other low-level helpers to fix that.

The fact that it's "just bpf LSM" programs doesn't alleviate any concerns whatsoever. Not just because that is just an entry vector but also because we have LSMs induced API abuse that we only ever get to see the fallout from when we refactor apis and then it causes pain for the vfs.

I'll take another look at the proposed helpers you need as bpf kfuncs and I'll give my best not to be overly annoyed by all of this. I have no intention of not helping you quite the opposite but I'm annoyed that we're here in the first place.

What I want is to stop this madness of exposing stuff to users without fully understanding it's semantics and required guarantees.

Quoted 24 lines · Christian Brauner

Quoted 7 lines · Matt Bobrowski

G'day All,

The original cover letter providing background context and motivating factors around the needs for the BPF kfuncs introduced within this patch series can be found here [0], so please do reference that if need be.

Quoted 11 lines

Notably, one of the main contention points within v1 of this patch series was that we were effectively leaning on some preexisting in-kernel APIs such as get_task_exe_file() and get_mm_exe_file() within some of the newly introduced BPF kfuncs. As noted in my response here [1] though, I struggle to understand the technical reasoning behind why exposing such in-kernel helpers, specifically only to BPF LSM program types in the form of BPF kfuncs, is inherently a terrible idea. So, until someone provides me with a sound technical explanation as to why this cannot or should not be done, I'll continue to lean on them. The alternative is to reimplement the necessary in-kernel APIs within the BPF kfuncs, but that's just nonsensical IMO.

You may lean as much as you like. What I've reacted to is that you've (not you specifically, I'm sure) messed up. You've exposed d_path() to users without understanding that it wasn't safe apparently.

Quoted 7 lines

And now we get patches that use the self-inflicted brokeness as an argument to expose a bunch of other low-level helpers to fix that.

The fact that it's "just bpf LSM" programs doesn't alleviate any concerns whatsoever. Not just because that is just an entry vector but also because we have LSMs induced API abuse that we only ever get to see the fallout from when we refactor apis and then it causes pain for the vfs.

Quoted 7 lines

I'll take another look at the proposed helpers you need as bpf kfuncs and I'll give my best not to be overly annoyed by all of this. I have no intention of not helping you quite the opposite but I'm annoyed that we're here in the first place.

What I want is to stop this madness of exposing stuff to users without fully understanding it's semantics and required guarantees.

So, looking at this series you're now asking us to expose:

(1) mmgrab()
(2) mmput()
(3) fput()
(5) get_mm_exe_file()
(4) get_task_exe_file()
(7) get_task_fs_pwd()
(6) get_task_fs_root()
(8) path_get()
(9) path_put()

in one go and the justification in all patches amounts to "This is common in some BPF LSM programs".

So, broken stuff got exposed to users or at least a broken BPF LSM program was written somewhere out there that is susceptible to UAFs becauase you didn't restrict bpf_d_path() to trusted pointer arguments. So you're now scrambling to fix this by asking for a bunch of low-level exports.

What is the guarantee that you don't end up writing another BPF LSM that abuses these exports in a way that causes even more issues and then someone else comes back asking for the next round of bpf funcs to be exposed to fix it.

The difference between a regular LSM asking about this and a BPF LSM program is that we can see in the hook implementation what the LSM intends to do with this and we can judge whether that's safe or not.

Quoted 32 lines · Christian Brauner

Quoted 7 lines · Matt Bobrowski

G'day All,

The original cover letter providing background context and motivating factors around the needs for the BPF kfuncs introduced within this patch series can be found here [0], so please do reference that if need be.

Quoted 11 lines

Notably, one of the main contention points within v1 of this patch series was that we were effectively leaning on some preexisting in-kernel APIs such as get_task_exe_file() and get_mm_exe_file() within some of the newly introduced BPF kfuncs. As noted in my response here [1] though, I struggle to understand the technical reasoning behind why exposing such in-kernel helpers, specifically only to BPF LSM program types in the form of BPF kfuncs, is inherently a terrible idea. So, until someone provides me with a sound technical explanation as to why this cannot or should not be done, I'll continue to lean on them. The alternative is to reimplement the necessary in-kernel APIs within the BPF kfuncs, but that's just nonsensical IMO.

You may lean as much as you like. What I've reacted to is that you've (not you specifically, I'm sure) messed up. You've exposed d_path() to users without understanding that it wasn't safe apparently.

And now we get patches that use the self-inflicted brokeness as an argument to expose a bunch of other low-level helpers to fix that.

The fact that it's "just bpf LSM" programs doesn't alleviate any concerns whatsoever. Not just because that is just an entry vector but also because we have LSMs induced API abuse that we only ever get to see the fallout from when we refactor apis and then it causes pain for the vfs.

Quoted 7 lines

I'll take another look at the proposed helpers you need as bpf kfuncs and I'll give my best not to be overly annoyed by all of this. I have no intention of not helping you quite the opposite but I'm annoyed that we're here in the first place.

What I want is to stop this madness of exposing stuff to users without fully understanding it's semantics and required guarantees.

So, looking at this series you're now asking us to expose:

(1) mmgrab()
(2) mmput()
(3) fput()
(5) get_mm_exe_file()
(4) get_task_exe_file()
(7) get_task_fs_pwd()
(6) get_task_fs_root()
(8) path_get()
(9) path_put()

in one go and the justification in all patches amounts to "This is common in some BPF LSM programs".

So, broken stuff got exposed to users or at least a broken BPF LSM program was written somewhere out there that is susceptible to UAFs becauase you didn't restrict bpf_d_path() to trusted pointer arguments. So you're now scrambling to fix this by asking for a bunch of low-level exports.

What is the guarantee that you don't end up writing another BPF LSM that abuses these exports in a way that causes even more issues and then someone else comes back asking for the next round of bpf funcs to be exposed to fix it.

The difference between a regular LSM asking about this and a BPF LSM program is that we can see in the hook implementation what the LSM intends to do with this and we can judge whether that's safe or not.

Here you're asking us to do this blindfolded.

Because really, all I see immediately supportable is the addition of a safe variant of bpf making use of the trusted pointer argument constraint:

[PATCH v2 bpf-next 8/9] bpf: add trusted d_path() based BPF kfunc bpf_path_d_path()

Quoted 63 lines · Christian Brauner

Quoted 32 lines · Christian Brauner

Quoted 7 lines · Matt Bobrowski

G'day All,

The original cover letter providing background context and motivating factors around the needs for the BPF kfuncs introduced within this patch series can be found here [0], so please do reference that if need be.

Quoted 11 lines

Notably, one of the main contention points within v1 of this patch series was that we were effectively leaning on some preexisting in-kernel APIs such as get_task_exe_file() and get_mm_exe_file() within some of the newly introduced BPF kfuncs. As noted in my response here [1] though, I struggle to understand the technical reasoning behind why exposing such in-kernel helpers, specifically only to BPF LSM program types in the form of BPF kfuncs, is inherently a terrible idea. So, until someone provides me with a sound technical explanation as to why this cannot or should not be done, I'll continue to lean on them. The alternative is to reimplement the necessary in-kernel APIs within the BPF kfuncs, but that's just nonsensical IMO.

You may lean as much as you like. What I've reacted to is that you've (not you specifically, I'm sure) messed up. You've exposed d_path() to users without understanding that it wasn't safe apparently.

And now we get patches that use the self-inflicted brokeness as an argument to expose a bunch of other low-level helpers to fix that.

The fact that it's "just bpf LSM" programs doesn't alleviate any concerns whatsoever. Not just because that is just an entry vector but also because we have LSMs induced API abuse that we only ever get to see the fallout from when we refactor apis and then it causes pain for the vfs.

Quoted 7 lines

I'll take another look at the proposed helpers you need as bpf kfuncs and I'll give my best not to be overly annoyed by all of this. I have no intention of not helping you quite the opposite but I'm annoyed that we're here in the first place.

What I want is to stop this madness of exposing stuff to users without fully understanding it's semantics and required guarantees.

So, looking at this series you're now asking us to expose:

(1) mmgrab()
(2) mmput()
(3) fput()
(5) get_mm_exe_file()
(4) get_task_exe_file()
(7) get_task_fs_pwd()
(6) get_task_fs_root()
(8) path_get()
(9) path_put()

in one go and the justification in all patches amounts to "This is common in some BPF LSM programs".

So, broken stuff got exposed to users or at least a broken BPF LSM program was written somewhere out there that is susceptible to UAFs becauase you didn't restrict bpf_d_path() to trusted pointer arguments. So you're now scrambling to fix this by asking for a bunch of low-level exports.

Quoted 4 lines

What is the guarantee that you don't end up writing another BPF LSM that abuses these exports in a way that causes even more issues and then someone else comes back asking for the next round of bpf funcs to be exposed to fix it.

There is no guarantee.
We made a safety mistake with bpf_d_path() though we restricted it very tight. And that UAF is tricky. I'm still amazed how Jann managed to find it. We all make mistakes.
It's not the first one and not going to be the last.

What Matt is doing is an honest effort to fix it
in the upstream kernel for all bpf users to benefit. He could have done it with a kernel module.
The above "low level" helpers are all either static inline in .h or they call EXPORT_SYMBOL[_GPL] or simply inc/dec refcnt.

One can implement such kfuncs in an out of tree kernel module and be done with it, but in the bpf community we encourage everyone to upstream their work.

So kudos to Matt for working on these patches.

His bpf-lsm use case is not special.
It just needs a safe way to call d_path.

(quoted diff) +14 -0

+SEC("lsm.s/file_open")
+__failure __msg("R1 must be referenced or trusted")
+int BPF_PROG(path_d_path_kfunc_untrusted_from_current)
+{
+       struct path *pwd;
+       struct task_struct *current;
+
+       current = bpf_get_current_task_btf();
+       /* Walking a trusted pointer returned from bpf_get_current_task_btf()
+        * yields and untrusted pointer. */
+       pwd = &current->fs->pwd;
+       bpf_path_d_path(pwd, buf, sizeof(buf));
+       return 0;
+}

This test checks that such an access pattern is unsafe and the verifier will catch it.

To make it safe one needs to do:

  current = bpf_get_current_task_btf();
  pwd = bpf_get_task_fs_pwd(current);
  if (!pwd) // error path
  bpf_path_d_path(pwd, ...);
  bpf_put_path(pwd);

these are the kfuncs from patch 6.

And notice that they have KF_ACQUIRE and KF_RELEASE flags.

They tell the verifier to recognize that bpf_get_task_fs_pwd() kfunc acquires 'struct path *'.
Meaning that bpf prog cannot just return without releasing it.

The bpf prog cannot use-after-free that 'pwd' either after it was released by bpf_put_path(pwd).

The verifier static analysis catches such UAF-s. It didn't catch Jann's UAF earlier, because we didn't have these kfuncs! Hence the fix is to add such kfuncs with acquire/release semantics.

Quoted 3 lines

The difference between a regular LSM asking about this and a BPF LSM program is that we can see in the hook implementation what the LSM intends to do with this and we can judge whether that's safe or not.

See above example.
The verifier is doing a much better job than humans when it comes to safety.

Quoted 1 line

Here you're asking us to do this blindfolded.

If you don't trust the verifier to enforce safety, you shouldn't trust Rust compiler to generate safe code either.

In another reply you've compared kfuncs to EXPORT_SYMBOL_GPL. Such analogy is correct to some extent,
but unlike exported symbols kfuncs are restricted to particular program types. They don't accept arbitrary pointers, and reference count is enforced as well.
That's a pretty big difference vs EXPORT_SYMBOL.

There's one fundamental question here that we'll need an official answer to:

Is it ok for an out-of-tree BPF LSM program, that nobody has ever seen to request access to various helpers in the kernel?

Because fundamentally this is what this patchset is asking to be done.

If the ZFS out-of-tree kernel module were to send us a similar patch series asking us for a list of 9 functions that they'd like us to export what would the answer to that be? It would be "no" - on principle alone.

So what is different between an out-of-tree BPF LSM program that no one even has ever seen and an out-of-tree kernel module that one can at least look at in Github? Why should we reject requests from the latter but are supposed to accept requests from the former?

If we say yes to the BPF LSM program requests we would have to say yes to ZFS as well.

Quoted 3 lines · Christian Brauner

There's one fundamental question here that we'll need an official answer to:

Quoted 2 lines

Is it ok for an out-of-tree BPF LSM program, that nobody has ever seen to request access to various helpers in the kernel?

Phrased in a slightly different way, and a bit more generalized: do we treat out-of-tree BPF programs the same as we do with out-of-tree kernel modules? I believe that's the real question, and if we answer that, we should also have our answer for the internal helper function question.

Quoted 5 lines · Paul Moore

Quoted 3 lines · Christian Brauner

There's one fundamental question here that we'll need an official answer to:

Quoted 8 lines

Quoted 2 lines

Is it ok for an out-of-tree BPF LSM program, that nobody has ever seen to request access to various helpers in the kernel?

Phrased in a slightly different way, and a bit more generalized: do we treat out-of-tree BPF programs the same as we do with out-of-tree kernel modules? I believe that's the real question, and if we answer that, we should also have our answer for the internal helper function question.

From 10k ft view bpf programs may look like kernel modules, but looking closely they are very different.

Modules can read/write any data structure and can call any exported function. All modules fall into two categories GPL or not.
While bpf progs are divided by program type.
Tracing progs can read any kernel memory safely via probe_read_kernel. Networking prog can read/write packets, but cannot read kernel memory. bpf_lsm programs can be called from lsm hooks and
call only kfuncs that were explicitly allowlisted to bpf_lsm prog type. Furthermore kfuncs have acquire/release semantics enforced by
the verifier.
For example, bpf progs can do bpf_rcu_read_lock() which is
a wrapper around rcu_read_lock() and the verifier will make sure that bpf_rcu_read_unlock() is called.
Under bpf_rcu_read_lock() bpf programs can dereference __rcu tagged fields and the verifier will track them as rcu protected objects until bpf_rcu_read_unlock().
In other words the verifier is doing sparse-on-steroids analysis and enforcing it.
Kernel modules are not subject to such enforcement.

One more distinction: 99.9% of bpf features require a GPL-ed bpf program. All kfuncs are GPL only.

Quoted 105 lines

Quoted 23 lines

So, looking at this series you're now asking us to expose:

(1) mmgrab()
(2) mmput()
(3) fput()
(5) get_mm_exe_file()
(4) get_task_exe_file()
(7) get_task_fs_pwd()
(6) get_task_fs_root()
(8) path_get()
(9) path_put()

in one go and the justification in all patches amounts to "This is common in some BPF LSM programs".

So, broken stuff got exposed to users or at least a broken BPF LSM program was written somewhere out there that is susceptible to UAFs becauase you didn't restrict bpf_d_path() to trusted pointer arguments. So you're now scrambling to fix this by asking for a bunch of low-level exports.

Quoted 4 lines

What is the guarantee that you don't end up writing another BPF LSM that abuses these exports in a way that causes even more issues and then someone else comes back asking for the next round of bpf funcs to be exposed to fix it.

There is no guarantee.
We made a safety mistake with bpf_d_path() though we restricted it very tight. And that UAF is tricky. I'm still amazed how Jann managed to find it. We all make mistakes.
It's not the first one and not going to be the last.

What Matt is doing is an honest effort to fix it
in the upstream kernel for all bpf users to benefit. He could have done it with a kernel module.
The above "low level" helpers are all either static inline in .h or they call EXPORT_SYMBOL[_GPL] or simply inc/dec refcnt.

One can implement such kfuncs in an out of tree kernel module and be done with it, but in the bpf community we encourage everyone to upstream their work.

So kudos to Matt for working on these patches.

His bpf-lsm use case is not special.
It just needs a safe way to call d_path.

(quoted diff) +14 -0

+SEC("lsm.s/file_open")
+__failure __msg("R1 must be referenced or trusted")
+int BPF_PROG(path_d_path_kfunc_untrusted_from_current)
+{
+       struct path *pwd;
+       struct task_struct *current;
+
+       current = bpf_get_current_task_btf();
+       /* Walking a trusted pointer returned from bpf_get_current_task_btf()
+        * yields and untrusted pointer. */
+       pwd = &current->fs->pwd;
+       bpf_path_d_path(pwd, buf, sizeof(buf));
+       return 0;
+}

This test checks that such an access pattern is unsafe and the verifier will catch it.

To make it safe one needs to do:

  current = bpf_get_current_task_btf();
  pwd = bpf_get_task_fs_pwd(current);
  if (!pwd) // error path
  bpf_path_d_path(pwd, ...);
  bpf_put_path(pwd);

these are the kfuncs from patch 6.

And notice that they have KF_ACQUIRE and KF_RELEASE flags.

They tell the verifier to recognize that bpf_get_task_fs_pwd() kfunc acquires 'struct path *'.
Meaning that bpf prog cannot just return without releasing it.

The bpf prog cannot use-after-free that 'pwd' either after it was released by bpf_put_path(pwd).

The verifier static analysis catches such UAF-s. It didn't catch Jann's UAF earlier, because we didn't have these kfuncs! Hence the fix is to add such kfuncs with acquire/release semantics.

Quoted 3 lines

The difference between a regular LSM asking about this and a BPF LSM program is that we can see in the hook implementation what the LSM intends to do with this and we can judge whether that's safe or not.

See above example.
The verifier is doing a much better job than humans when it comes to safety.

Quoted 1 line

Here you're asking us to do this blindfolded.

If you don't trust the verifier to enforce safety, you shouldn't trust Rust compiler to generate safe code either.

Quoted 6 lines

In another reply you've compared kfuncs to EXPORT_SYMBOL_GPL. Such analogy is correct to some extent,
but unlike exported symbols kfuncs are restricted to particular program types. They don't accept arbitrary pointers, and reference count is enforced as well.
That's a pretty big difference vs EXPORT_SYMBOL.

There's one fundamental question here that we'll need an official answer to:

Is it ok for an out-of-tree BPF LSM program, that nobody has ever seen to request access to various helpers in the kernel?

Because fundamentally this is what this patchset is asking to be done.

If the ZFS out-of-tree kernel module were to send us a similar patch series asking us for a list of 9 functions that they'd like us to export what would the answer to that be?

Quoted 9 lines

Quoted 9 lines

(1) mmgrab()
(2) mmput()
(3) fput()
(5) get_mm_exe_file()
(4) get_task_exe_file()
(7) get_task_fs_pwd()
(6) get_task_fs_root()
(8) path_get()
(9) path_put()

Quoted 112 lines · Christian Brauner

Quoted 105 lines

Quoted 23 lines

So, looking at this series you're now asking us to expose:

(1) mmgrab()
(2) mmput()
(3) fput()
(5) get_mm_exe_file()
(4) get_task_exe_file()
(7) get_task_fs_pwd()
(6) get_task_fs_root()
(8) path_get()
(9) path_put()

in one go and the justification in all patches amounts to "This is common in some BPF LSM programs".

So, broken stuff got exposed to users or at least a broken BPF LSM program was written somewhere out there that is susceptible to UAFs becauase you didn't restrict bpf_d_path() to trusted pointer arguments. So you're now scrambling to fix this by asking for a bunch of low-level exports.

Quoted 4 lines

What is the guarantee that you don't end up writing another BPF LSM that abuses these exports in a way that causes even more issues and then someone else comes back asking for the next round of bpf funcs to be exposed to fix it.

There is no guarantee.
We made a safety mistake with bpf_d_path() though we restricted it very tight. And that UAF is tricky. I'm still amazed how Jann managed to find it. We all make mistakes.
It's not the first one and not going to be the last.

What Matt is doing is an honest effort to fix it
in the upstream kernel for all bpf users to benefit. He could have done it with a kernel module.
The above "low level" helpers are all either static inline in .h or they call EXPORT_SYMBOL[_GPL] or simply inc/dec refcnt.

One can implement such kfuncs in an out of tree kernel module and be done with it, but in the bpf community we encourage everyone to upstream their work.

So kudos to Matt for working on these patches.

His bpf-lsm use case is not special.
It just needs a safe way to call d_path.

(quoted diff) +14 -0

+SEC("lsm.s/file_open")
+__failure __msg("R1 must be referenced or trusted")
+int BPF_PROG(path_d_path_kfunc_untrusted_from_current)
+{
+       struct path *pwd;
+       struct task_struct *current;
+
+       current = bpf_get_current_task_btf();
+       /* Walking a trusted pointer returned from bpf_get_current_task_btf()
+        * yields and untrusted pointer. */
+       pwd = &current->fs->pwd;
+       bpf_path_d_path(pwd, buf, sizeof(buf));
+       return 0;
+}

This test checks that such an access pattern is unsafe and the verifier will catch it.

To make it safe one needs to do:

  current = bpf_get_current_task_btf();
  pwd = bpf_get_task_fs_pwd(current);
  if (!pwd) // error path
  bpf_path_d_path(pwd, ...);
  bpf_put_path(pwd);

these are the kfuncs from patch 6.

And notice that they have KF_ACQUIRE and KF_RELEASE flags.

They tell the verifier to recognize that bpf_get_task_fs_pwd() kfunc acquires 'struct path *'.
Meaning that bpf prog cannot just return without releasing it.

The bpf prog cannot use-after-free that 'pwd' either after it was released by bpf_put_path(pwd).

The verifier static analysis catches such UAF-s. It didn't catch Jann's UAF earlier, because we didn't have these kfuncs! Hence the fix is to add such kfuncs with acquire/release semantics.

Quoted 3 lines

The difference between a regular LSM asking about this and a BPF LSM program is that we can see in the hook implementation what the LSM intends to do with this and we can judge whether that's safe or not.

See above example.
The verifier is doing a much better job than humans when it comes to safety.

Quoted 1 line

Here you're asking us to do this blindfolded.

If you don't trust the verifier to enforce safety, you shouldn't trust Rust compiler to generate safe code either.

Quoted 6 lines

In another reply you've compared kfuncs to EXPORT_SYMBOL_GPL. Such analogy is correct to some extent,
but unlike exported symbols kfuncs are restricted to particular program types. They don't accept arbitrary pointers, and reference count is enforced as well.
That's a pretty big difference vs EXPORT_SYMBOL.

Quoted 4 lines

There's one fundamental question here that we'll need an official answer to:

Is it ok for an out-of-tree BPF LSM program, that nobody has ever seen to request access to various helpers in the kernel?

obviously not.

Quoted 1 line

Because fundamentally this is what this patchset is asking to be done.

Pardon ?

Quoted 3 lines

If the ZFS out-of-tree kernel module were to send us a similar patch series asking us for a list of 9 functions that they'd like us to export what would the answer to that be?

This patch set doesn't request any new EXPORT_SYMBOL.

First of all, there is no such thing as get_task_fs_pwd/root in the kernel.

One can argue that get_mm_exe_file() is not exported, but it's nothing but rcu_lock-wrap plus get_file_rcu() which is EXPORT_SYMBOL.

Christian,

I understand your irritation with anything bpf related due to our mistake with fd=0, but as I said earlier it was an honest mistake. There was no malicious intent. Time to move on.

These exports are specifically for an out-of-tree BPF LSM program that is not accessible to the public. The question in the other mail stands.

Quoted 2 lines

First of all, there is no such thing as get_task_fs_pwd/root in the kernel.

Yeah, we'd need specific helpers for a never seen before out-of-tree BPF LSM. I don't see how that's different from an out-of-tree kernel module.

Quoted 3 lines

One can argue that get_mm_exe_file() is not exported, but it's nothing but rcu_lock-wrap plus get_file_rcu() which is EXPORT_SYMBOL.

Oh, good spot. That's an accident. get_file_rcu() definitely shouldn't be exported. So that'll be removed asap.

Quoted 4 lines · Christian Brauner

These exports are specifically for an out-of-tree BPF LSM program that is not accessible to the public. The question in the other mail stands.

The question was already answered. You just don't like the answer. bpf progs are not equivalent to kernel modules.
They have completely different safety and visibility properties. The safety part I already talked about.
Sounds like the visibility has to be explained.
Kernel modules are opaque binary blobs.
bpf programs are fully transparent. The intent is known
to the verifier and to anyone with understanding
of bpf assembly.
Those that cannot read bpf asm can read C source code that is embedded in the bpf program in kernel memory.
It's not the same as "llvm-dwarfdump module.ko" on disk. The bpf prog source code is loaded into the kernel
at program verification time for debugging and visibility reasons. If there is a verifier bug and bpf manages to crash the kernel vmcore will have relevant lines of program C source code right there.

Hence out-of-tree or in-tree bpf makes no practical difference. The program cannot hide its meaning and doesn't hamper debugging.

Hence adding EXPORT_SYMBOL == Brace for impact!
Expect crashes, api misuse and what not.

While adding bpf_kfunc is a nop for kernel development.
If kfunc is in the way of code refactoring it can be removed (as we demonstrated several times).
A kfunc won't cause headaches for the kernel code it is calling (assuming no verifier bugs).
If there is a bug it's on us to fix it as we demonstrated in the past. For example: bpf_probe_read_kernel().
It's a wrapper of copy_from_kernel_nofault() and over the years bpf users hit various bugs in copy_from_kernel_nofault(), reported them, and _bpf developers_ fixed them.
Though copy_from_kernel_nofault() is as generic as it can get and the same bugs could have been reproduced without bpf
we took care of fixing these parts of the kernel.

Look at path_put().
It's EXPORT_SYMBOL and any kernel module can easily screw up reference counting, so that sooner or later distro folks will experience debug pains due to out-of-tree drivers.

kfunc that calls path_put() won't have such consequences. The verifier will prevent path_put() on a pointer that wasn't acquired by the same bpf program. No support pains. It's a nop for vfs folks.

Quoted 5 lines

Quoted 2 lines

First of all, there is no such thing as get_task_fs_pwd/root in the kernel.

Yeah, we'd need specific helpers for a never seen before out-of-tree BPF LSM. I don't see how that's different from an out-of-tree kernel module.

Sorry, but you don't seem to understand what bpf can and cannot do, hence they look similar.

Quoted 6 lines

Quoted 3 lines

One can argue that get_mm_exe_file() is not exported, but it's nothing but rcu_lock-wrap plus get_file_rcu() which is EXPORT_SYMBOL.

Oh, good spot. That's an accident. get_file_rcu() definitely shouldn't be exported. So that'll be removed asap.

So, just to make a point that
"Included in that set are functions that aren't currently even exported to modules"
you want to un-export get_file_rcu() ?

Quoted 53 lines · Alexei Starovoitov

Quoted 4 lines · Christian Brauner

These exports are specifically for an out-of-tree BPF LSM program that is not accessible to the public. The question in the other mail stands.

The question was already answered. You just don't like the answer. bpf progs are not equivalent to kernel modules.
They have completely different safety and visibility properties. The safety part I already talked about.
Sounds like the visibility has to be explained.
Kernel modules are opaque binary blobs.
bpf programs are fully transparent. The intent is known
to the verifier and to anyone with understanding
of bpf assembly.
Those that cannot read bpf asm can read C source code that is embedded in the bpf program in kernel memory.
It's not the same as "llvm-dwarfdump module.ko" on disk. The bpf prog source code is loaded into the kernel
at program verification time for debugging and visibility reasons. If there is a verifier bug and bpf manages to crash the kernel vmcore will have relevant lines of program C source code right there.

Hence out-of-tree or in-tree bpf makes no practical difference. The program cannot hide its meaning and doesn't hamper debugging.

Hence adding EXPORT_SYMBOL == Brace for impact!
Expect crashes, api misuse and what not.

While adding bpf_kfunc is a nop for kernel development.
If kfunc is in the way of code refactoring it can be removed (as we demonstrated several times).
A kfunc won't cause headaches for the kernel code it is calling (assuming no verifier bugs).
If there is a bug it's on us to fix it as we demonstrated in the past. For example: bpf_probe_read_kernel().
It's a wrapper of copy_from_kernel_nofault() and over the years bpf users hit various bugs in copy_from_kernel_nofault(), reported them, and _bpf developers_ fixed them.
Though copy_from_kernel_nofault() is as generic as it can get and the same bugs could have been reproduced without bpf
we took care of fixing these parts of the kernel.

Look at path_put().
It's EXPORT_SYMBOL and any kernel module can easily screw up reference counting, so that sooner or later distro folks will experience debug pains due to out-of-tree drivers.

kfunc that calls path_put() won't have such consequences. The verifier will prevent path_put() on a pointer that wasn't acquired by the same bpf program. No support pains. It's a nop for vfs folks.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

First of all, there is no such thing as get_task_fs_pwd/root in the kernel.

Yeah, we'd need specific helpers for a never seen before out-of-tree BPF LSM. I don't see how that's different from an out-of-tree kernel module.

Sorry, but you don't seem to understand what bpf can and cannot do, hence they look similar.

Maybe. On the other hand you seem to ignore what I'm saying. You currently don't have a clear set of rules for when it's ok for someone to send patches and request access to bpf kfuncs to implement a new BPF program. This patchset very much illustrates this point. The safety properties of bpf don't matter for this. And again, your safety properties very much didn't protect you from your bpf_d_path() mess.

We're not even clearly told where and how these helper are supposed to be used. That's not ok and will never be ok. As long as there are no clear criteria to operate under this is highly problematic. This may be fine from a bpf perspective and one can even understand why because that's apparently your model or promise to your users. But there's no reason to expect the same level of laxness from any of the subsystems you're requesting kfuncs from.

Hey Christian,

Quoted 70 lines · Christian Brauner

Quoted 53 lines · Alexei Starovoitov

Quoted 4 lines · Christian Brauner

These exports are specifically for an out-of-tree BPF LSM program that is not accessible to the public. The question in the other mail stands.

The question was already answered. You just don't like the answer. bpf progs are not equivalent to kernel modules.
They have completely different safety and visibility properties. The safety part I already talked about.
Sounds like the visibility has to be explained.
Kernel modules are opaque binary blobs.
bpf programs are fully transparent. The intent is known
to the verifier and to anyone with understanding
of bpf assembly.
Those that cannot read bpf asm can read C source code that is embedded in the bpf program in kernel memory.
It's not the same as "llvm-dwarfdump module.ko" on disk. The bpf prog source code is loaded into the kernel
at program verification time for debugging and visibility reasons. If there is a verifier bug and bpf manages to crash the kernel vmcore will have relevant lines of program C source code right there.

Hence out-of-tree or in-tree bpf makes no practical difference. The program cannot hide its meaning and doesn't hamper debugging.

Hence adding EXPORT_SYMBOL == Brace for impact!
Expect crashes, api misuse and what not.

While adding bpf_kfunc is a nop for kernel development.
If kfunc is in the way of code refactoring it can be removed (as we demonstrated several times).
A kfunc won't cause headaches for the kernel code it is calling (assuming no verifier bugs).
If there is a bug it's on us to fix it as we demonstrated in the past. For example: bpf_probe_read_kernel().
It's a wrapper of copy_from_kernel_nofault() and over the years bpf users hit various bugs in copy_from_kernel_nofault(), reported them, and _bpf developers_ fixed them.
Though copy_from_kernel_nofault() is as generic as it can get and the same bugs could have been reproduced without bpf
we took care of fixing these parts of the kernel.

Look at path_put().
It's EXPORT_SYMBOL and any kernel module can easily screw up reference counting, so that sooner or later distro folks will experience debug pains due to out-of-tree drivers.

kfunc that calls path_put() won't have such consequences. The verifier will prevent path_put() on a pointer that wasn't acquired by the same bpf program. No support pains. It's a nop for vfs folks.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

First of all, there is no such thing as get_task_fs_pwd/root in the kernel.

Yeah, we'd need specific helpers for a never seen before out-of-tree BPF LSM. I don't see how that's different from an out-of-tree kernel module.

Sorry, but you don't seem to understand what bpf can and cannot do, hence they look similar.

Maybe. On the other hand you seem to ignore what I'm saying. You currently don't have a clear set of rules for when it's ok for someone to send patches and request access to bpf kfuncs to implement a new BPF program. This patchset very much illustrates this point. The safety properties of bpf don't matter for this. And again, your safety properties very much didn't protect you from your bpf_d_path() mess.

Quoted 7 lines

We're not even clearly told where and how these helper are supposed to be used. That's not ok and will never be ok. As long as there are no clear criteria to operate under this is highly problematic. This may be fine from a bpf perspective and one can even understand why because that's apparently your model or promise to your users. But there's no reason to expect the same level of laxness from any of the subsystems you're requesting kfuncs from.

You raise a completely fair point, and I truly do apologies for the lack of context and in depth explanations around the specific situations that the proposed BPF kfuncs are intended to be used from. Admittedly, that's a failure on my part, and I can completely understand why from a maintainers point of view there would be reservations around acknowledging requests for adding such invisible dependencies.

Now, I'm in a little bit of a tough situation as I'm unable to point you to an open-source BPF LSM implementation that intends to make use of such newly proposed BPF kfuncs. That's just an unfortunate constraint and circumstance that I'm having to deal with, so I'm just going to have to provide heavily redacted and incomplete example to illustrate how these BPF kfuncs intend to be used from BPF LSM programs that I personally work on here at Google. Notably though, the contexts that I do share here may obviously be a nonholistic view on how these newly introduced BPF kfuncs end up getting used in practice by some other completely arbitrary open-source BPF LSM programs.

Anyway, as Alexei had pointed out in one of the prior responses, the core motivating factor behind introducing these newly proposed BPF kfuncs purely stems from the requirement of needing to call bpf_d_path() safely on a struct path from the context of a BPF LSM program, specifically within the security_file_open() and security_mmap_file() LSM hooks. Now, as noted within the original bug report [0], it's currently not considered safe to pluck a struct path out from an arbitrary in-kernel data structure, which in our case was current->mm->exe_file->f_path, and have it passed to bpf_d_path() from the aforementioned LSM hook points, or any other LSM hook point for that matter.

So, without using these newly introduced BPF kfuncs, our BPF LSM program hanging off security_file_open() looks as follows:

```
int BPF_PROG(file_open, struct file *file)
{
  // Perform a whole bunch of operations on the supplied file argument. This
  // includes some form of policy evaluation, and if there's a violation against
  // policy and auditing is enabled, then we eventually call bpf_d_path() on
  // file->f_path. Calling bpf_d_path() on the file argument isn't problematic
  // as we have a stable path here as the file argument is reference counted.
  struct path *target = &file->f_path;

  // ...

  struct task_struct *current = bpf_get_current_task_btf();

  // ...
  
  bpf_rcu_read_lock();
  // Reserve a slot on the BPF ring buffer such that the actor's path can be
  // passed back to userspace.
  void *buf = bpf_ringbuf_reserve(&ringbuf, PATH_MAX, 0);
  if (!buf) {
    goto unlock;
  }

  // For contextual purposes when performing an audit we also call bpf_d_path()
  // on the actor, being current->mm->exe_file->f_path.
  struct path *actor = &current->mm->exe_file->f_path;

  // Now perform the path resolution on the actor via bpf_d_path().
  u64 ret = bpf_d_path(actor, buf, PATH_MAX);
  if (ret > 0) {
    bpf_ringbuf_submit(buf, BPF_RB_NO_WAKEUP);
  } else {

    bpf_ringbuf_discard(buf, 0);
  }

unlock:
  bpf_rcu_read_unlock();
  return 0;
}
```

Hey Christian,

Quoted 70 lines · Christian Brauner

Quoted 53 lines · Alexei Starovoitov

Quoted 4 lines · Christian Brauner

These exports are specifically for an out-of-tree BPF LSM program that is not accessible to the public. The question in the other mail stands.

The question was already answered. You just don't like the answer. bpf progs are not equivalent to kernel modules.
They have completely different safety and visibility properties. The safety part I already talked about.
Sounds like the visibility has to be explained.
Kernel modules are opaque binary blobs.
bpf programs are fully transparent. The intent is known
to the verifier and to anyone with understanding
of bpf assembly.
Those that cannot read bpf asm can read C source code that is embedded in the bpf program in kernel memory.
It's not the same as "llvm-dwarfdump module.ko" on disk. The bpf prog source code is loaded into the kernel
at program verification time for debugging and visibility reasons. If there is a verifier bug and bpf manages to crash the kernel vmcore will have relevant lines of program C source code right there.

Hence out-of-tree or in-tree bpf makes no practical difference. The program cannot hide its meaning and doesn't hamper debugging.

Hence adding EXPORT_SYMBOL == Brace for impact!
Expect crashes, api misuse and what not.

While adding bpf_kfunc is a nop for kernel development.
If kfunc is in the way of code refactoring it can be removed (as we demonstrated several times).
A kfunc won't cause headaches for the kernel code it is calling (assuming no verifier bugs).
If there is a bug it's on us to fix it as we demonstrated in the past. For example: bpf_probe_read_kernel().
It's a wrapper of copy_from_kernel_nofault() and over the years bpf users hit various bugs in copy_from_kernel_nofault(), reported them, and _bpf developers_ fixed them.
Though copy_from_kernel_nofault() is as generic as it can get and the same bugs could have been reproduced without bpf
we took care of fixing these parts of the kernel.

Look at path_put().
It's EXPORT_SYMBOL and any kernel module can easily screw up reference counting, so that sooner or later distro folks will experience debug pains due to out-of-tree drivers.

kfunc that calls path_put() won't have such consequences. The verifier will prevent path_put() on a pointer that wasn't acquired by the same bpf program. No support pains. It's a nop for vfs folks.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

First of all, there is no such thing as get_task_fs_pwd/root in the kernel.

Yeah, we'd need specific helpers for a never seen before out-of-tree BPF LSM. I don't see how that's different from an out-of-tree kernel module.

Sorry, but you don't seem to understand what bpf can and cannot do, hence they look similar.

Maybe. On the other hand you seem to ignore what I'm saying. You currently don't have a clear set of rules for when it's ok for someone to send patches and request access to bpf kfuncs to implement a new BPF program. This patchset very much illustrates this point. The safety properties of bpf don't matter for this. And again, your safety properties very much didn't protect you from your bpf_d_path() mess.

Quoted 7 lines

We're not even clearly told where and how these helper are supposed to be used. That's not ok and will never be ok. As long as there are no clear criteria to operate under this is highly problematic. This may be fine from a bpf perspective and one can even understand why because that's apparently your model or promise to your users. But there's no reason to expect the same level of laxness from any of the subsystems you're requesting kfuncs from.

You raise a completely fair point, and I truly do apologies for the lack of context and in depth explanations around the specific situations that the proposed BPF kfuncs are intended to be used from. Admittedly, that's a failure on my part, and I can completely understand why from a maintainers point of view there would be reservations around acknowledging requests for adding such invisible dependencies.

Now, I'm in a little bit of a tough situation as I'm unable to point you to an open-source BPF LSM implementation that intends to make use of such newly proposed BPF kfuncs. That's just an unfortunate constraint and circumstance that I'm having to deal with, so I'm just going to have to provide heavily redacted and incomplete example to illustrate how these BPF kfuncs intend to be used from BPF LSM programs that I personally work on here at Google. Notably though, the contexts that I do share here may obviously be a nonholistic view on how these newly introduced BPF kfuncs end up getting used in practice by some other completely arbitrary open-source BPF LSM programs.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

One can argue that get_mm_exe_file() is not exported, but it's nothing but rcu_lock-wrap plus get_file_rcu() which is EXPORT_SYMBOL.

Quoted 7 lines

Quoted 2 lines

Oh, good spot. That's an accident. get_file_rcu() definitely shouldn't be exported. So that'll be removed asap.

So, just to make a point that
"Included in that set are functions that aren't currently even exported to modules"
you want to un-export get_file_rcu() ?

No. The reason it was exported was because of the drm subsystem and we already quite disliked that. But it turned out that's not needed so in commit 61d4fb0b349e ("file, i915: fix file reference for mmap_singleton()") they were moved away from this helper.

There is absolutely no way that any userspace will
get access to such low-level helpers. They have zero business to be involved in the lifetimes of objects on this level just as no module has.

Quoted 13 lines · Christian Brauner

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

One can argue that get_mm_exe_file() is not exported, but it's nothing but rcu_lock-wrap plus get_file_rcu() which is EXPORT_SYMBOL.

Quoted 7 lines

Quoted 2 lines

Oh, good spot. That's an accident. get_file_rcu() definitely shouldn't be exported. So that'll be removed asap.

So, just to make a point that
"Included in that set are functions that aren't currently even exported to modules"
you want to un-export get_file_rcu() ?

Quoted 4 lines

No. The reason it was exported was because of the drm subsystem and we already quite disliked that. But it turned out that's not needed so in commit 61d4fb0b349e ("file, i915: fix file reference for mmap_singleton()") they were moved away from this helper.

Arguably that commit 61d4fb0b349e should have had

Fixes 0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU")

i915 was buggy before you touched it
and safe_by_rcu exposed the bug.
I can see why you guys looked at it, saw issues,
and decided to look away.
Though your guess in commit 61d4fb0b349e
"

    Now, there might be delays until
    file->f_op->release::singleton_release() is called and
    i915->gem.mmap_singleton is set to NULL.

"
feels unlikely.
I suspect release() delay cannot be that long to cause rcu stall. In the log prior to the splat there are just two mmap related calls from selftests in i915_gem_mman_live_selftests():
i915: Running i915_gem_mman_live_selftests/igt_mmap_offset_exhaustion
i915: Running i915_gem_mman_live_selftests/igt_mmap
1st mmap test passed, but 2nd failed.
So it looks like it's not a race, but an issue with cleanup in that driver. And instead of getting to the bottom of the issue
you've decided to paper over with get_file_active().
I agree with that trade-off.
But the bug in i915 is still there and it's probably an UAF. get_file_active() is probably operating on a broken 'struct file' that got to zero, but somehow it still around
or it's just a garbage memory and file->f_count
just happened to be zero.

My point is that it's not ok to have such double standards. On one side you're arguing that we shouldn't introduce kfunc:

(quoted diff) +4 -0

+__bpf_kfunc struct file *bpf_get_task_exe_file(struct task_struct *task)
+{
+ return get_task_exe_file(task);
+}

that cleanly takes ref cnt on task->mm->exe_file and _not_ using lower level get_file/get_file_rcu/get_file_active api-s directly which are certainly problematic to expose anywhere, since safe_by_rcu protocol is delicate.

But on the other side there is buggy i915 that does questionable dance with get_file_active().
It's EXPORT_SYMBOL_GPL as well and out of tree driver can ruin safe_by_rcu file properties with hard to debug consequences.

Quoted 4 lines

There is absolutely no way that any userspace will
get access to such low-level helpers. They have zero business to be involved in the lifetimes of objects on this level just as no module has.

correct, and kfuncs do not give bpf prog to do direct get_file*() access because we saw how tricky safe_by_rcu is.
Hence kfuncs acquire file via get_task_exe_file or get_mm_exe_file and release via fput.
That's the same pattern that security/tomoyo/util.c is doing:

   exe_file = get_mm_exe_file(mm);
   if (!exe_file)
        return NULL;

   cp = tomoyo_realpath_from_path(&exe_file->f_path);
   fput(exe_file);

in bpf_lsm case it will be:

   exe_file = bpf_get_mm_exe_file(mm);
   if (!exe_file)
   // the verifier will enforce that bpf prog has this NULL check here
   // because we annotate kfunc as:

BTF_ID_FLAGS(func, bpf_get_mm_exe_file, KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)

bpf_path_d_path(&exe_file->f_path, ...); bpf_put_file(exe_file);
// and the verifier will enforce that bpf_put_file() is called too. // and there is no path out of this bpf program that can take file refcnt // without releasing.

So really these kfuncs are a nop from vfs pov.
If there is a bug in the verifier we will debug it and we will fix it.

You keep saying that bpf_d_path() is a mess. Right. It is a mess now and we're fixing it.
When it was introduced 4 years ago it was safe at that time.

The unrelated verifier "smartness" made it possible to use it in UAF. We found the issue now and we're fixing it.
Over these years we didn't ask vfs folks to help fix such bugs, and not asking for help now.
You're being cc-ed on the patches to be aware on how we plan to fix this bpf_d_path() mess. If you have a viable alternative please suggest.

As it stands the new kfuncs are clean and safe way to solve this mess.

On Wed, Mar 13, 2024 at 02:05:13PM -0700, Alexei Starovoitov wrote:

But this whole polemic just illustrates that you simply didn't bother to understand how the code works. The way you talk about UAF together with SLAB_TYPESAFE_BY_RCU is telling. Please read the code instead of guessing.

You've been provided:

a) good reasons why the patchset in it's current form isn't acceptable repeated multiple times

b) support for exporting a variant of bpf_d_path() that is safe to use

c) a request that all kfunc exports for the vfs will have to be located under fs/, not in kernel/bpf/

d) a path on how to move forward with additional kfunc requests:
Clear and documented rules when it's ok for someone to come along and request access to bpf kfuncs when it's to be rejected and when it's ok to be supported.

You repeatedly threatening to go over the heads of people will not make them more amenable to happily integrate with your subsystem.