[PATCH bpf-next v2 0/6] bpf: bound trusted fields to their parent's lifetime

[PATCH bpf-next v2 0/6] bpf: bound trusted fields to their parent's lifetime

[Justin Suess]

Howdy folks,

Fixes to the lifetime of trusted pointers derived from other pointers:
fields walked out of a parent, and elements produced by an iterator.

A field in the __safe_trusted or __safe_trusted_or_null sets becomes
PTR_TRUSTED regardless of how its parent is kept alive: nothing ties
the resulting pointer to its parent's lifetime, even though kfuncs.rst
defines the annotations as marking a field "guaranteed to be valid
(trusted or rcu ...) as long as its parent pointer is valid". The
documented bound was never enforced. There are two separate cases,
RCU-protected parents and referenced parents.

First, an RCU-protected parent, e.g. mm->exe_file read under
bpf_rcu_read_lock(), is MEM_RCU. It can hand out file->f_inode as
fully trusted, and the inode pointer survives bpf_rcu_read_unlock().
It remains dereferenceable without PROBE_MEM and passable to kfuncs
expecting trusted arguments after the grace period.

Second, a referenced parent has the same lifetime problem with release
kfuncs. A trusted field walked out of an acquired object stays usable
even after the reference to its parent is dropped. An in-tree example:
the file acquired from bpf_get_task_exe_file() is released at
bpf_put_file(), but an f_inode taken out of it stays trusted after the
object may be freed.

Fix both by making the walked field inherit its parent's bound.
Patch 1 substitutes MEM_RCU for PTR_TRUSTED when the parent is
MEM_RCU, so the field is invalidated with the critical section like
every other RCU pointer. Patch 3 sets the walked field's parent_id to
the parent's reference. Programs that relied on either escape will no
longer load, but the behavior they relied on was essentially a
use-after-free being papered over with deferred free mechanisms and
refcounts held outside BPF.

Third, the same gap exists for open-coded iterators. The element
returned by bpf_iter_<type>_next() is kept alive by the iterator, yet
the verifier marks it plain PTR_TRUSTED with nothing tying it to the
iterator's reference. bpf_iter_task_vma_next() returns a snapshot
from the iterator's own allocation, with the vma's vm_file reference
held only until the next step or destroy, and the kmem_cache and
dmabuf iterators hand out objects whose reference the iterator drops
the same way. A saved element, or a trusted field walked out of it,
stays usable after bpf_iter_<type>_destroy() freed it. Patch 5
binds trusted elements to the iterator's reference, as already done
for the PTR_TO_MEM elements of bpf_iter_num_next(). Elements of
RCU-protected iterators (task, css, scx_dsq) keep MEM_RCU and stay
bounded by the critical section; they point at RCU-managed objects, and
in-tree programs legitimately return them past the iterator's
destruction (__COMPAT_scx_bpf_dsq_peek()).

With the three bounds in place, a trusted field is valid exactly as
long as whatever pins its parent: the program (hook arguments), a
reference (released by a kfunc or an iterator destroy), or an RCU
critical section.

Patches 2, 4 and 6 add reject and accept selftests for each.

Changes in v2:
- Trusted fields of an RCU parent are PTR_MAYBE_NULL as well as
  MEM_RCU: the parent may be a SLAB_TYPESAFE_BY_RCU object reused
  under the reader, as Sashiko pointed out for file->f_inode
  (patches 1, 2).
- Bind trusted iterator elements to the iterator (patches 5, 6).
- Cc stable on the three fixes.
- v1: https://lore.kernel.org/bpf/20261014173039.3666415-1-utilityemal77@gmail.com/

Justin

Justin Suess (6):
  bpf: Carry MEM_RCU for trusted fields with MEM_RCU parents
  selftests/bpf: Test MEM_RCU carry-over for trusted fields
  bpf: Bind trusted fields to their parent's reference
  selftests/bpf: Test trusted-field invalidation on parent release
  bpf: Bind iterator elements to their iterator
  selftests/bpf: Test iterator element invalidation on destroy

 Documentation/bpf/bpf_iterators.rst           |  6 +-
 kernel/bpf/verifier.c                         | 24 ++++++-
 .../selftests/bpf/progs/iters_task_failure.c  | 68 ++++++++++++++++++
 .../selftests/bpf/progs/verifier_vfs_accept.c | 41 +++++++++++
 .../selftests/bpf/progs/verifier_vfs_reject.c | 70 +++++++++++++++++++
 5 files changed, 206 insertions(+), 3 deletions(-)


