Re: [PATCH bpf-next v4 3/4] bpf: Reject untrusted allocated-object pointers

[bot+bpf-ci@kernel.org]

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

> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 27b43fda9b178..22fcd01c3cceb 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -1341,7 +1341,16 @@ static inline bool bpf_type_has_unsafe_modifiers(u32 type)
>
>  static inline bool type_is_ptr_alloc_obj(u32 type)
>  {
> -	return base_type(type) == PTR_TO_BTF_ID && type_flag(type) & MEM_ALLOC;
> +	return base_type(type) == PTR_TO_BTF_ID &&
> +	       type_flag(type) & MEM_ALLOC &&
> +	       !(type_flag(type) & PTR_UNTRUSTED);
> +}
> +
> +static inline bool type_is_untrusted_ptr_alloc_obj(u32 type)
> +{
> +	return base_type(type) == PTR_TO_BTF_ID &&
> +	       type_flag(type) & MEM_ALLOC &&
> +	       type_flag(type) & PTR_UNTRUSTED;
>  }
>
>  static inline bool type_is_non_owning_ref(u32 type)

Does adding the PTR_UNTRUSTED check to type_is_ptr_alloc_obj() also
change type_is_non_owning_ref()?

Looking at the definition in include/linux/bpf_verifier.h:

    static inline bool type_is_non_owning_ref(u32 type)
    {
            return type_is_ptr_alloc_obj(type) && type_flag(type) & NON_OWN_REF;
    }

It calls type_is_ptr_alloc_obj(), so after this patch it will also
reject PTR_UNTRUSTED registers. The commit message describes tightening
bpf_refcount_acquire() but doesn't mention this transitive effect.

Can this affect invalidate_non_owning_refs() in sleepable programs?

Consider a sleepable program using bpf_spin_lock() around graph
operations (which is a supported pattern, as seen in
tools/testing/selftests/bpf/progs/refcounted_kptr.c tests like
rbtree_sleepable_rcu).

When the program calls bpf_spin_unlock(), process_spin_lock() runs:

    if (!in_rcu_cs(env))
            invalidate_rcu_protected_refs(env);
    invalidate_non_owning_refs(env);

For sleepable programs, in_rcu_cs() returns false, so
invalidate_rcu_protected_refs() runs first. That function does:

    reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL);
    reg->type |= PTR_UNTRUSTED;

A register holding a non-owning ref to a stashed graph node starts as
PTR_TO_BTF_ID|MEM_ALLOC|NON_OWN_REF|MEM_RCU (set by
btf_ld_kptr_type()/ref_set_non_owning() when loading from the graph).
After invalidate_rcu_protected_refs() it becomes
PTR_TO_BTF_ID|MEM_ALLOC|NON_OWN_REF|PTR_UNTRUSTED.

The immediately following invalidate_non_owning_refs() only acts on
registers where type_is_non_owning_ref() returns true. But after this
patch, type_is_non_owning_ref() returns false for registers with
PTR_UNTRUSTED.

So these registers that should be invalidated instead survive
bpf_spin_unlock() as live, readable pointers. Is this intended?

Pre-patch they would have been invalidated by mark_reg_invalid() in
invalidate_non_owning_refs(). Post-patch they escape invalidation.

The commit message states untrusted pointers "are still allowed for
BPF_PROBE_MEM reads," but does the BPF_PROBE_MEM conversion actually
work for the NON_OWN_REF variant?

Looking at kernel/bpf/fixups.c, the probe conversion logic has:

    case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
            /* probe mem access */
            ...
    default:
            continue;

It has a case for the plain MEM_ALLOC|PTR_UNTRUSTED type, but no case
for PTR_TO_BTF_ID|MEM_ALLOC|NON_OWN_REF|PTR_UNTRUSTED. So a load
through the surviving register is emitted as a plain load with no
exception-table entry, which would fault in the kernel instead of
returning zero.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index bc021c8c8fbf7..c3a11c98cd451 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5820,7 +5820,13 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>  			return -EACCES;
>  		}
>
> -		if (type_is_alloc(reg->type) && !type_is_non_owning_ref(reg->type) &&
> +		/*
> +		 * Skip this referenced-ID sanity check for untrusted allocated objects;
> +		 * the access check above already rejects writes through them.
> +		 */
> +		if (type_is_alloc(reg->type) &&
> +		    !type_is_untrusted_ptr_alloc_obj(reg->type) &&
> +		    !type_is_non_owning_ref(reg->type) &&
>  		    !(reg->type & MEM_RCU) && !reg_is_referenced(env, reg)) {
>  			verifier_bug(env, "allocated object must have a referenced id");
>  			return -EFAULT;

This change allows the demoted PTR_UNTRUSTED|NON_OWN_REF registers to
reach mark_btf_ld_reg() instead of hitting the sanity assertion, which
opens the path for the read-through described above.


---
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/31745514567