| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aEHvkQ1C7Ne1dB4n@google.com>
Date: Thu, 5 Jun 2025 19:27:13 +0000
From: Matt Bobrowski <mattbobrowski@...gle.com>
To: Song Liu <song@...nel.org>
Cc: bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
kernel-team@...a.com, andrii@...nel.org, eddyz87@...il.com,
ast@...nel.org, daniel@...earbox.net, martin.lau@...ux.dev,
viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
kpsingh@...nel.org, amir73il@...il.com, repnop@...gle.com,
jlayton@...nel.org, josef@...icpanda.com, mic@...ikod.net,
gnoack@...gle.com, m@...wtm.org
Subject: Re: [PATCH v2 bpf-next 3/4] bpf: Introduce path iterator
On Mon, Jun 02, 2025 at 11:59:19PM -0700, Song Liu wrote:
> Introduce a path iterator, which reliably walk a struct path toward
> the root. This path iterator is based on path_walk_parent. A fixed
> zero'ed root is passed to path_walk_parent(). Therefore, unless the
> user terminates it earlier, the iterator will terminate at the real
> root.
>
> Signed-off-by: Song Liu <song@...nel.org>
> ---
> kernel/bpf/Makefile | 1 +
> kernel/bpf/helpers.c | 3 +++
> kernel/bpf/path_iter.c | 58 ++++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 5 ++++
> 4 files changed, 67 insertions(+)
> create mode 100644 kernel/bpf/path_iter.c
>
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 3a335c50e6e3..454a650d934e 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -56,6 +56,7 @@ obj-$(CONFIG_BPF_SYSCALL) += kmem_cache_iter.o
> ifeq ($(CONFIG_DMA_SHARED_BUFFER),y)
> obj-$(CONFIG_BPF_SYSCALL) += dmabuf_iter.o
> endif
> +obj-$(CONFIG_BPF_SYSCALL) += path_iter.o
>
> CFLAGS_REMOVE_percpu_freelist.o = $(CC_FLAGS_FTRACE)
> CFLAGS_REMOVE_bpf_lru_list.o = $(CC_FLAGS_FTRACE)
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b71e428ad936..b190c78e40f6 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -3397,6 +3397,9 @@ BTF_ID_FLAGS(func, bpf_iter_dmabuf_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLEEPAB
> BTF_ID_FLAGS(func, bpf_iter_dmabuf_destroy, KF_ITER_DESTROY | KF_SLEEPABLE)
> #endif
> BTF_ID_FLAGS(func, __bpf_trap)
> +BTF_ID_FLAGS(func, bpf_iter_path_new, KF_ITER_NEW | KF_SLEEPABLE)
Hm, I'd expect KF_TRUSTED_ARGS to be enforced onto
bpf_iter_path_new(), no? Shouldn't this only be operating on a stable
struct path reference?
> +BTF_ID_FLAGS(func, bpf_iter_path_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_iter_path_destroy, KF_ITER_DESTROY | KF_SLEEPABLE)
At this point, the claim is that such are only to be used from the
context of the BPF LSM. If true, I'd expect these BPF kfuncs to be
part of bpf_fs_kfunc_set_ids once moved into fs/bpf_fs_kfuncs.c.
> static const struct btf_kfunc_id_set common_kfunc_set = {
> diff --git a/kernel/bpf/path_iter.c b/kernel/bpf/path_iter.c
> new file mode 100644
> index 000000000000..0d972ec84beb
> --- /dev/null
> +++ b/kernel/bpf/path_iter.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> +#include <linux/bpf.h>
> +#include <linux/bpf_mem_alloc.h>
> +#include <linux/namei.h>
> +#include <linux/path.h>
> +
> +/* open-coded iterator */
> +struct bpf_iter_path {
> + __u64 __opaque[3];
> +} __aligned(8);
> +
> +struct bpf_iter_path_kern {
> + struct path path;
> + __u64 flags;
> +} __aligned(8);
> +
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_iter_path_new(struct bpf_iter_path *it,
> + struct path *start,
> + __u64 flags)
> +{
> + struct bpf_iter_path_kern *kit = (void *)it;
> +
> + BUILD_BUG_ON(sizeof(*kit) > sizeof(*it));
> + BUILD_BUG_ON(__alignof__(*kit) != __alignof__(*it));
> +
> + if (flags) {
> + memset(&kit->path, 0, sizeof(struct path));
This warrants a comment for sure. Also why not just zero it out
entirely?
> + return -EINVAL;
> + }
> +
> + kit->path = *start;
> + path_get(&kit->path);
> + kit->flags = flags;
> +
> + return 0;
> +}
> +
> +__bpf_kfunc struct path *bpf_iter_path_next(struct bpf_iter_path *it)
> +{
> + struct bpf_iter_path_kern *kit = (void *)it;
> + struct path root = {};
I think this also warrants a comment. Specifically, that unless the
loop is explicitly terminated, bpf_iter_path_next() will continue
looping until we've reached the global root of the VFS.
> + if (!path_walk_parent(&kit->path, &root))
> + return NULL;
> + return &kit->path;
> +}
> +
> +__bpf_kfunc void bpf_iter_path_destroy(struct bpf_iter_path *it)
> +{
> + struct bpf_iter_path_kern *kit = (void *)it;
> +
> + path_put(&kit->path);
> +}
> +
> +__bpf_kfunc_end_defs();
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a7d6e0c5928b..45b45cdfb223 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7036,6 +7036,10 @@ BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct socket) {
> struct sock *sk;
> };
>
> +BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct path) {
> + struct dentry *dentry;
> +};
Only trusted if struct path is trusted, and hence why KF_TRUSTED_ARGS
should be enforced.
> static bool type_is_rcu(struct bpf_verifier_env *env,
> struct bpf_reg_state *reg,
> const char *field_name, u32 btf_id)
> @@ -7076,6 +7080,7 @@ static bool type_is_trusted_or_null(struct bpf_verifier_env *env,
> const char *field_name, u32 btf_id)
> {
> BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct socket));
> + BTF_TYPE_EMIT(BTF_TYPE_SAFE_TRUSTED_OR_NULL(struct path));
>
> return btf_nested_type_is_trusted(&env->log, reg, field_name, btf_id,
> "__safe_trusted_or_null");
> --
> 2.47.1
>
Powered by blists - more mailing lists