base-commit: 763187dbbf83764a57d5811168ef9de7b9a60e2d
-- 
2.55.0

[PATCH bpf-next v2 1/6] bpf: Carry MEM_RCU for trusted fields with MEM_RCU parents

[Justin Suess]

Substitute MEM_RCU | PTR_MAYBE_NULL instead of PTR_TRUSTED for fields
declared as trusted when the parent pointer is MEM_RCU.

Today, walking a field in the __safe_trusted or __safe_trusted_or_null
set invariably yields PTR_TRUSTED if its parent is trusted. If the
parent is instead RCU protected, the resulting pointer survives
bpf_rcu_read_unlock() as fully trusted: it can be dereferenced without
PROBE_MEM and passed to kfuncs expecting trusted arguments after the
parent's RCU grace period, e.g. mm->exe_file (MEM_RCU) followed by
file->f_inode escapes the RCU CS as a trusted inode. Limit the child
field's lifetime to that of the parent.

An RCU-protected parent also guarantees less about the field's value
than a trusted one. __safe_trusted promises a non-NULL field for as
long as the parent object is valid, but RCU only keeps the parent's
memory valid: struct file is allocated SLAB_TYPESAFE_BY_RCU, so a file
read under RCU can be freed and reused within the same CS.

Mark the field PTR_MAYBE_NULL as well, as is already done for __rcu
tagged fields, so programs check it before dereferencing.

