| Message ID | 20240731110833.1834742-2-mattbobrowski@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: introduce new VFS based BPF kfuncs | expand |
On Wed, Jul 31, 2024 at 4:09 AM Matt Bobrowski <mattbobrowski@google.com> 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 susceptability 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 flexibily 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 BPF LSM program types. These can be either sleepable or > non-sleepable variants of BPF LSM program types. > > [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/ > > Acked-by: Christian Brauner <brauner@kernel.org> > Signed-off-by: Matt Bobrowski <mattbobrowski@google.com> Acked-by: Song Liu <song@kernel.org> with one nitpic below > --- > fs/Makefile | 1 + > fs/bpf_fs_kfuncs.c | 127 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 128 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..2a66331d8921 > --- /dev/null > +++ b/fs/bpf_fs_kfuncs.c > @@ -0,0 +1,127 @@ > +// 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> It appears we don't need to include all these headers. With my daily config, #include <linux/bpf.h> alone is sufficient. Thanks, Song
On Wed, Jul 31, 2024 at 6:16 PM Song Liu <song@kernel.org> wrote: > > > --- /dev/null > > +++ b/fs/bpf_fs_kfuncs.c > > @@ -0,0 +1,127 @@ > > +// 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> > > It appears we don't need to include all these headers. With my > daily config, #include <linux/bpf.h> alone is sufficient. In general it's a good idea to include necessary headers and not rely on implicit recursive includes, but in this case the list is indeed excessive. I trimmed it a bit while applying. Thanks everyone!
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..2a66331d8921 --- /dev/null +++ b/fs/bpf_fs_kfuncs.c @@ -0,0 +1,127 @@ +// 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 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 BPF LSM programs. + */ +__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 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) + return -EINVAL; + + ret = d_path(path, buf, buf__sz); + if (IS_ERR(ret)) + return PTR_ERR(ret); + + len = buf + buf__sz - ret; + memmove(buf, ret, len); + return len; +} + +__bpf_kfunc_end_defs(); + +BTF_KFUNCS_START(bpf_fs_kfunc_set_ids) +BTF_ID_FLAGS(func, bpf_get_task_exe_file, + KF_ACQUIRE | KF_TRUSTED_ARGS | KF_RET_NULL) +BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE) +BTF_ID_FLAGS(func, bpf_path_d_path, KF_TRUSTED_ARGS) +BTF_KFUNCS_END(bpf_fs_kfunc_set_ids) + +static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id) +{ + if (!btf_id_set8_contains(&bpf_fs_kfunc_set_ids, kfunc_id) || + prog->type == BPF_PROG_TYPE_LSM) + return 0; + return -EACCES; +} + +static const struct btf_kfunc_id_set bpf_fs_kfunc_set = { + .owner = THIS_MODULE, + .set = &bpf_fs_kfunc_set_ids, + .filter = bpf_fs_kfuncs_filter, +}; + +static int __init bpf_fs_kfuncs_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set); +} + +late_initcall(bpf_fs_kfuncs_init);