Misc bug fixes - part 3

A set of miscellaneous fixes for bugs reported by Nicholas, batched together again. See commit logs for details. Some of this was caught and posted by Ning before, but AI raised some concerns, so I'm resolving those issues and commandeering their patches now.

Changelog:
----------
v1 -> v2
v1: https://lore.kernel.org/bpf/[email protected]

* Fix GCC-BPF failure due to missed BTF emission for a type.

Kumar Kartikeya Dwivedi (4):
bpf: Require MEM_PERCPU for percpu kptr stores selftests/bpf: Reject non-percpu values in percpu kptr fields
bpf: Clear NON_OWN_REF after RCU protection ends selftests/bpf: Reject graph kptr use after RCU unlock

Ning Ding (4):
bpf: Keep refcount_acquire nullable for borrowed RCU kptrs selftests/bpf: Test borrowed refcount acquisition nullability
bpf: Reject untrusted allocated-object pointers selftests/bpf: Reject refcount acquisition after RCU unlock

 include/linux/bpf_verifier.h                  |   4 +-
 kernel/bpf/verifier.c                         |  27 +++-
 .../selftests/bpf/progs/percpu_alloc_fail.c   |  59 +++++++
 .../selftests/bpf/progs/rcu_read_lock.c       |   6 +-
 .../selftests/bpf/progs/refcounted_kptr.c     |  61 +++++++
 .../bpf/progs/refcounted_kptr_fail.c          | 150 ++++++++++++++++++
 6 files changed, 298 insertions(+), 9 deletions(-)

base-commit: 254c881fe0554c5efb16d355c273702a27a32a20

2.53.0

map_kptr_match_type() treats perm_flags as the set of register type flags that a kptr field permits. Adding MEM_PERCPU to that set for BPF_KPTR_PERCPU does not require the source register to carry it, however. The subset test consequently accepts both a plain bpf_obj_new() allocation and a referenced kernel pointer into a __percpu_kptr map field.

Loads from the field are always marked MEM_PERCPU. Consumers then treat the stored value as the cookie returned by bpf_percpu_obj_new(): per-CPU pointer helpers relocate it, and map teardown selects the per-CPU free path. A plain allocation can therefore provide an arbitrary kernel read/write, while a kernel pointer can be relocated into an invalid address or sent through a missing destructor.

Require the source MEM_PERCPU flag to match the destination field kind. This preserves valid bpf_percpu_obj_new() stores and rejects both the program-BTF and kernel-BTF variants.

---
 kernel/bpf/verifier.c | 7 +++++++
 1 file changed, 7 insertions(+)

index d7dd0befbd10..68353aa61fa1 100644
@@ -4488,6 +4488,13 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
 	if (type_flag(reg->type) & ~perm_flags)
 		goto bad_type;
 
+	/*
+	 * A BPF_KPTR_PERCPU field is read back as MEM_PERCPU, so the value
+	 * stored in it must carry the same flag.
+	 */
+	if ((kptr_field->type == BPF_KPTR_PERCPU) != !!(reg->type & MEM_PERCPU))
+		goto bad_type;
+
 	/* We need to verify reg->type and reg->btf, before accessing reg->btf */
 	reg_name = btf_type_name(reg->btf, reg->btf_id);
 

2.53.0

Add verifier coverage for the two ways a non-percpu pointer can be stored in a __percpu_kptr field: a program-BTF local allocation returned by bpf_obj_new(), and a referenced kernel-BTF task_struct pointer.

Without the verifier fix, both programs are unexpectedly accepted and the negative tests fail. Requiring MEM_PERCPU makes both programs fail verification with the expected invalid-kptr diagnostic.

---
 .../selftests/bpf/progs/percpu_alloc_fail.c   | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)

index 08379c3b6a03..3701f4ea58c7 100644
@@ -33,6 +33,20 @@ struct {
 	__type(value, struct elem);
 } array SEC(".maps");
 
+struct kernel_percpu_elem {
+	struct task_struct __percpu_kptr *task;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct kernel_percpu_elem);
+} kernel_percpu_array SEC(".maps");
+
+struct task_struct *bpf_task_from_pid(s32 pid) __ksym;
+void bpf_task_release(struct task_struct *p) __ksym;
+
 long ret;
 
 SEC("?fentry/bpf_fentry_test1")