Fixes: 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/bpf/sashiko-outbox-171590@kernel.org/
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 kernel/bpf/verifier.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 2f190abe3448..534200c0d27d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6684,9 +6684,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		 * trusted if we are in an RCU CS. Such pointer can be NULL.
 		 */
 		if (type_is_trusted(env, reg, field_name, btf_id)) {
-			flag |= PTR_TRUSTED;
+			flag |= is_rcu_reg(reg) ? MEM_RCU | PTR_MAYBE_NULL : PTR_TRUSTED;
 		} else if (type_is_trusted_or_null(env, reg, field_name, btf_id)) {
-			flag |= PTR_TRUSTED | PTR_MAYBE_NULL;
+			flag |= (is_rcu_reg(reg) ? MEM_RCU : PTR_TRUSTED) | PTR_MAYBE_NULL;
 		} else if (in_rcu_cs(env) && !type_may_be_null(reg->type)) {
 			if (type_is_rcu(env, reg, field_name, btf_id)) {
 				/* ignore __rcu tag and mark it MEM_RCU */
-- 
2.55.0

[PATCH bpf-next v2 2/6] selftests/bpf: Test MEM_RCU carry-over for trusted fields

[Justin Suess]

Walking file->f_inode out of an RCU-protected mm->exe_file must yield
an RCU pointer that may be NULL rather than a trusted one. Add reject
tests where the inode is dereferenced without a NULL check and where it
is used as a trusted argument after bpf_rcu_read_unlock(), and an
accept test where it is checked and dereferenced inside the RCU
critical section.

Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 .../selftests/bpf/progs/verifier_vfs_accept.c | 24 +++++++++
 .../selftests/bpf/progs/verifier_vfs_reject.c | 51 +++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
index 55398c04290a..34919abcff04 100644
--- a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
+++ b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
@@ -100,4 +100,28 @@ int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry,
 	return 0;
 }
 
+SEC("lsm.s/bprm_committed_creds")
+__success
+int BPF_PROG(trusted_field_used_in_rcu_cs, struct linux_binprm *bprm)
+{
+	struct mm_struct *mm = bprm->mm;
+	struct inode *inode;
+	struct file *file;
+	ino_t ino = 0;
+
+	if (!mm)
+		return 0;
+
+	bpf_rcu_read_lock();
+	file = mm->exe_file;
+	if (file) {
+		inode = file->f_inode;
+		if (inode)
+			ino = inode->i_ino;
+	}
+	bpf_rcu_read_unlock();
+	__sink(ino);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c b/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
index b5f456d57669..7ca0d8c3edf1 100644
--- a/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
+++ b/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
@@ -173,4 +173,55 @@ int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry,
 		return -EACCES;
 	return 0;
 }
+
+__weak int subprog_trusted_inode(struct inode *inode __arg_trusted)
+{
+	return inode->i_ino;
+}
+
+SEC("lsm.s/bprm_committed_creds")
+__failure __msg("invalid mem access 'rcu_ptr_or_null_'")
+int BPF_PROG(trusted_field_of_rcu_parent_unchecked, struct linux_binprm *bprm)
+{
+	struct mm_struct *mm = bprm->mm;
+	struct file *file;
+	ino_t ino = 0;
+
+	if (!mm)
+		return 0;
+
+	bpf_rcu_read_lock();
+	file = mm->exe_file;
+	/* The file may have been recycled under RCU: f_inode can be NULL. */
+	if (file)
+		ino = file->f_inode->i_ino;
+	bpf_rcu_read_unlock();
+	__sink(ino);
+	return 0;
+}
+
+SEC("lsm.s/bprm_committed_creds")
+__failure __msg("R1 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_")
+int BPF_PROG(trusted_field_escapes_rcu_cs, struct linux_binprm *bprm)
+{
+	struct mm_struct *mm = bprm->mm;
+	struct inode *inode;
+	struct file *file;
+
+	if (!mm)
+		return 0;
+
+	bpf_rcu_read_lock();
+	file = mm->exe_file;
+	if (!file) {
+		bpf_rcu_read_unlock();
+		return 0;
+	}
+	inode = file->f_inode;
+	bpf_rcu_read_unlock();
+
+	/* inode inherited MEM_RCU from file and is untrusted after unlock. */
+	subprog_trusted_inode(inode);
+	return 0;
+}
 char _license[] SEC("license") = "GPL";
-- 
2.55.0

[PATCH bpf-next v2 3/6] bpf: Bind trusted fields to their parent's reference

[Justin Suess]

A field in the __safe_trusted or __safe_trusted_or_null set walked out
of a referenced parent yields PTR_TRUSTED with nothing tying it to the
reference that keeps it alive. Releasing the parent invalidates the
parent's own register but not the walked field, which stays fully
trusted after the object may already be freed:

	file = bpf_get_task_exe_file(task);	/* acquired */
	inode = file->f_inode;			/* __safe_trusted */
	bpf_put_file(file);			/* inode stays trusted */

Set the walked field's parent_id to the parent's reference id, or to
the parent's own parent_id so a chain of derived fields stays bound to
the original reference. release_reference() already invalidates every
register whose parent_id matches, recursively, and regsafe() already
maps parent_id through check_ids(), so no other verifier changes are
needed. An RCU-protected parent keeps bounding its fields to the RCU
critical section through MEM_RCU instead.

Fixes: 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier.")
Cc: stable@vger.kernel.org
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 kernel/bpf/verifier.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 534200c0d27d..c85c83a6ee63 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6562,6 +6562,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	const char *tname = btf_name_by_offset(reg->btf, t->name_off);
 	const char *field_name = NULL;
 	enum bpf_type_flag flag = 0;
+	u32 parent_id = 0;
 	u32 btf_id = 0;
 	s64 min_off;
 	int ret;
@@ -6716,6 +6717,11 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 			 */
 			flag = PTR_UNTRUSTED;
 		}
+
+		/* A trusted field is pinned by whatever pins its parent. */
+		if (flag & PTR_TRUSTED)
+			parent_id = reg_is_referenced(env, reg) ? reg->id :
+								  reg->parent_id;
 	} else {
 		/* Old compat. Deprecated */
 		clear_trusted_flags(&flag);
@@ -6725,6 +6731,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		ret = mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
 		if (ret < 0)
 			return ret;
+		regs[value_regno].parent_id = parent_id;
 	}
 
 	return 0;
-- 
2.55.0

[PATCH bpf-next v2 4/6] selftests/bpf: Test trusted-field invalidation on parent release

[Justin Suess]

Walking file->f_inode out of an acquired file must not outlive
bpf_put_file(). Add a reject test where the inode is used as a trusted
argument after the file's reference is released and an accept test
where it is read before.

Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 .../selftests/bpf/progs/verifier_vfs_accept.c | 17 +++++++++++++++++
 .../selftests/bpf/progs/verifier_vfs_reject.c | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
index 34919abcff04..53441391bcae 100644
--- a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
+++ b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
@@ -124,4 +124,21 @@ int BPF_PROG(trusted_field_used_in_rcu_cs, struct linux_binprm *bprm)
 	return 0;
 }
 
