Re: [PATCH bpf-next v1 2/4] selftests/bpf: Test MEM_RCU carry-over for trusted fields

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: unhandled NULL deref of f_inode due to SLAB_TYPESAFE_BY_RCU
--

commit 3b6fbe50569bfdf0fb6b88ab1c38f2e6056c0f68
Author: Justin Suess <utilityemal77@gmail.com>

selftests/bpf: Test MEM_RCU carry-over for trusted fields

This commit adds selftests to verify the behavior of the BPF verifier when
accessing fields from RCU-protected pointers. It includes an accept test
for dereferencing an inode within an RCU critical section, and a reject test
for using it after the critical section ends.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
> index 55398c04290a8..f519ba5f4ea20 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_vfs_accept.c
[ ... ]
> +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;
> +		ino = inode->i_ino;

[Severity: High]
Can this result in an unhandled NULL pointer dereference leading to a
kernel Oops?

Because struct file is allocated with SLAB_TYPESAFE_BY_RCU in files_init() 
in fs/file_table.c, the file object can be concurrently freed and reallocated
while an RCU reader holds a pointer to it. During this reallocation process,
alloc_empty_file() calls init_file(), which zeroes out f_inode.

In check_ptr_to_btf_access() in the verifier, the PTR_MAYBE_NULL flag is omitted
for trusted fields read from MEM_RCU pointers. This prevents the verifier from
enforcing a NULL check on the resulting inode pointer. Additionally, the
dereference bypasses the exception table rewrite that would normally handle
memory faults for untrusted pointers.

If a file is closed and reallocated concurrently while the BPF program runs,
could inode be NULL when inode->i_ino is accessed? Is it safe to codify this
pattern as an accept test without requiring a NULL check for f_inode?

> +	}
> +	bpf_rcu_read_unlock();
> +	__sink(ino);
> +	return 0;
> +}
> +
>  char _license[] SEC("license") = "GPL";

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