@@ -137,6 +151,51 @@ int BPF_PROG(test_array_map_5)
 	return 0;
 }
 
+SEC("?syscall")
+__failure __msg("invalid kptr access, R2 type=trusted_ptr_ expected=ptr_task_struct")
+int reject_kernel_ptr_into_percpu_kptr(void *ctx)
+{
+	struct kernel_percpu_elem *e;
+	struct task_struct *p, *old;
+	int index = 0;
+
+	e = bpf_map_lookup_elem(&kernel_percpu_array, &index);
+	if (!e)
+		return 0;
+
+	p = bpf_task_from_pid(1);
+	if (!p)
+		return 0;
+
+	old = bpf_kptr_xchg(&e->task, p);
+	if (old)
+		bpf_task_release(old);
+	return 0;
+}
+
+SEC("?fentry.s/bpf_fentry_test1")
+__failure __msg("invalid kptr access, R2 type=ptr_ expected=ptr_val_t")
+int BPF_PROG(reject_plain_alloc_into_percpu_kptr)
+{
+	struct val_t __percpu_kptr *old;
+	struct val_t *p;
+	struct elem *e;
+	int index = 0;
+
+	e = bpf_map_lookup_elem(&array, &index);
+	if (!e)
+		return 0;
+
+	p = bpf_obj_new(struct val_t);
+	if (!p)
+		return 0;
+
+	old = bpf_kptr_xchg(&e->pc, p);
+	if (old)
+		bpf_percpu_obj_drop(old);
+	return 0;
+}
+
 SEC("?fentry.s/bpf_fentry_test1")
 __failure __msg("bpf_percpu_obj_new type ID argument must be of a struct of scalars")
 int BPF_PROG(test_array_map_6)

2.53.0

From: Ning Ding <[email protected]>

bpf_refcount_acquire() is fallible for a borrowed reference because the object may have reached a zero refcount. The verifier therefore keeps KF_RET_NULL on the return value unless the argument is an owning reference.

An RCU-protected load of a local kptr is marked MEM_ALLOC, but it only receives NON_OWN_REF when the pointee contains a graph node. A refcounted object without a graph node consequently looks like an owning reference even though the loaded register has no acquired reference state. If the program drops the last real reference while remaining in the RCU critical section, refcount_inc_not_zero() returns NULL while the verifier treats the result as non-NULL.

Only classify the argument as owning when it is backed by a verifier-tracked reference. This retains the non-NULL return for pointers from bpf_obj_new(), bpf_kptr_xchg(), or an earlier successful acquisition, while requiring a NULL check for borrowed RCU kptrs.

---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 68353aa61fa1..bc0abf96cc89 100644
@@ -13178,7 +13178,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_call_arg_me
 						      bpf_diag_reg_type_plain(env, reg->type));
 				return -EINVAL;
 			}
-			if (!type_is_non_owning_ref(reg->type))
+			if (!type_is_non_owning_ref(reg->type) && reg_is_referenced(env, reg))
 				meta->arg_owning_ref = true;
 
 			rec = reg_btf_record(reg);

2.53.0

From: Ning Ding <[email protected]>

Add verifier coverage for the distinction between owning and borrowed arguments to bpf_refcount_acquire().

An owning pointer returned by bpf_obj_new() must continue producing a non-NULL result without an extra check. An RCU-loaded local kptr is only borrowed, so a checked result must load successfully while passing an unchecked result to bpf_obj_drop() must be rejected as possibly NULL.

Use a sleepable syscall program for the borrowed cases so the explicit RCU critical section is what permits the local kptr load. Without the verifier fix, the unchecked case is incorrectly accepted. With it, the verifier rejects the possibly NULL argument.

---
 .../selftests/bpf/progs/refcounted_kptr.c     | 61 +++++++++++++++++++
 .../bpf/progs/refcounted_kptr_fail.c          | 48 +++++++++++++++
 2 files changed, 109 insertions(+)

index 61906f48025c..cae00f7b0a24 100644
@@ -23,6 +23,15 @@ struct map_value {
 	struct node_data __kptr *node;
 };
 
+struct node_refcount_only {
+	long key;
+	struct bpf_refcount refcount;
+};
+
+struct map_value_refcount_only {
+	struct node_refcount_only __kptr *node;
+};
+
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
 	__type(key, int);