+SEC("lsm.s/file_open")
+__success
+int BPF_PROG(trusted_field_used_before_put)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct file *file = bpf_get_task_exe_file(task);
+	ino_t ino;
+
+	if (!file)
+		return 0;
+
+	ino = file->f_inode->i_ino;
+	bpf_put_file(file);
+	__sink(ino);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c b/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
index 7ca0d8c3edf1..c2be63ad8055 100644
--- a/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
+++ b/tools/testing/selftests/bpf/progs/verifier_vfs_reject.c
@@ -224,4 +224,23 @@ int BPF_PROG(trusted_field_escapes_rcu_cs, struct linux_binprm *bprm)
 	subprog_trusted_inode(inode);
 	return 0;
 }
+
+SEC("lsm.s/file_open")
+__failure __msg("R1 type=scalar expected=ptr_, trusted_ptr_, rcu_ptr_")
+int BPF_PROG(trusted_field_outlives_parent_reference)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct file *file = bpf_get_task_exe_file(task);
+	struct inode *inode;
+
+	if (!file)
+		return 0;
+
+	inode = file->f_inode;
+	bpf_put_file(file);
+
+	/* inode was pinned by file's reference and died with it. */
+	subprog_trusted_inode(inode);
+	return 0;
+}
 char _license[] SEC("license") = "GPL";
-- 
2.55.0

[PATCH bpf-next v2 5/6] bpf: Bind iterator elements to their iterator

[Justin Suess]

An element returned by an iterator's next() kfunc is kept alive by the
iterator alone. bpf_iter_task_vma_next() returns a snapshot that lives
in the iterator's own allocation and holds the vma's vm_file reference
only until the next step or destroy; bpf_iter_kmem_cache_next() and
bpf_iter_dmabuf_next() hand out objects whose reference the iterator
drops the same way. The verifier marks every such element PTR_TRUSTED
with nothing tying it to the iterator, so a pointer saved across
bpf_iter_<type>_destroy() stays fully trusted after the memory or the
reference behind it is gone:

	bpf_iter_task_vma_new(&it, task, 0);
	vma = bpf_iter_task_vma_next(&it);
	file = vma->vm_file;			/* __safe_trusted_or_null */
	bpf_iter_task_vma_destroy(&it);		/* frees snapshot, fput() */
	start = vma->vm_start;			/* vma, file still trusted */

Set the element's parent_id to the iterator's reference id, as is
already done for the PTR_TO_MEM elements of bpf_iter_num_next(), so
that destroying the iterator invalidates the element together with
every trusted field acquired from it. Elements of an RCU-protected
iterator keep MEM_RCU and stay bounded by the critical section instead:
they point at RCU-managed objects rather than iterator state.

Fixes: 4ac454682158 ("bpf: Introduce task_vma open-coded iterator kfuncs")
Cc: stable@vger.kernel.org
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 Documentation/bpf/bpf_iterators.rst |  6 +++++-
 kernel/bpf/verifier.c               | 13 +++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/bpf/bpf_iterators.rst b/Documentation/bpf/bpf_iterators.rst
index 189e3ec1c6c8..08c9c15b59ac 100644
--- a/Documentation/bpf/bpf_iterators.rst
+++ b/Documentation/bpf/bpf_iterators.rst
@@ -79,7 +79,11 @@ Constructor/next/destructor implementation contract is as follows:
     will eventually return NULL when elements are exhausted. Once NULL is
     returned, subsequent next calls *should keep returning NULL*. Next method
     is marked with KF_ITER_NEXT (and should also have KF_RET_NULL as
-    NULL-returning kfunc, of course).
+    NULL-returning kfunc, of course). An element is only guaranteed to be
+    valid until the iterator is destroyed, so the verifier invalidates it,
+    and every trusted pointer derived from it, at `bpf_iter_<type>_destroy()`.
+    Elements of an RCU-protected iterator (see KF_RCU_PROTECTED) are instead
+    valid until the end of the RCU critical section.
 
   - destructor, `bpf_iter_<type>_destroy()`, is always called once. Even if
     constructor failed or next returned nothing.  Destructor frees up any
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c85c83a6ee63..9ce5517dc1a0 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -15504,6 +15504,19 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
 			regs[BPF_REG_0].btf = desc_btf;
 			regs[BPF_REG_0].type = type;
 			regs[BPF_REG_0].btf_id = ptr_type_id;
