Currently the ref_obj_id and OBJ_RELEASE searching is done in the code
that examines each individual arg (check_func_arg for helpers and
check_kfunc_args inner loop for kfuncs). This patch pulls out this
searching to occur before individual arg type handling, resulting in a
cleaner separation of logic and shared logic between kfuncs and helpers.
The logic for this searching is already very similar between kfuncs and
helpers:
Kfuncs:
* Function-level KF_RELEASE flag indicates that the kfunc releases
some previously-acquired arg
* Verifier searches through arg regs to find those with ref_obj_id set
* One such arg reg is selected. If multiple arg regs have ref_obj_id
set, the last one (by regno) is chosen to be released
Helpers:
* OBJ_RELEASE is used in function proto to tag a particular arg as the
one that should be released
* There can only be one such tagged arg
* Verifier confirms that only one arg reg has ref_obj_id set and that
that reg matches expected OBJ_RELEASE arg
* If OBJ_RELEASE arg doesn't have a matching ref_obj_id arg reg, or
some other arg reg has ref_obj_id, it's an invalid state
It's a long-term goal to merge as much kfunc and helper logic as
possible. Merging the similar functionality here is a small step in that
direction.
Two new helper functions are added:
* args_find_ref_obj_id_regno
* For helpers and kfuncs. Searches through arg regs to find
ref_obj_id reg and returns its regno.
* helper_proto_find_release_arg_regno
* For helpers only. Searches through fn proto args to find the
OBJ_RELEASE arg and returns the corresponding regno.
The refactoring strives to keep failure logic and error messages
unchanged. However, because the release arg searching is now done before
any arg-specific type checking, verifier states that are invalid due to
both invalid release arg state _and_ some type- or helper-specific
checking logic might see the release arg-related error message first,
when previously verification would fail for the other reason.
Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
---
v2 -> v3:
* Edit patch summary for clarity
* Correct err_multi comment in args_find_ref_obj_id_regno doc string
* Rebase onto latest bpf-next: 'Revert "bpf: Add --skip_encoding_btf_inconsistent_proto, --btf_gen_optimized to pahole flags for v1.25"'
v1 -> v2: https://lore.kernel.org/bpf/20230121002417.1684602-1-davemarchevsky@xxxxxx/
* Fix uninitialized variable complaint (kernel test bot)
* Add err_multi param to args_find_ref_obj_id_regno - kfunc arg reg
checking wasn't erroring if multiple ref_obj_id arg regs were found,
retain this behavior
v0 -> v1: https://lore.kernel.org/bpf/20221217082506.1570898-2-davemarchevsky@xxxxxx/
* Remove allow_multi from args_find_ref_obj_id_regno, no need to
support multiple ref_obj_id arg regs
* No need to use temp variable 'i' to count nargs (David)
* Proper formatting of function-level comments on newly-added helpers (David)
kernel/bpf/verifier.c | 220 +++++++++++++++++++++++++++++-------------
1 file changed, 153 insertions(+), 67 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 21e08c111702..c0d01085f44f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6735,48 +6735,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
return err;
skip_type_check:
- if (arg_type_is_release(arg_type)) {
- if (arg_type_is_dynptr(arg_type)) {
- struct bpf_func_state *state = func(env, reg);
- int spi;
-
- /* Only dynptr created on stack can be released, thus
- * the get_spi and stack state checks for spilled_ptr
- * should only be done before process_dynptr_func for
- * PTR_TO_STACK.
- */
- if (reg->type == PTR_TO_STACK) {
- spi = dynptr_get_spi(env, reg);
- if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) {
- verbose(env, "arg %d is an unacquired reference\n", regno);
- return -EINVAL;
- }
- } else {
- verbose(env, "cannot release unowned const bpf_dynptr\n");
- return -EINVAL;
- }
- } else if (!reg->ref_obj_id && !register_is_null(reg)) {
- verbose(env, "R%d must be referenced when passed to release function\n",
- regno);
- return -EINVAL;
- }
- if (meta->release_regno) {
- verbose(env, "verifier internal error: more than one release argument\n");
- return -EFAULT;
- }
- meta->release_regno = regno;
- }
-
- if (reg->ref_obj_id) {
- if (meta->ref_obj_id) {
- verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
- regno, reg->ref_obj_id,
- meta->ref_obj_id);
- return -EFAULT;
- }
- meta->ref_obj_id = reg->ref_obj_id;
- }
-
switch (base_type(arg_type)) {
case ARG_CONST_MAP_PTR:
/* bpf_map_xxx(map_ptr) call: remember that map_ptr */
@@ -6891,6 +6849,26 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
err = check_mem_size_reg(env, reg, regno, true, meta);
break;
case ARG_PTR_TO_DYNPTR:
+ if (meta->release_regno == regno) {
+ struct bpf_func_state *state = func(env, reg);
+ int spi;
+
+ /* Only dynptr created on stack can be released, thus
+ * the get_spi and stack state checks for spilled_ptr
+ * should only be done before process_dynptr_func for
+ * PTR_TO_STACK.
+ */
+ if (reg->type == PTR_TO_STACK) {
+ spi = dynptr_get_spi(env, reg);
+ if (spi < 0 || !state->stack[spi].spilled_ptr.ref_obj_id) {
+ verbose(env, "arg %d is an unacquired reference\n", regno);
+ return -EINVAL;
+ }
+ } else {
+ verbose(env, "cannot release unowned const bpf_dynptr\n");
+ return -EINVAL;
+ }
+ }
err = process_dynptr_func(env, regno, arg_type, meta);
if (err)
return err;
@@ -8104,10 +8082,95 @@ static void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno
state->callback_subprogno == subprogno);
}
+/**
+ * args_find_ref_obj_id_regno() - Find regno that should become meta->ref_obj_id
+ * @env: Verifier env
+ * @regs: Regs to search for ref_obj_id
+ * @nargs: Number of arg regs to search
+ * @err_multi: Should this function error if multiple ref_obj_id args found
+ *
+ * Call arg meta's ref_obj_id is used to either:
+ * * For release funcs, keep track of ref that needs to be released
+ * * For other funcs, keep track of ref that needs to be propagated to retval
+ *
+ * Find the arg regno with nonzero ref_obj_id
+ *
+ * If err_multi is false and multiple ref_obj_id arg regs are seen, regno of the
+ * last one is returned
+ *
+ * Return:
+ * * On success, regno that should become meta->ref_obj_id (regno > 0 since
+ * BPF_REG_1 is first arg
+ * * 0 if no arg had ref_obj_id set
+ * * -err if some invalid arg reg state
+ */
+static int args_find_ref_obj_id_regno(struct bpf_verifier_env *env, struct bpf_reg_state *regs,
+ u32 nargs, bool err_multi)
+{
+ struct bpf_reg_state *reg;
+ u32 i, regno, found_regno = 0;
+
+ for (i = 0; i < nargs; i++) {
+ regno = i + BPF_REG_1;
+ reg = ®s[regno];
+
+ if (!reg->ref_obj_id)
+ continue;
+
+ if (found_regno && err_multi) {
+ verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
+ regno, reg->ref_obj_id, regs[found_regno].ref_obj_id);
+ return -EFAULT;
+ }
+
+ found_regno = regno;
+ }
+
+ return found_regno;
+}
+
+/**
+ * helper_proto_find_release_arg_regno() - Find OBJ_RELEASE arg in func proto
+ * @env: Verifier env
+ * @fn: Func proto to search for OBJ_RELEASE
+ * @nargs: Number of arg specs to search
+ *
+ * For helpers, to determine which arg reg should be released, loop through
+ * func proto arg specification to find arg with OBJ_RELEASE
+ *
+ * Return:
+ * * On success, regno of single OBJ_RELEASE arg
+ * * 0 if no arg in the proto was OBJ_RELEASE
+ * * -err if some invalid func proto state
+ */
+static int helper_proto_find_release_arg_regno(struct bpf_verifier_env *env,
+ const struct bpf_func_proto *fn, u32 nargs)
+{
+ enum bpf_arg_type arg_type;
+ int i, release_regno = 0;
+
+ for (i = 0; i < nargs; i++) {
+ arg_type = fn->arg_type[i];
+
+ if (!arg_type_is_release(arg_type))
+ continue;
+
+ if (release_regno) {
+ verbose(env, "verifier internal error: more than one release argument\n");
+ return -EFAULT;
+ }
+
+ release_regno = i + BPF_REG_1;
+ }
+
+ return release_regno;
+}
+
static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
int *insn_idx_p)
{
enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
+ int i, err, func_id, nargs, release_regno, ref_regno;
const struct bpf_func_proto *fn = NULL;
enum bpf_return_type ret_type;
enum bpf_type_flag ret_flag;
@@ -8115,7 +8178,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
struct bpf_call_arg_meta meta;
int insn_idx = *insn_idx_p;
bool changes_data;
- int i, err, func_id;
/* find function prototype */
func_id = insn->imm;
@@ -8179,8 +8241,37 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
}
meta.func_id = func_id;
+ regs = cur_regs(env);
+
+ /* find actual arg count */
+ for (nargs = 0; nargs < MAX_BPF_FUNC_REG_ARGS; nargs++)
+ if (fn->arg_type[nargs] == ARG_DONTCARE)
+ break;
+
+ release_regno = helper_proto_find_release_arg_regno(env, fn, nargs);
+ if (release_regno < 0)
+ return release_regno;
+
+ ref_regno = args_find_ref_obj_id_regno(env, regs, nargs, true);
+ if (ref_regno < 0)
+ return ref_regno;
+ else if (ref_regno > 0)
+ meta.ref_obj_id = regs[ref_regno].ref_obj_id;
+
+ if (release_regno > 0) {
+ if (!regs[release_regno].ref_obj_id &&
+ !register_is_null(®s[release_regno]) &&
+ !arg_type_is_dynptr(fn->arg_type[release_regno - BPF_REG_1])) {
+ verbose(env, "R%d must be referenced when passed to release function\n",
+ release_regno);
+ return -EINVAL;
+ }
+
+ meta.release_regno = release_regno;
+ }
+
/* check args */
- for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) {
+ for (i = 0; i < nargs; i++) {
err = check_func_arg(env, i, &meta, fn);
if (err)
return err;
@@ -8204,8 +8295,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
return err;
}
- regs = cur_regs(env);
-
/* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot
* be reinitialized by any dynptr helper. Hence, mark_stack_slots_dynptr
* is safe to do directly.
@@ -9442,10 +9531,11 @@ static int process_kf_arg_ptr_to_rbtree_node(struct bpf_verifier_env *env,
static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_arg_meta *meta)
{
const char *func_name = meta->func_name, *ref_tname;
+ struct bpf_reg_state *regs = cur_regs(env);
const struct btf *btf = meta->btf;
const struct btf_param *args;
+ int ret, ref_regno;
u32 i, nargs;
- int ret;
args = (const struct btf_param *)(meta->func_proto + 1);
nargs = btf_type_vlen(meta->func_proto);
@@ -9455,17 +9545,31 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
return -EINVAL;
}
+ ref_regno = args_find_ref_obj_id_regno(env, cur_regs(env), nargs, false);
+ if (ref_regno < 0) {
+ return ref_regno;
+ } else if (!ref_regno && is_kfunc_release(meta)) {
+ verbose(env, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
+ func_name);
+ return -EINVAL;
+ }
+
+ meta->ref_obj_id = regs[ref_regno].ref_obj_id;
+ if (is_kfunc_release(meta))
+ meta->release_regno = ref_regno;
+
/* Check that BTF function arguments match actual types that the
* verifier sees.
*/
for (i = 0; i < nargs; i++) {
- struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[i + 1];
const struct btf_type *t, *ref_t, *resolve_ret;
enum bpf_arg_type arg_type = ARG_DONTCARE;
u32 regno = i + 1, ref_id, type_size;
bool is_ret_buf_sz = false;
+ struct bpf_reg_state *reg;
int kf_arg_type;
+ reg = ®s[regno];
t = btf_type_skip_modifiers(btf, args[i].type, NULL);
if (is_kfunc_arg_ignore(btf, &args[i]))
@@ -9528,18 +9632,6 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
return -EACCES;
}
- if (reg->ref_obj_id) {
- if (is_kfunc_release(meta) && meta->ref_obj_id) {
- verbose(env, "verifier internal error: more than one arg with ref_obj_id R%d %u %u\n",
- regno, reg->ref_obj_id,
- meta->ref_obj_id);
- return -EFAULT;
- }
- meta->ref_obj_id = reg->ref_obj_id;
- if (is_kfunc_release(meta))
- meta->release_regno = regno;
- }
-
ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
ref_tname = btf_name_by_offset(btf, ref_t->name_off);
@@ -9585,7 +9677,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
return -EFAULT;
}
- if (is_kfunc_release(meta) && reg->ref_obj_id)
+ if (is_kfunc_release(meta) && regno == meta->release_regno)
arg_type |= OBJ_RELEASE;
ret = check_func_arg_reg_off(env, reg, regno, arg_type);
if (ret < 0)
@@ -9747,12 +9839,6 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
}
}
- if (is_kfunc_release(meta) && !meta->release_regno) {
- verbose(env, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
- func_name);
- return -EINVAL;
- }
-
return 0;
}
--
2.30.2