@@ -30,6 +39,13 @@ struct {
 	__uint(max_entries, 2);
 } stashed_nodes SEC(".maps");
 
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, struct map_value_refcount_only);
+	__uint(max_entries, 1);
+} stashed_refcount_only SEC(".maps");
+
 struct node_acquire {
 	long key;
 	long data;

@@ -832,6 +848,51 @@ long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
 	return 0;
 }
 
+SEC("tc")
+__success
+long refcount_acquire_owning_input_no_null_check(void *ctx)
+{
+	struct node_refcount_only *n, *m;
+
+	n = bpf_obj_new(typeof(*n));
+	if (!n)
+		return 1;
+
+	m = bpf_refcount_acquire(n);
+	bpf_obj_drop(m);
+	bpf_obj_drop(n);
+
+	return 0;
+}
+
+SEC("?syscall")
+__success
+long refcount_acquire_rcu_map_kptr_null_checked(void *ctx)
+{
+	struct map_value_refcount_only *mapval;
+	struct node_refcount_only *n, *m;
+	int idx = 0;
+
+	mapval = bpf_map_lookup_elem(&stashed_refcount_only, &idx);
+	if (!mapval)
+		return 1;
+
+	bpf_rcu_read_lock();
+	n = mapval->node;
+	if (!n) {
+		bpf_rcu_read_unlock();
+		return 2;
+	}
+	m = bpf_refcount_acquire(n);
+	bpf_rcu_read_unlock();
+
+	if (!m)
+		return 3;
+	bpf_obj_drop(m);
+
+	return 0;
+}
+
 static long __stash_map_empty_xchg(struct node_data *n, int idx)
 {
 	struct map_value *mapval = bpf_map_lookup_elem(&stashed_nodes, &idx);

2.53.0

A local kptr load of an object containing a graph node is marked MEM_RCU and NON_OWN_REF while protected by RCU. When the last RCU read-side critical section ends, invalidate_rcu_protected_refs() removes MEM_RCU and marks the pointer PTR_UNTRUSTED, but leaves NON_OWN_REF set.

The stale flag lets graph kfunc argument checks continue treating the pointer as a live borrowed reference. In particular, bpf_rbtree_remove() can accept a pointer after its protection ended and return it as a new owning reference, even though the object may already have been freed.

Clear NON_OWN_REF when an RCU-protected pointer is demoted. A spin lock also provides implicit RCU protection, so invalidate non-owning references before demoting RCU-protected pointers when releasing the lock. Otherwise the demotion would clear the flag before invalidate_non_owning_refs() can find and invalidate those aliases.

The demoted pointer remains available for fault-protected reads. Exempt such reads from the allocated-object reference-state assertion; writes through a fault-prone pointer are already rejected, and bpf_may_fault_on_deref() makes the surviving loads use BPF_PROBE_MEM.

---
 kernel/bpf/verifier.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

index bc0abf96cc89..56a10f79f9a8 100644
@@ -6038,7 +6038,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) &&
+		/*
+		 * A fault-prone allocated object may still be read through a
+		 * BPF_PROBE_MEM load after its lifetime protection ends. Writes
+		 * through such pointers were rejected above.
+		 */
+		if (type_is_alloc(reg->type) && !bpf_may_fault_on_deref(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;

@@ -7416,10 +7422,14 @@ static int process_spin_lock(struct bpf_verifier_env *env, struct bpf_reg_state
 				lock);
 			return -EINVAL;
 		}
+		/*
+		 * Invalidate non-owning refs before RCU demotion clears their
+		 * NON_OWN_REF flag.
+		 */
+		invalidate_non_owning_refs(env);
+
 		if (!in_rcu_cs(env))
 			invalidate_rcu_protected_refs(env);
-
-		invalidate_non_owning_refs(env);
 	}
 	return 0;
 }

@@ -9519,7 +9529,7 @@ static void invalidate_rcu_protected_refs(struct bpf_verifier_env *env)
 	bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, stack, clear_mask, ({
 		if (reg->type & MEM_RCU) {
 			bpf_diag_mod_begin(env, reg, NULL, BPF_DIAG_MOD_WRITE);
-			reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL);
+			reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL | NON_OWN_REF);
 			reg->type |= PTR_UNTRUSTED;
 			bpf_diag_mod_end(env);
 		}

2.53.0

Add a sleepable verifier test that loads a graph-node local kptr in an explicit RCU read-side critical section, then passes its node to bpf_rbtree_remove() after the section ends.