+
+			/*
+			 * A trusted element is pinned by the iterator that
+			 * produced it and dies with it, like the memory
+			 * returned by bpf_iter_num_next() above. An RCU
+			 * element is bounded by the critical section instead.
+			 */
+			if (bpf_is_iter_next_kfunc(&meta) && (type & PTR_TRUSTED)) {
+				err = validate_ref_obj(env, &meta.ref_obj);
+				if (err)
+					return err;
+				regs[BPF_REG_0].parent_id = meta.ref_obj.id;
+			}
 		}
 
 		if (is_kfunc_ret_null(&meta)) {
-- 
2.55.0

[PATCH bpf-next v2 6/6] selftests/bpf: Test iterator element invalidation on destroy

[Justin Suess]

Save a task_vma element and a trusted field walked out of it across
bpf_iter_task_vma_destroy() and check that both uses are rejected, and
that the existing operations already allowed before destroy are accepted.

Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
 .../selftests/bpf/progs/iters_task_failure.c  | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/iters_task_failure.c b/tools/testing/selftests/bpf/progs/iters_task_failure.c
index 566a1d3dffea..f9c149b4a2d5 100644
--- a/tools/testing/selftests/bpf/progs/iters_task_failure.c
+++ b/tools/testing/selftests/bpf/progs/iters_task_failure.c
@@ -14,6 +14,74 @@ void bpf_cgroup_release(struct cgroup *p) __ksym;
 void bpf_rcu_read_lock(void) __ksym;
 void bpf_rcu_read_unlock(void) __ksym;
 
+__weak int subprog_trusted_file(struct file *file __arg_trusted)
+{
+	return file->f_inode->i_ino;
+}
+
+SEC("?raw_tp")
+__failure __msg("invalid mem access 'scalar'")
+int iter_task_vma_use_after_destroy(const void *ctx)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct bpf_iter_task_vma it;
+	struct vm_area_struct *vma;
+	unsigned long start;
+
+	bpf_iter_task_vma_new(&it, task, 0);
+	vma = bpf_iter_task_vma_next(&it);
+	if (!vma) {
+		bpf_iter_task_vma_destroy(&it);
+		return 0;
+	}
+	bpf_iter_task_vma_destroy(&it);
+
+	/* The vma snapshot lives in the iterator and died with it. */
+	start = vma->vm_start;
+	return start;
+}
+
+SEC("?raw_tp")
+__failure __msg("R1 type=scalar expected=ptr_, trusted_ptr_, rcu_ptr_")
+int iter_task_vma_file_after_destroy(const void *ctx)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct bpf_iter_task_vma it;
+	struct vm_area_struct *vma;
+	struct file *file;
+
+	bpf_iter_task_vma_new(&it, task, 0);
+	vma = bpf_iter_task_vma_next(&it);
+	file = vma ? vma->vm_file : NULL;
+	if (!file) {
+		bpf_iter_task_vma_destroy(&it);
+		return 0;
+	}
+	bpf_iter_task_vma_destroy(&it);
+
+	/* vm_file was pinned through the vma by the iterator. */
+	return subprog_trusted_file(file);
+}
+
+SEC("?raw_tp")
+__success
+int iter_task_vma_file_before_destroy(const void *ctx)
+{
+	struct task_struct *task = bpf_get_current_task_btf();
+	struct bpf_iter_task_vma it;
+	struct vm_area_struct *vma;
+	struct file *file;
+	int ret = 0;
+
+	bpf_iter_task_vma_new(&it, task, 0);
+	vma = bpf_iter_task_vma_next(&it);
+	file = vma ? vma->vm_file : NULL;
+	if (file)
+		ret = subprog_trusted_file(file);
+	bpf_iter_task_vma_destroy(&it);
+	return ret;
+}
+
 SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
 __failure __msg("kernel func bpf_iter_task_new requires RCU critical section protection")
 int BPF_PROG(iter_tasks_without_lock)
-- 
2.55.0

Re: [PATCH bpf-next v2 5/6] bpf: Bind iterator elements to their iterator

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 7228 bytes --]

> bpf: Bind iterator elements to their iterator

TL;DR:
- Iterator elements may stay trusted after the next next() call drops them, so
  the verifier may still accept use-after-free programs
- Fixes: tag likely should be 4cc8c50c9abc

