Merge branch 'bpf-reject-offset-refcount-acquire-arguments'

Yiyang Chen says:

====================
bpf: Reject offset refcount acquire arguments

bpf_refcount_acquire() is modeled as returning a refcounted allocation
base, but it currently accepts PTR_TO_BTF_ID | MEM_ALLOC arguments whose
offset already points at an embedded graph node returned from a list or
rbtree operation.

At runtime the kfunc starts from the supplied pointer and adds the type's
refcount offset. With a graph-node pointer, that starts from base +
node_off, while the verifier treats the returned pointer as the allocation
base. Reject non-zero fixed-offset arguments to keep the runtime operation
and the verifier model aligned.

Programs that pop graph nodes can still acquire a reference after
normalizing the node pointer with container_of().

Patch 1 handles the zero fixed-offset requirement in the existing
check_func_arg_reg_off() / __check_ptr_off_reg() path without consuming a
bpf_type_flag bit.

Patch 2 adds a rejected direct list-node case.

Changes from v3:
  - Add Eduard's Acked-by to patch 1.
  - Drop the redundant rbtree selftest case; the list case exercises the same
    refcount-acquire fixed-offset rejection path.
  - Trim the selftest commit message and remove the selftest Fixes tag.

Changes from v2:
  - Avoid adding a new bpf_type_flag bit.
  - Carry the refcount-acquire zero fixed-offset requirement with an
    internal check_func_arg_reg_off() parameter.

Changes from v1:
  - Move zero fixed-offset enforcement into check_func_arg_reg_off() /
    __check_ptr_off_reg(), as suggested by Eduard.
  - Drop the positive container_of() selftest case.
  - Remove the stale bpf_obj_drop() after bpf_list_push_front(), since the
    pushed reference is consumed even when the verifier explores the error
    branch.
  - Rebase to bpf-next master a975094bf9.

v3: https://lore.kernel.org/bpf/cover.1781979133.git.chenyy23@mails.tsinghua.edu.cn/
v2: https://lore.kernel.org/bpf/cover.1781963957.git.chenyy23@mails.tsinghua.edu.cn/
v1: https://lore.kernel.org/bpf/cover.1781852308.git.chenyy23@mails.tsinghua.edu.cn/
====================

Link: https://patch.msgid.link/cover.1782192383.git.chenyy23@mails.tsinghua.edu.cn
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

kernel/bpf/verifier.c

@@ -7996,9 +7996,10 @@ reg_find_field_offset(const struct bpf_reg_state *reg, s32 off, u32 fields)
 	return field;
 }
 