Before the verifier fix, the stale NON_OWN_REF flag makes the node look like a live borrowed reference and the program is accepted. After the fix, the pointer is demoted without NON_OWN_REF and the graph kfunc argument is rejected.

Also exercise a graph kptr loaded while a spin lock provides implicit RCU protection. The pointer must be invalidated when the lock is released, which guards the required ordering between non-owning-reference invalidation and RCU demotion.

Update the existing fault-protected load test state description. The post-unlock pointer no longer carries NON_OWN_REF, but remains readable because the load is rewritten to use BPF_PROBE_MEM.

---
 .../selftests/bpf/progs/rcu_read_lock.c       |  6 +-
 .../bpf/progs/refcounted_kptr_fail.c          | 75 +++++++++++++++++++
 2 files changed, 78 insertions(+), 3 deletions(-)

index 31d4081c3a9f..cdb255addbc3 100644
@@ -592,9 +592,9 @@ int non_own_ref_untrusted_ld(void *ctx)
 	}
 	bpf_rcu_read_unlock();
 	/*
-	 * The unlock leaves node as PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED
-	 * | NON_OWN_REF, and the load below has to get the BPF_PROBE_MEM
-	 * rewrite for it, otherwise a bad address panics the kernel.
+	 * The unlock leaves node as PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED,
+	 * and the load below has to get the BPF_PROBE_MEM rewrite for it,
+	 * otherwise a bad address panics the kernel.
 	 */
 	non_own_ref_key = node->key;
 	return 0;

2.53.0

From: Ning Ding <[email protected]>

When the final RCU read-side critical section ends, a local kptr is demoted to PTR_UNTRUSTED but retains MEM_ALLOC. The pointer may be NULL or may refer to an object whose lifetime is no longer protected.

type_is_ptr_alloc_obj() nevertheless recognizes any PTR_TO_BTF_ID with MEM_ALLOC as a live allocated object. In particular, a refcount-only local kptr never carries NON_OWN_REF, so it still passes the bpf_refcount_acquire() argument check after RCU protection ends. The kfunc can then dereference NULL or stale memory.

Make type_is_ptr_alloc_obj() reject PTR_UNTRUSTED pointers. Since type_is_non_owning_ref() is based on the same predicate, graph kfunc arguments obey the same live-object requirement. Fault-protected reads of the demoted pointer remain valid: writes are already rejected, and read fixups use bpf_may_fault_on_deref() rather than this predicate.

---
 include/linux/bpf_verifier.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 1339c2f028db..36b65797877d 100644
@@ -1381,7 +1381,9 @@ 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_non_owning_ref(u32 type)

2.53.0

From: Ning Ding <[email protected]>

Add a sleepable verifier test that loads a refcount-only local kptr in an explicit RCU read-side critical section, ends the section, and passes the pointer to bpf_refcount_acquire().

The loaded pointer never carries NON_OWN_REF. After RCU unlock it retains MEM_ALLOC while becoming PTR_UNTRUSTED, which previously made the kfunc argument check accept it as a live allocated object. Expect verification to reject the untrusted argument instead.

---
 .../bpf/progs/refcounted_kptr_fail.c          | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

index f787ecf189d8..338e43822ffe 100644
@@ -189,6 +189,33 @@ long rbtree_remove_after_rcu_unlock(void *ctx)
 	return 0;
 }
 
+SEC("?syscall")
+__failure __msg("R1 is neither owning or non-owning ref")
+long refcount_acquire_after_rcu_unlock(void *ctx)
+{
+	struct map_value_refcount_only *mapval;
+	struct node_refcount_only *node, *ref;
+	int idx = 0;
+
+	mapval = bpf_map_lookup_elem(&stashed_refcount_only, &idx);
+	if (!mapval)
+		return 0;
+
+	bpf_rcu_read_lock();
+	node = mapval->node;
+	if (!node) {
+		bpf_rcu_read_unlock();
+		return 0;
+	}
+	bpf_rcu_read_unlock();
+
+	ref = bpf_refcount_acquire(node);
+	if (ref)
+		bpf_obj_drop(ref);
+
+	return 0;
+}
+
 SEC("?syscall")
 __failure __msg("invalid mem access 'scalar'")
 long graph_kptr_after_spin_unlock(void *ctx)

2.53.0