>     bpf: Bind iterator elements to their iterator
>
>     An element returned by an iterator's next() kfunc is kept alive by the
>     iterator alone. bpf_iter_task_vma_next() returns a snapshot that lives
>     in the iterator's own allocation and holds the vma's vm_file reference
>     only until the next step or destroy; bpf_iter_kmem_cache_next() and
>     bpf_iter_dmabuf_next() hand out objects whose reference the iterator
>     drops the same way. The verifier marks every such element PTR_TRUSTED
>     with nothing tying it to the iterator, so a pointer saved across
>     bpf_iter_<type>_destroy() stays fully trusted after the memory or the
>     reference behind it is gone:
>
>     	bpf_iter_task_vma_new(&it, task, 0);
>     	vma = bpf_iter_task_vma_next(&it);
>     	file = vma->vm_file;			/* __safe_trusted_or_null */
>     	bpf_iter_task_vma_destroy(&it);		/* frees snapshot, fput() */
>     	start = vma->vm_start;			/* vma, file still trusted */
>
>     Set the element's parent_id to the iterator's reference id, as is
>     already done for the PTR_TO_MEM elements of bpf_iter_num_next(), so
>     that destroying the iterator invalidates the element together with
>     every trusted field acquired from it. Elements of an RCU-protected
>     iterator keep MEM_RCU and stay bounded by the critical section instead:
>     they point at RCU-managed objects rather than iterator state.
>
>     Fixes: 4ac454682158 ("bpf: Introduce task_vma open-coded iterator kfuncs")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Justin Suess <utilityemal77@gmail.com>

The Fixes: tag points at 4ac454682158 ("bpf: Introduce task_vma open-coded
iterator kfuncs").  Did the problem described here exist at that commit?

At 4ac454682158, check_kfunc_call() gave every kfunc-returned struct pointer,
including the result of bpf_iter_task_vma_next(), a plain PTR_TO_BTF_ID
without PTR_TRUSTED, so there was no trusted pointer to outlive the iterator.

The PTR_TRUSTED marking of iter next() results (or MEM_RCU for
KF_RCU_PROTECTED iterators) came with 4cc8c50c9abc ("bpf: Make the pointer
returned by iter next method valid"), and that change affects all three
iterators named in the commit message.  The kmem_cache (2e9a548009c2) and
dmabuf (6eab7ac7c5ee) iterators were added after it, and task_vma only became
affected through it.

The task_vma snapshot and vm_file reference described in the commit message
came later with 4cbee026db54 ("bpf: return VMA snapshot from task_vma
iterator"), which itself carries a Fixes: tag pointing at 4ac454682158.

Should this be:

  Fixes: 4cc8c50c9abc ("bpf: Make the pointer returned by iter next method valid")

> diff --git a/Documentation/bpf/bpf_iterators.rst b/Documentation/bpf/bpf_iterators.rst
> index 189e3ec1c6c8..08c9c15b59ac 100644
> --- a/Documentation/bpf/bpf_iterators.rst
> +++ b/Documentation/bpf/bpf_iterators.rst
> @@ -79,7 +79,11 @@ Constructor/next/destructor implementation contract is as follows:

[ ... ]

> -    NULL-returning kfunc, of course).
> +    NULL-returning kfunc, of course). An element is only guaranteed to be
> +    valid until the iterator is destroyed, so the verifier invalidates it,
> +    and every trusted pointer derived from it, at `bpf_iter_<type>_destroy()`.
> +    Elements of an RCU-protected iterator (see KF_RCU_PROTECTED) are instead
> +    valid until the end of the RCU critical section.

[ ... ]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5d3deea1cdc7..869e8d4083c6 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15762,6 +15762,19 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			regs[BPF_REG_0].btf = desc_btf;
>  			regs[BPF_REG_0].type = type;
>  			regs[BPF_REG_0].btf_id = ptr_type_id;
> +
> +			/*
> +			 * A trusted element is pinned by the iterator that
> +			 * produced it and dies with it, like the memory
> +			 * returned by bpf_iter_num_next() above. An RCU
> +			 * element is bounded by the critical section instead.
> +			 */
> +			if (bpf_is_iter_next_kfunc(&meta) && (type & PTR_TRUSTED)) {
> +				err = validate_ref_obj(env, &meta.ref_obj);
> +				if (err)
> +					return err;
> +				regs[BPF_REG_0].parent_id = meta.ref_obj.id;
> +			}
>  		}

Is this fix complete for the lifetime of the element?  The documentation
hunk above and the new comment say the element is valid until
bpf_iter_<type>_destroy(), but several next() kfuncs drop the previous
element inside the following call to next(), not only at destroy.

The commit message says as much for task_vma: "holds the vma's vm_file
reference only until the next step or destroy".