-static int check_func_arg_reg_off(struct bpf_verifier_env *env,
-				  const struct bpf_reg_state *reg, argno_t argno,
-				  enum bpf_arg_type arg_type)
+static int __check_func_arg_reg_off(struct bpf_verifier_env *env,
+				    const struct bpf_reg_state *reg, argno_t argno,
+				    enum bpf_arg_type arg_type,
+				    bool btf_id_fixed_off_ok)
 {
 	u32 type = reg->type;
 
@@ -8055,12 +8056,11 @@ static int check_func_arg_reg_off(struct bpf_verifier_env *env,
 	case PTR_TO_BTF_ID | MEM_ALLOC | NON_OWN_REF | MEM_RCU:
 		/* When referenced PTR_TO_BTF_ID is passed to release function,
 		 * its fixed offset must be 0. In the other cases, fixed offset
-		 * can be non-zero. This was already checked above. So pass
-		 * fixed_off_ok as true to allow fixed offset for all other
-		 * cases. var_off always must be 0 for PTR_TO_BTF_ID, hence we
-		 * still need to do checks instead of returning.
+		 * can be non-zero unless the caller requires otherwise.
+		 * var_off always must be 0 for PTR_TO_BTF_ID, hence we still
+		 * need to do checks instead of returning.
 		 */
-		return __check_ptr_off_reg(env, reg, argno, true);
+		return __check_ptr_off_reg(env, reg, argno, btf_id_fixed_off_ok);
 	case PTR_TO_CTX:
 		/*
 		 * Allow fixed and variable offsets for syscall context, but
@@ -8076,6 +8076,13 @@ static int check_func_arg_reg_off(struct bpf_verifier_env *env,
 	}
 }
 
+static int check_func_arg_reg_off(struct bpf_verifier_env *env,
+				  const struct bpf_reg_state *reg, argno_t argno,
+				  enum bpf_arg_type arg_type)
+{
+	return __check_func_arg_reg_off(env, reg, argno, arg_type, true);
+}
+
 static int check_arg_const_str(struct bpf_verifier_env *env,
 			       struct bpf_reg_state *reg, argno_t argno)
 {
@@ -11947,6 +11954,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 		enum bpf_arg_type arg_type = ARG_DONTCARE;
 		argno_t argno = argno_from_arg(i + 1);
 		int regno = reg_from_argno(argno);
+		bool btf_id_fixed_off_ok = true;
 		u32 ref_id, type_size;
 		bool is_ret_buf_sz = false;
 		int kf_arg_type;
@@ -12120,7 +12128,6 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 		case KF_ARG_PTR_TO_MEM:
 		case KF_ARG_PTR_TO_MEM_SIZE:
 		case KF_ARG_PTR_TO_CALLBACK:
-		case KF_ARG_PTR_TO_REFCOUNTED_KPTR:
 		case KF_ARG_PTR_TO_CONST_STR:
 		case KF_ARG_PTR_TO_WORKQUEUE:
 		case KF_ARG_PTR_TO_TIMER:
@@ -12134,6 +12141,10 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 		case KF_ARG_PTR_TO_CTX:
 			arg_type = ARG_PTR_TO_CTX;
 			break;
+		case KF_ARG_PTR_TO_REFCOUNTED_KPTR:
+			arg_type = ARG_PTR_TO_BTF_ID;
+			btf_id_fixed_off_ok = false;
+			break;
 		default:
 			verifier_bug(env, "unknown kfunc arg type %d", kf_arg_type);
 			return -EFAULT;
@@ -12141,7 +12152,8 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 
 		if (regno == meta->release_regno)
 			arg_type |= OBJ_RELEASE;
-		ret = check_func_arg_reg_off(env, reg, argno, arg_type);
+		ret = __check_func_arg_reg_off(env, reg, argno, arg_type,
+					       btf_id_fixed_off_ok);
 		if (ret < 0)
 			return ret;
 

tools/testing/selftests/bpf/progs/refcounted_kptr_fail.c

@@ -13,12 +13,20 @@ struct node_acquire {
 	struct bpf_refcount refcount;
 };
 
+struct node_refcounted {
+	long key;
+	struct bpf_list_node list;
+	struct bpf_refcount refcount;
+};
+
 extern void bpf_rcu_read_lock(void) __ksym;
 extern void bpf_rcu_read_unlock(void) __ksym;
 
 #define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8)))
 private(A) struct bpf_spin_lock glock;
 private(A) struct bpf_rb_root groot __contains(node_acquire, node);
+private(B) struct bpf_spin_lock lock;
+private(B) struct bpf_list_head head __contains(node_refcounted, list);
 
 static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
 {
@@ -93,6 +101,32 @@ long rbtree_refcounted_node_ref_escapes_owning_input(void *ctx)
 	return 0;
 }
 
+SEC("?tc")
+__failure __msg("dereference of modified ptr_ ptr R1")
+long refcount_acquire_list_node_offset(void *ctx)
+{
+	struct node_refcounted *node, *base, *ref;
+	struct bpf_list_node *list_node;
+
+	node = bpf_obj_new(typeof(*node));
+	if (!node)
+		return 1;
+
+	bpf_spin_lock(&lock);
+	bpf_list_push_front(&head, &node->list);
+	list_node = bpf_list_pop_front(&head);
+	bpf_spin_unlock(&lock);
+	if (!list_node)
+		return 2;
+
+	base = container_of(list_node, struct node_refcounted, list);
+	ref = bpf_refcount_acquire(list_node);
+	if (ref)
+		bpf_obj_drop(ref);
+	bpf_obj_drop(base);
+	return 0;
+}
+
 SEC("?fentry.s/" SYS_PREFIX "sys_getpgid")
 __failure __msg("function calls are not allowed while holding a lock")
 int BPF_PROG(rbtree_fail_sleepable_lock_across_rcu,