[v2,bpf-next,4/9] bpf: add new acquire/release based BPF kfuncs for exe_file

Commit Message

Matt Bobrowski March 6, 2024, 7:40 a.m. UTC

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=[e-mail]/

Signed-off-by: Matt Bobrowski <[e-mail]>
---
 kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

Comments

Christian Brauner March 6, 2024, 11:31 a.m. UTC | #1

On Wed, Mar 06, 2024 at 07:40:00AM +0000, Matt Bobrowski wrote:
> 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=[e-mail]/
> 
> Signed-off-by: Matt Bobrowski <[e-mail]>
> ---

get_mm_exe_file() and get_task_exe_file() aren't even exported to
modules which makes me a lot more hesitant.

>  kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 801808b6efb0..539c58db74d7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -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);
> +}
> +
> +/**
> + * bpf_put_file - put a reference on the supplied file
> + * @f: file of which to put a reference on
> + *
> + * Put a reference on the supplied *f*.
> + */
> +__bpf_kfunc void bpf_put_file(struct file *f)
> +{
> +	fput(f);
> +}

That's probably reasonable because this is already exported to modules.
But that's not a generic argument. So there'll never be kfuncs for
__fput_sync(), flushed_delayed_fput() and other special purpose file put
or get helpers.

Conditio sine qua non is that all such kfunc definitions must live in
the respective file in fs/. They won't be hidden somewhere in
kernel/bpf/.

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 801808b6efb0..539c58db74d7 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -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);
+}
+
+/**
+ * bpf_put_file - put a reference on the supplied file
+ * @f: file of which to put a reference on
+ *
+ * Put a reference on the supplied *f*.
+ */
+__bpf_kfunc void bpf_put_file(struct file *f)
+{
+	fput(f);
+}
+
 __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_ID_FLAGS(func, bpf_get_task_exe_file,
+	     KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_get_mm_exe_file,
+	     KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE | KF_SLEEPABLE)
 BTF_KFUNCS_END(lsm_kfunc_set_ids)
 
 static int bpf_lsm_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)