What the next() kfuncs do to the previous element:

  bpf_iter_task_vma_next():
      bpf_iter_task_vma_snapshot_reset() does fput(snap->vm_file) and then
      overwrites the snapshot.  This runs before
      bpf_iter_task_vma_find_next(), so it also runs on the call that
      returns NULL.

  bpf_iter_dmabuf_next() -> dma_buf_iter_next():
      dma_buf_put() on the previous buffer.

  bpf_iter_kmem_cache_next():
      prev->refcount--, or kmem_cache_destroy(prev) if the refcount was 1.

  bpf_iter_css_task_next() -> css_task_iter_next():
      put_task_struct(it->cur_task).

With this patch the element, and the trusted fields walked from it, keep
parent_id equal to the iterator id and stay PTR_TRUSTED until destroy.  Would
a program like this still verify?

    bpf_iter_task_vma_new(&it, task, 0);
    while ((vma = bpf_iter_task_vma_next(&it)))
            if (vma->vm_file)
                    file = vma->vm_file;   /* trusted, parent_id = iter */
    /* the last next() returned NULL after fput(file) */
    if (file)
            ret = subprog_trusted_file(file);  /* still trusted */
    bpf_iter_task_vma_destroy(&it);

Once bpf_iter_task_vma_next() has called vma_end_read(), nothing stops the
VMA from being unmapped concurrently.  Can the file then lose its last
reference while the program still passes it to a trusted-argument kfunc?

The kmem_cache and dmabuf cases look similar: an element saved from
iteration N stays trusted in iteration N+1 after the iterator has put it.

A later patch in the series, a51a1f92e627 ("selftests/bpf: Test iterator
element invalidation on destroy"), covers only the destroy case, and nothing
in the series invalidates the descendants of the iterator id at next().

Should the verifier invalidate the registers whose parent_id is the iterator
id at each next() call for PTR_TRUSTED elements?  If not, should the
documentation, the comment and the commit message say that elements stop
being valid at the next step rather than at destroy?

This looks like a remaining gap in the fix rather than something this patch
introduces.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39445554665

Re: [PATCH bpf-next v2 5/6] bpf: Bind iterator elements to their iterator

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] bpf: iterator elements stay PTR_TRUSTED across next() in check_kfunc_call()
- [High] bpf: missing parent_id reference propagation in check_func_arg() and check_kfunc_call()
--

commit 5ca4ad8c9e1677551effb1ab248bfb034b276685
Author: Justin Suess <utilityemal77@gmail.com>

bpf: Bind iterator elements to their iterator

This commit sets parent_id on PTR_TRUSTED elements returned by open-coded
iterator next() kfuncs to the iterator's reference id so that destroying the
iterator invalidates the returned element and any trusted fields derived from
it.

> diff --git a/Documentation/bpf/bpf_iterators.rst b/Documentation/bpf/bpf_iterators.rst
> index 189e3ec1c6c8e..08c9c15b59acd 100644
> --- a/Documentation/bpf/bpf_iterators.rst
> +++ b/Documentation/bpf/bpf_iterators.rst
> @@ -79,7 +79,11 @@ Constructor/next/destructor implementation contract is as follows:
>      will eventually return NULL when elements are exhausted. Once NULL is
>      returned, subsequent next calls *should keep returning NULL*. Next method
>      is marked with KF_ITER_NEXT (and should also have KF_RET_NULL as
> -    NULL-returning kfunc, of course).
> +    NULL-returning kfunc, of course). An element is only guaranteed to be
> +    valid until the iterator is destroyed, so the verifier invalidates it,
> +    and every trusted pointer derived from it, at `bpf_iter_<type>_destroy()`.
> +    Elements of an RCU-protected iterator (see KF_RCU_PROTECTED) are instead
> +    valid until the end of the RCU critical section.
>  
>    - destructor, `bpf_iter_<type>_destroy()`, is always called once. Even if
>      constructor failed or next returned nothing.  Destructor frees up any
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c85c83a6ee63a..9ce5517dc1a02 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15504,6 +15504,19 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  			regs[BPF_REG_0].btf = desc_btf;
>  			regs[BPF_REG_0].type = type;
>  			regs[BPF_REG_0].btf_id = ptr_type_id;
> +
> +			/*
> +			 * A trusted element is pinned by the iterator that
> +			 * produced it and dies with it, like the memory
> +			 * returned by bpf_iter_num_next() above. An RCU
> +			 * element is bounded by the critical section instead.
> +			 */
> +			if (bpf_is_iter_next_kfunc(&meta) && (type & PTR_TRUSTED)) {
> +				err = validate_ref_obj(env, &meta.ref_obj);
> +				if (err)
> +					return err;
> +				regs[BPF_REG_0].parent_id = meta.ref_obj.id;
> +			}

