Re: [PATCH v3 bpf-next 1/3] bpf: introduce new VFS based BPF kfuncs

[Christian Brauner <brauner@kernel.org>]

On Fri, Jul 26, 2024 at 08:56:02AM GMT, Matt Bobrowski wrote:
> Add a new variant of bpf_d_path() named bpf_path_d_path() which takes
> the form of a BPF kfunc and enforces KF_TRUSTED_ARGS semantics onto
> its arguments.
> 
> This new d_path() based BPF kfunc variant is intended to address the
> legacy bpf_d_path() BPF helper's susceptibility to memory corruption
> issues [0, 1, 2] by ensuring to only operate on supplied arguments
> which are deemed trusted by the BPF verifier. Typically, this means
> that only pointers to a struct path which have been referenced counted
> may be supplied.
> 
> In addition to the new bpf_path_d_path() BPF kfunc, we also add a
> KF_ACQUIRE based BPF kfunc bpf_get_task_exe_file() and KF_RELEASE
> counterpart BPF kfunc bpf_put_file(). This is so that the new
> bpf_path_d_path() BPF kfunc can be used more flexibility from within
> the context of a BPF LSM program. It's rather common to ascertain the
> backing executable file for the calling process by performing the
> following walk current->mm->exe_file while instrumenting a given
> operation from the context of the BPF LSM program. However, walking
> current->mm->exe_file directly is never deemed to be OK, and doing so
> from both inside and outside of BPF LSM program context should be
> considered as a bug. Using bpf_get_task_exe_file() and in turn
> bpf_put_file() will allow BPF LSM programs to reliably get and put
> references to current->mm->exe_file.
> 
> As of now, all the newly introduced BPF kfuncs within this patch are
> limited to sleepable BPF LSM program types. Therefore, they may only
> be called when a BPF LSM program is attached to one of the listed
> attachment points defined within the sleepable_lsm_hooks BTF ID set.
> 
> [0] https://lore.kernel.org/bpf/CAG48ez0ppjcT=QxU-jtCUfb5xQb3mLr=5FcwddF_VKfEBPs_Dg@mail.gmail.com/
> [1] https://lore.kernel.org/bpf/20230606181714.532998-1-jolsa@kernel.org/
> [2] https://lore.kernel.org/bpf/20220219113744.1852259-1-memxor@gmail.com/
> 
> Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
> ---
>  fs/Makefile        |   1 +
>  fs/bpf_fs_kfuncs.c | 133 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 134 insertions(+)
>  create mode 100644 fs/bpf_fs_kfuncs.c
> 
> diff --git a/fs/Makefile b/fs/Makefile
> index 6ecc9b0a53f2..61679fd587b7 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -129,3 +129,4 @@ obj-$(CONFIG_EFIVAR_FS)		+= efivarfs/
>  obj-$(CONFIG_EROFS_FS)		+= erofs/
>  obj-$(CONFIG_VBOXSF_FS)		+= vboxsf/
>  obj-$(CONFIG_ZONEFS_FS)		+= zonefs/
> +obj-$(CONFIG_BPF_LSM)		+= bpf_fs_kfuncs.o
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> new file mode 100644
> index 000000000000..3813e2a83313
> --- /dev/null
> +++ b/fs/bpf_fs_kfuncs.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Google LLC. */
> +
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include <linux/btf_ids.h>
> +#include <linux/dcache.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +#include <linux/init.h>
> +#include <linux/mm.h>
> +#include <linux/path.h>
> +#include <linux/sched.h>
> +
> +__bpf_kfunc_start_defs();
> +/**
> + * bpf_get_task_exe_file - get a reference on the exe_file struct file member of
> + *                         the mm_struct that is nested within the supplied
> + *                         task_struct
> + * @task: task_struct of which the nested mm_struct exe_file member to get a
> + * reference on
> + *
> + * Get a reference on the exe_file struct file member field of the mm_struct
> + * nested within the supplied *task*. The referenced file pointer acquired by
> + * this BPF kfunc must be released using bpf_put_file(). Failing to call
> + * bpf_put_file() on the returned referenced struct file pointer that has been
> + * acquired by this BPF kfunc will result in the BPF program being rejected by
> + * the BPF verifier.
> + *
> + * This BPF kfunc may only be called from sleepable BPF LSM programs.
> + *
> + * Internally, this BPF kfunc leans on get_task_exe_file(), such that calling
> + * bpf_get_task_exe_file() would be analogous to calling get_task_exe_file()
> + * directly in kernel context.
> + *
> + * Return: A referenced struct file pointer to the exe_file member of the
> + * mm_struct that is nested within the supplied *task*. On error, NULL is
> + * returned.
> + */
> +__bpf_kfunc struct file *bpf_get_task_exe_file(struct task_struct *task)
> +{
> +	return get_task_exe_file(task);
> +}
> +
> +/**
> + * bpf_put_file - put a reference on the supplied file
> + * @file: file to put a reference on
> + *
> + * Put a reference on the supplied *file*. Only referenced file pointers may be
> + * passed to this BPF kfunc. Attempting to pass an unreferenced file pointer, or
> + * any other arbitrary pointer for that matter, will result in the BPF program
> + * being rejected by the BPF verifier.
> + *
> + * This BPF kfunc may only be called from sleepable BPF LSM programs. Though
> + * fput() can be called from IRQ context, we're enforcing sleepability here.
> + */
> +__bpf_kfunc void bpf_put_file(struct file *file)
> +{
> +	fput(file);
> +}
> +
> +/**
> + * bpf_path_d_path - resolve the pathname for the supplied path
> + * @path: path to resolve the pathname for
> + * @buf: buffer to return the resolved pathname in
> + * @buf__sz: length of the supplied buffer
> + *
> + * Resolve the pathname for the supplied *path* and store it in *buf*. This BPF
> + * kfunc is the safer variant of the legacy bpf_d_path() helper and should be
> + * used in place of bpf_d_path() whenever possible. It enforces KF_TRUSTED_ARGS
> + * semantics, meaning that the supplied *path* must itself hold a valid
> + * reference, or else the BPF program will be outright rejected by the BPF
> + * verifier.
> + *
> + * This BPF kfunc may only be called from sleepable BPF LSM programs.
> + *
> + * Return: A positive integer corresponding to the length of the resolved
> + * pathname in *buf*, including the NUL termination character. On error, a
> + * negative integer is returned.
> + */
> +__bpf_kfunc int bpf_path_d_path(struct path *path, char *buf, size_t buf__sz)
> +{
> +	int len;
> +	char *ret;
> +
> +	if (buf__sz <= 0)
> +		return -EINVAL;

size_t is unsigned so this should just be !buf__sz I can fix that
though. The __sz thing has meaning to the verifier afaict so I guess
that's fine as name then.