[Severity: Critical]
Does binding parent_id to the iterator's reference id still leave elements
and derived pointers valid across subsequent bpf_iter_<type>_next() calls?

In process_iter_arg(), unmark_stack_slots_iter() (which calls
release_reference(env, st->id)) is only invoked when is_iter_destroy_kfunc()
is true:

kernel/bpf/verifier.c:process_iter_arg() {
	...
	meta->iter.spi = spi;
	meta->iter.frameno = reg->frameno;
	update_ref_obj(&meta->ref_obj, &bpf_stack_slot(state, spi)->spilled_ptr);

	if (is_iter_destroy_kfunc(meta)) {
		err = unmark_stack_slots_iter(env, reg, nr_slots);
		if (err)
			return err;
	}
	...
}

However, non-RCU open-coded iterator next() implementations drop the
previous element's reference during the next bpf_iter_<type>_next() call
(including the final call that returns NULL), before destroy is called:

- bpf_iter_task_vma_next() calls bpf_iter_task_vma_snapshot_reset(snap),
  which calls fput(snap->vm_file) and overwrites the snapshot.
- bpf_iter_kmem_cache_next() decrements prev->refcount and calls
  kmem_cache_destroy(prev) when prev->refcount == 1.
- bpf_iter_dmabuf_next() calls dma_buf_iter_next(kit->dmabuf), which calls
  dma_buf_put() on the previous dma_buf.
- bpf_iter_css_task_next() calls css_task_iter_next(kit->css_it), which
  calls put_task_struct(it->cur_task) on the previous task.

If a BPF program saves an element pointer or a trusted field walked from it
(such as file = vma->vm_file) in a callee-saved register or stack spill,
calls bpf_iter_<type>_next() again, and then accesses the saved pointer
before calling bpf_iter_<type>_destroy(), won't the pointer remain
PTR_TRUSTED after the underlying reference has been dropped?

Also, should Documentation/bpf/bpf_iterators.rst state that an element is
only valid until the next bpf_iter_<type>_next() call or
bpf_iter_<type>_destroy()?

[Severity: High]
What happens when a pointer pinned via parent_id (such as vma->vm_file) is
passed to bpf_dynptr_from_file() or a kfunc returning a borrowed
PTR_TO_BTF_ID such as bpf_real_data_inode()?

In check_func_arg(), meta->ref_obj is only updated when reg_is_referenced()
is true, which checks cur_state->refs for reg->id and ignores reg->parent_id:

kernel/bpf/verifier.c:check_func_arg() {
	...
	if (reg_is_referenced(env, reg))
		update_ref_obj(&meta->ref_obj, reg);
	...
}

When file = vma->vm_file (which has reg->parent_id set to the iterator's id
and reg_is_referenced() == false) is passed to bpf_dynptr_from_file(),
mark_stack_slots_dynptr() sees ref_obj->id == 0 and calls
acquire_reference(env, insn_idx, 0). Doesn't this create the file dynptr
with parent_id = 0, allowing the dynptr and its slices to outlive
bpf_iter_task_vma_destroy() after fput(snap->vm_file) has run?

Note also that if a child reference is bound to the iterator's reference id
and left unreleased at destroy time, unmark_stack_slots_iter() ignores the
-EINVAL return from release_reference():

kernel/bpf/verifier.c:unmark_stack_slots_iter() {
	...
	if (i == 0)
		WARN_ON_ONCE(release_reference(env, st->id));
	...
}

Similarly, in check_kfunc_call() above, setting regs[BPF_REG_0].parent_id is
gated on bpf_is_iter_next_kfunc(&meta) (unlike the PTR_TO_MEM branch above
it). If a BPF program calls bpf_real_data_inode(file) on either an acquired
file or vma->vm_file, won't the returned trusted struct inode * have
parent_id == 0 and remain valid after bpf_put_file() or
bpf_iter_task_vma_destroy()?

>  		}
>  
>  		if (is_kfunc_ret_null(&meta)) {

([Morph](https://morph.so))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020145059.2146170-1-utilityemal77@gmail.com?part=5