Factor the return value range calculation logic in check_return_code out of the function in preparation for separating the return value validation logic for BPF_EXIT and bpf_throw(). Signed-off-by: Emil Tsalapatis --- include/linux/bpf_verifier.h | 1 + kernel/bpf/verifier.c | 225 +++++++++++++++++++---------------- 2 files changed, 126 insertions(+), 100 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index ef8e45a362d9..0e639613d8cf 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -266,6 +266,7 @@ struct bpf_reference_state { struct bpf_retval_range { s32 minval; s32 maxval; + bool return_32bit; }; /* state of the program: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index edf5342b982f..6cf2270c343c 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2925,7 +2925,11 @@ static void init_reg_state(struct bpf_verifier_env *env, static struct bpf_retval_range retval_range(s32 minval, s32 maxval) { - return (struct bpf_retval_range){ minval, maxval }; + /* + * return_32bit is set to false by default and set explicitly + * by the caller when necessary. + */ + return (struct bpf_retval_range){ minval, maxval, false }; } #define BPF_MAIN_FUNC (-1) @@ -11146,10 +11150,9 @@ static bool in_rbtree_lock_required_cb(struct bpf_verifier_env *env) return is_rbtree_lock_required_kfunc(kfunc_btf_id); } -static bool retval_range_within(struct bpf_retval_range range, const struct bpf_reg_state *reg, - bool return_32bit) +static bool retval_range_within(struct bpf_retval_range range, const struct bpf_reg_state *reg) { - if (return_32bit) + if (range.return_32bit) return range.minval <= reg->s32_min_value && reg->s32_max_value <= range.maxval; else return range.minval <= reg->smin_value && reg->smax_value <= range.maxval; @@ -11193,7 +11196,7 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) return err; /* enforce R0 return value range, and bpf_callback_t returns 64bit */ - if (!retval_range_within(callee->callback_ret_range, r0, false)) { + if (!retval_range_within(callee->callback_ret_range, r0)) { verbose_invalid_scalar(env, r0, callee->callback_ret_range, "At callback return", "R0"); return -EINVAL; @@ -17837,6 +17840,115 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn) return 0; } + +static bool return_retval_range(struct bpf_verifier_env *env, struct bpf_retval_range *range) +{ + enum bpf_prog_type prog_type = resolve_prog_type(env->prog); + + /* Default return value range. */ + *range = retval_range(0, 1); + + switch (prog_type) { + case BPF_PROG_TYPE_CGROUP_SOCK_ADDR: + switch (env->prog->expected_attach_type) { + case BPF_CGROUP_UDP4_RECVMSG: + case BPF_CGROUP_UDP6_RECVMSG: + case BPF_CGROUP_UNIX_RECVMSG: + case BPF_CGROUP_INET4_GETPEERNAME: + case BPF_CGROUP_INET6_GETPEERNAME: + case BPF_CGROUP_UNIX_GETPEERNAME: + case BPF_CGROUP_INET4_GETSOCKNAME: + case BPF_CGROUP_INET6_GETSOCKNAME: + case BPF_CGROUP_UNIX_GETSOCKNAME: + *range = retval_range(1, 1); + break; + case BPF_CGROUP_INET4_BIND: + case BPF_CGROUP_INET6_BIND: + *range = retval_range(0, 3); + break; + default: + break; + } + break; + case BPF_PROG_TYPE_CGROUP_SKB: + if (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) + *range = retval_range(0, 3); + break; + case BPF_PROG_TYPE_CGROUP_SOCK: + case BPF_PROG_TYPE_SOCK_OPS: + case BPF_PROG_TYPE_CGROUP_DEVICE: + case BPF_PROG_TYPE_CGROUP_SYSCTL: + case BPF_PROG_TYPE_CGROUP_SOCKOPT: + break; + case BPF_PROG_TYPE_RAW_TRACEPOINT: + if (!env->prog->aux->attach_btf_id) + return false; + *range = retval_range(0, 0); + break; + case BPF_PROG_TYPE_TRACING: + switch (env->prog->expected_attach_type) { + case BPF_TRACE_FENTRY: + case BPF_TRACE_FEXIT: + case BPF_TRACE_FSESSION: + *range = retval_range(0, 0); + break; + case BPF_TRACE_RAW_TP: + case BPF_MODIFY_RETURN: + return false; + case BPF_TRACE_ITER: + break; + default: + } + break; + case BPF_PROG_TYPE_KPROBE: + switch (env->prog->expected_attach_type) { + case BPF_TRACE_KPROBE_SESSION: + case BPF_TRACE_UPROBE_SESSION: + break; + default: + return false; + } + break; + case BPF_PROG_TYPE_SK_LOOKUP: + *range = retval_range(SK_DROP, SK_PASS); + break; + + case BPF_PROG_TYPE_LSM: + if (env->prog->expected_attach_type != BPF_LSM_CGROUP) { + /* no range found, any return value is allowed */ + if (!get_func_retval_range(env->prog, range)) + return false; + /* no restricted range, any return value is allowed */ + if (range->minval == S32_MIN && range->maxval == S32_MAX) + return false; + range->return_32bit = true; + } else if (!env->prog->aux->attach_func_proto->type) { + /* Make sure programs that attach to void + * hooks don't try to modify return value. + */ + *range = retval_range(1, 1); + } + break; + + case BPF_PROG_TYPE_NETFILTER: + *range = retval_range(NF_DROP, NF_ACCEPT); + break; + case BPF_PROG_TYPE_STRUCT_OPS: + *range = retval_range(0, 0); + break; + case BPF_PROG_TYPE_EXT: + /* freplace program can return anything as its return value + * depends on the to-be-replaced kernel func or bpf program. + */ + default: + return false; + } + + /* Continue calculating. */ + + return true; +} + static int check_return_code(struct bpf_verifier_env *env, int regno, const char *reg_name) { const char *exit_ctx = "At program exit"; @@ -17845,7 +17957,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char struct bpf_reg_state *reg = reg_state(env, regno); struct bpf_retval_range range = retval_range(0, 1); enum bpf_prog_type prog_type = resolve_prog_type(env->prog); - int err; + int ret, err; struct bpf_func_state *frame = env->cur_state->frame[0]; const bool is_subprog = frame->subprogno; bool return_32bit = false; @@ -17856,7 +17968,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char switch (prog_type) { case BPF_PROG_TYPE_LSM: if (prog->expected_attach_type == BPF_LSM_CGROUP) - /* See below, can be 0 or 0-1 depending on hook. */ + /* See return_retval_range, can be 0 or 0-1 depending on hook. */ break; if (!prog->aux->attach_func_proto->type) return 0; @@ -17914,101 +18026,14 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char return 0; } - switch (prog_type) { - case BPF_PROG_TYPE_CGROUP_SOCK_ADDR: - if (env->prog->expected_attach_type == BPF_CGROUP_UDP4_RECVMSG || - env->prog->expected_attach_type == BPF_CGROUP_UDP6_RECVMSG || - env->prog->expected_attach_type == BPF_CGROUP_UNIX_RECVMSG || - env->prog->expected_attach_type == BPF_CGROUP_INET4_GETPEERNAME || - env->prog->expected_attach_type == BPF_CGROUP_INET6_GETPEERNAME || - env->prog->expected_attach_type == BPF_CGROUP_UNIX_GETPEERNAME || - env->prog->expected_attach_type == BPF_CGROUP_INET4_GETSOCKNAME || - env->prog->expected_attach_type == BPF_CGROUP_INET6_GETSOCKNAME || - env->prog->expected_attach_type == BPF_CGROUP_UNIX_GETSOCKNAME) - range = retval_range(1, 1); - if (env->prog->expected_attach_type == BPF_CGROUP_INET4_BIND || - env->prog->expected_attach_type == BPF_CGROUP_INET6_BIND) - range = retval_range(0, 3); - break; - case BPF_PROG_TYPE_CGROUP_SKB: - if (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) { - range = retval_range(0, 3); - enforce_attach_type_range = tnum_range(2, 3); - } - break; - case BPF_PROG_TYPE_CGROUP_SOCK: - case BPF_PROG_TYPE_SOCK_OPS: - case BPF_PROG_TYPE_CGROUP_DEVICE: - case BPF_PROG_TYPE_CGROUP_SYSCTL: - case BPF_PROG_TYPE_CGROUP_SOCKOPT: - break; - case BPF_PROG_TYPE_RAW_TRACEPOINT: - if (!env->prog->aux->attach_btf_id) - return 0; - range = retval_range(0, 0); - break; - case BPF_PROG_TYPE_TRACING: - switch (env->prog->expected_attach_type) { - case BPF_TRACE_FENTRY: - case BPF_TRACE_FEXIT: - case BPF_TRACE_FSESSION: - range = retval_range(0, 0); - break; - case BPF_TRACE_RAW_TP: - case BPF_MODIFY_RETURN: - return 0; - case BPF_TRACE_ITER: - break; - default: - return -ENOTSUPP; - } - break; - case BPF_PROG_TYPE_KPROBE: - switch (env->prog->expected_attach_type) { - case BPF_TRACE_KPROBE_SESSION: - case BPF_TRACE_UPROBE_SESSION: - range = retval_range(0, 1); - break; - default: - return 0; - } - break; - case BPF_PROG_TYPE_SK_LOOKUP: - range = retval_range(SK_DROP, SK_PASS); - break; + if (prog_type == BPF_PROG_TYPE_STRUCT_OPS && !ret_type) + return 0; - case BPF_PROG_TYPE_LSM: - if (env->prog->expected_attach_type != BPF_LSM_CGROUP) { - /* no range found, any return value is allowed */ - if (!get_func_retval_range(env->prog, &range)) - return 0; - /* no restricted range, any return value is allowed */ - if (range.minval == S32_MIN && range.maxval == S32_MAX) - return 0; - return_32bit = true; - } else if (!env->prog->aux->attach_func_proto->type) { - /* Make sure programs that attach to void - * hooks don't try to modify return value. - */ - range = retval_range(1, 1); - } - break; + if (prog_type == BPF_PROG_TYPE_CGROUP_SKB && (env->prog->expected_attach_type == BPF_CGROUP_INET_EGRESS)) + enforce_attach_type_range = tnum_range(2, 3); - case BPF_PROG_TYPE_NETFILTER: - range = retval_range(NF_DROP, NF_ACCEPT); - break; - case BPF_PROG_TYPE_STRUCT_OPS: - if (!ret_type) - return 0; - range = retval_range(0, 0); - break; - case BPF_PROG_TYPE_EXT: - /* freplace program can return anything as its return value - * depends on the to-be-replaced kernel func or bpf program. - */ - default: + if (!return_retval_range(env, &range)) return 0; - } enforce_retval: if (reg->type != SCALAR_VALUE) { @@ -18021,7 +18046,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char if (err) return err; - if (!retval_range_within(range, reg, return_32bit)) { + if (!retval_range_within(range, reg)) { verbose_invalid_scalar(env, reg, range, exit_ctx, reg_name); if (!is_subprog && prog->expected_attach_type == BPF_LSM_CGROUP && -- 2.49.0 From: Eduard Zingerman The check_return_code function has explicit checks on whether a program type can return void. Factor this logic out to reuse it later for both main progs and subprogs. Signed-off-by: Eduard Zingerman Reviewed-by: Emil Tsalapatis --- kernel/bpf/verifier.c | 64 ++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 28 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 6cf2270c343c..9b49f1a6e8dc 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -17949,6 +17949,28 @@ static bool return_retval_range(struct bpf_verifier_env *env, struct bpf_retval_ return true; } +static bool program_returns_void(struct bpf_verifier_env *env) +{ + const struct bpf_prog *prog = env->prog; + enum bpf_prog_type prog_type = resolve_prog_type(prog); + + switch (prog_type) { + case BPF_PROG_TYPE_LSM: + /* See return_retval_range, for BPF_LSM_CGROUP can be 0 or 0-1 depending on hook. */ + if (prog->expected_attach_type != BPF_LSM_CGROUP && + !prog->aux->attach_func_proto->type) + return true; + break; + case BPF_PROG_TYPE_STRUCT_OPS: + if (!prog->aux->attach_func_proto->type) + return true; + break; + default: + break; + } + return false; +} + static int check_return_code(struct bpf_verifier_env *env, int regno, const char *reg_name) { const char *exit_ctx = "At program exit"; @@ -17965,35 +17987,21 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char /* LSM and struct_ops func-ptr's return type could be "void" */ if (!is_subprog || frame->in_exception_callback_fn) { - switch (prog_type) { - case BPF_PROG_TYPE_LSM: - if (prog->expected_attach_type == BPF_LSM_CGROUP) - /* See return_retval_range, can be 0 or 0-1 depending on hook. */ - break; - if (!prog->aux->attach_func_proto->type) - return 0; - break; - case BPF_PROG_TYPE_STRUCT_OPS: - if (!prog->aux->attach_func_proto->type) - return 0; - - if (frame->in_exception_callback_fn) - break; + if (program_returns_void(env)) + return 0; + } - /* Allow a struct_ops program to return a referenced kptr if it - * matches the operator's return type and is in its unmodified - * form. A scalar zero (i.e., a null pointer) is also allowed. - */ - reg_type = reg->btf ? btf_type_by_id(reg->btf, reg->btf_id) : NULL; - ret_type = btf_type_resolve_ptr(prog->aux->attach_btf, - prog->aux->attach_func_proto->type, - NULL); - if (ret_type && ret_type == reg_type && reg->ref_obj_id) - return __check_ptr_off_reg(env, reg, regno, false); - break; - default: - break; - } + if (!is_subprog && prog_type == BPF_PROG_TYPE_STRUCT_OPS) { + /* Allow a struct_ops program to return a referenced kptr if it + * matches the operator's return type and is in its unmodified + * form. A scalar zero (i.e., a null pointer) is also allowed. + */ + reg_type = reg->btf ? btf_type_by_id(reg->btf, reg->btf_id) : NULL; + ret_type = btf_type_resolve_ptr(prog->aux->attach_btf, + prog->aux->attach_func_proto->type, + NULL); + if (ret_type && ret_type == reg_type && reg->ref_obj_id) + return __check_ptr_off_reg(env, reg, regno, false); } /* eBPF calling convention is such that R0 is used -- 2.49.0 From: Eduard Zingerman Both main progs and subprogs use the same function in the verifier, check_return_code, to verify the type and value range of the register being returned. However, subprogs only need a subset of the logic in check_return_code. this also goes the way - check_return_code explicitly checks whether it is handling a subprogram in multiple places, complicating the logic. Separate the handling of the two into separate fucntions. Signed-off-by: Eduard Zingerman Reviewed-by: Emil Tsalapatis --- kernel/bpf/verifier.c | 65 ++++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 9b49f1a6e8dc..fb4d14516043 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -17979,19 +17979,15 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char struct bpf_reg_state *reg = reg_state(env, regno); struct bpf_retval_range range = retval_range(0, 1); enum bpf_prog_type prog_type = resolve_prog_type(env->prog); - int ret, err; struct bpf_func_state *frame = env->cur_state->frame[0]; - const bool is_subprog = frame->subprogno; - bool return_32bit = false; const struct btf_type *reg_type, *ret_type = NULL; + int ret, err; /* LSM and struct_ops func-ptr's return type could be "void" */ - if (!is_subprog || frame->in_exception_callback_fn) { - if (program_returns_void(env)) - return 0; - } + if (!frame->in_async_callback_fn && program_returns_void(env)) + return 0; - if (!is_subprog && prog_type == BPF_PROG_TYPE_STRUCT_OPS) { + if (prog_type == BPF_PROG_TYPE_STRUCT_OPS) { /* Allow a struct_ops program to return a referenced kptr if it * matches the operator's return type and is in its unmodified * form. A scalar zero (i.e., a null pointer) is also allowed. @@ -18025,15 +18021,6 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char goto enforce_retval; } - if (is_subprog && !frame->in_exception_callback_fn) { - if (reg->type != SCALAR_VALUE) { - verbose(env, "At subprogram exit the register R%d is not a scalar value (%s)\n", - regno, reg_type_str(env, reg->type)); - return -EINVAL; - } - return 0; - } - if (prog_type == BPF_PROG_TYPE_STRUCT_OPS && !ret_type) return 0; @@ -18056,8 +18043,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char if (!retval_range_within(range, reg)) { verbose_invalid_scalar(env, reg, range, exit_ctx, reg_name); - if (!is_subprog && - prog->expected_attach_type == BPF_LSM_CGROUP && + if (prog->expected_attach_type == BPF_LSM_CGROUP && prog_type == BPF_PROG_TYPE_LSM && !prog->aux->attach_func_proto->type) verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n"); @@ -18070,6 +18056,29 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char return 0; } +static int check_subprogram_return_code(struct bpf_verifier_env *env) +{ + struct bpf_reg_state *reg = reg_state(env, BPF_REG_0); + int err; + + err = check_reg_arg(env, BPF_REG_0, SRC_OP); + if (err) + return err; + + if (is_pointer_value(env, BPF_REG_0)) { + verbose(env, "R%d leaks addr as return value\n", BPF_REG_0); + return -EACCES; + } + + if (reg->type != SCALAR_VALUE) { + verbose(env, "At subprogram exit the register R0 is not a scalar value (%s)\n", + reg_type_str(env, reg->type)); + return -EINVAL; + } + + return 0; +} + static void mark_subprog_changes_pkt_data(struct bpf_verifier_env *env, int off) { struct bpf_subprog_info *subprog; @@ -20865,6 +20874,8 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, bool *do_print_state, bool exception_exit) { + struct bpf_func_state *cur_frame = cur_func(env); + /* We must do check_reference_leak here before * prepare_func_exit to handle the case when * state->curframe > 0, it may be a callback function, @@ -20898,7 +20909,21 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return 0; } - err = check_return_code(env, BPF_REG_0, "R0"); + /* + * Return from a regular global subprogram differs from return + * from the main program or async/exception callback. + * Main program exit implies return code restrictions + * that depend on program type. + * Exit from exception callback is equivalent to main program exit. + * Exit from async callback implies return code restrictions + * that depend on async scheduling mechanism. + */ + if (cur_frame->subprogno && + !cur_frame->in_async_callback_fn && + !cur_frame->in_exception_callback_fn) + err = check_subprogram_return_code(env); + else + err = check_return_code(env, BPF_REG_0, "R0"); if (err) return err; return PROCESS_BPF_EXIT; -- 2.49.0 Global subprogs are currently not allowed to return void. Adjust verifier logic to allow global functions with a void return type. Signed-off-by: Emil Tsalapatis --- kernel/bpf/btf.c | 7 +- kernel/bpf/verifier.c | 64 ++++++++++++++++--- .../selftests/bpf/progs/exceptions_fail.c | 2 +- .../selftests/bpf/progs/test_global_func7.c | 2 +- 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7708958e3fb8..51f8d3dece5d 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -7836,15 +7836,16 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog) tname, nargs, MAX_BPF_FUNC_REG_ARGS); return -EINVAL; } - /* check that function returns int, exception cb also requires this */ + /* check that function is void or returns int, exception cb also requires this */ t = btf_type_by_id(btf, t->type); while (btf_type_is_modifier(t)) t = btf_type_by_id(btf, t->type); - if (!btf_type_is_int(t) && !btf_is_any_enum(t)) { + if (!btf_type_is_void(t) && !btf_type_is_int(t) && !btf_is_any_enum(t)) { if (!is_global) return -EINVAL; bpf_log(log, - "Global function %s() doesn't return scalar. Only those are supported.\n", + "Global function %s() return value not void or scalar. " + "Only those are supported.\n", tname); return -EINVAL; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index fb4d14516043..702b3583cd78 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -444,6 +444,29 @@ static bool subprog_is_global(const struct bpf_verifier_env *env, int subprog) return aux && aux[subprog].linkage == BTF_FUNC_GLOBAL; } +static bool subprog_returns_void(struct bpf_verifier_env *env, int subprog) +{ + const struct btf_type *type, *func, *func_proto; + const struct btf *btf = env->prog->aux->btf; + u32 btf_id; + + btf_id = env->prog->aux->func_info[subprog].type_id; + + func = btf_type_by_id(btf, btf_id); + if (verifier_bug_if(!func, env, "btf_id %u not found", btf_id)) + return false; + + func_proto = btf_type_by_id(btf, func->type); + if (!func_proto) + return false; + + type = btf_type_skip_modifiers(btf, func_proto->type, NULL); + if (!type) + return false; + + return btf_type_is_void(type); +} + static const char *subprog_name(const struct bpf_verifier_env *env, int subprog) { struct bpf_func_info *info; @@ -10861,9 +10884,11 @@ static int check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn, subprog_aux(env, subprog)->called = true; clear_caller_saved_regs(env, caller->regs); - /* All global functions return a 64-bit SCALAR_VALUE */ - mark_reg_unknown(env, caller->regs, BPF_REG_0); - caller->regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; + /* All non-void global functions return a 64-bit SCALAR_VALUE. */ + if (!subprog_returns_void(env, subprog)) { + mark_reg_unknown(env, caller->regs, BPF_REG_0); + caller->regs[BPF_REG_0].subreg_def = DEF_NOT_SUBREG; + } /* continue with next insn after call */ return 0; @@ -17952,7 +17977,7 @@ static bool return_retval_range(struct bpf_verifier_env *env, struct bpf_retval_ static bool program_returns_void(struct bpf_verifier_env *env) { const struct bpf_prog *prog = env->prog; - enum bpf_prog_type prog_type = resolve_prog_type(prog); + enum bpf_prog_type prog_type = prog->type; switch (prog_type) { case BPF_PROG_TYPE_LSM: @@ -17965,6 +17990,16 @@ static bool program_returns_void(struct bpf_verifier_env *env) if (!prog->aux->attach_func_proto->type) return true; break; + case BPF_PROG_TYPE_EXT: + /* + * If the actual program is an extension, let it + * return void - attaching will succeed only if the + * program being replaced also returns void, and since + * it has passed verification its actual type doesn't matter. + */ + if (subprog_returns_void(env, 0)) + return true; + break; default: break; } @@ -17981,7 +18016,7 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char enum bpf_prog_type prog_type = resolve_prog_type(env->prog); struct bpf_func_state *frame = env->cur_state->frame[0]; const struct btf_type *reg_type, *ret_type = NULL; - int ret, err; + int err; /* LSM and struct_ops func-ptr's return type could be "void" */ if (!frame->in_async_callback_fn && program_returns_void(env)) @@ -18059,8 +18094,13 @@ static int check_return_code(struct bpf_verifier_env *env, int regno, const char static int check_subprogram_return_code(struct bpf_verifier_env *env) { struct bpf_reg_state *reg = reg_state(env, BPF_REG_0); + struct bpf_func_state *cur_frame = cur_func(env); int err; + if (subprog_is_global(env, cur_frame->subprogno) && + subprog_returns_void(env, cur_frame->subprogno)) + return 0; + err = check_reg_arg(env, BPF_REG_0, SRC_OP); if (err) return err; @@ -24560,10 +24600,18 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog) if (subprog_is_exc_cb(env, subprog)) { state->frame[0]->in_exception_callback_fn = true; - /* We have already ensured that the callback returns an integer, just - * like all global subprogs. We need to determine it only has a single - * scalar argument. + + /* + * Global functions are scalar or void, make sure + * we return a scalar. */ + if (subprog_returns_void(env, subprog)) { + verbose(env, "exception cb cannot return void\n"); + ret = -EINVAL; + goto out; + } + + /* Also ensure the callback only has a single scalar argument. */ if (sub->arg_cnt != 1 || sub->args[0].arg_type != ARG_ANYTHING) { verbose(env, "exception cb only supports single integer argument\n"); ret = -EINVAL; diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c index 8a0fdff89927..d28ecc4ee2d0 100644 --- a/tools/testing/selftests/bpf/progs/exceptions_fail.c +++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c @@ -51,7 +51,7 @@ __noinline int exception_cb_ok_arg_small(int a) SEC("?tc") __exception_cb(exception_cb_bad_ret_type) -__failure __msg("Global function exception_cb_bad_ret_type() doesn't return scalar.") +__failure __msg("Global function exception_cb_bad_ret_type() return value not void or scalar.") int reject_exception_cb_type_1(struct __sk_buff *ctx) { bpf_throw(0); diff --git a/tools/testing/selftests/bpf/progs/test_global_func7.c b/tools/testing/selftests/bpf/progs/test_global_func7.c index f182febfde3c..9e59625c1c92 100644 --- a/tools/testing/selftests/bpf/progs/test_global_func7.c +++ b/tools/testing/selftests/bpf/progs/test_global_func7.c @@ -12,7 +12,7 @@ void foo(struct __sk_buff *skb) } SEC("tc") -__failure __msg("foo() doesn't return scalar") +__success int global_func7(struct __sk_buff *skb) { foo(skb); -- 2.49.0 Add additional testing for void global functions. The tests ensure that calls to void global functions properly keep R0 invalid. Also make sure that exception callbacks still require a return value. Signed-off-by: Emil Tsalapatis --- .../selftests/bpf/prog_tests/exceptions.c | 1 + .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 24 +++++++++++++ .../testing/selftests/bpf/progs/exceptions.c | 14 ++++++++ .../selftests/bpf/progs/exceptions_fail.c | 35 +++++++++++++++++-- .../bpf/progs/freplace_int_with_void.c | 14 ++++++++ .../selftests/bpf/progs/freplace_void.c | 12 +++++++ .../bpf/progs/verifier_global_subprogs.c | 19 ++++++++++ 7 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 tools/testing/selftests/bpf/progs/freplace_int_with_void.c create mode 100644 tools/testing/selftests/bpf/progs/freplace_void.c diff --git a/tools/testing/selftests/bpf/prog_tests/exceptions.c b/tools/testing/selftests/bpf/prog_tests/exceptions.c index 516f4a13013c..84ab73e08b0e 100644 --- a/tools/testing/selftests/bpf/prog_tests/exceptions.c +++ b/tools/testing/selftests/bpf/prog_tests/exceptions.c @@ -83,6 +83,7 @@ static void test_exceptions_success(void) RUN_SUCCESS(exception_assert_range_with, 1); RUN_SUCCESS(exception_bad_assert_range, 0); RUN_SUCCESS(exception_bad_assert_range_with, 10); + RUN_SUCCESS(exception_throw_from_void_global, 11); #define RUN_EXT(load_ret, attach_err, expr, msg, after_link) \ { \ diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c index f29fc789c14b..23d933f1aec6 100644 --- a/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c +++ b/tools/testing/selftests/bpf/prog_tests/fexit_bpf2bpf.c @@ -347,6 +347,17 @@ static void test_func_sockmap_update(void) prog_name, false, NULL); } +static void test_func_replace_void(void) +{ + const char *prog_name[] = { + "freplace/foo", + }; + test_fexit_bpf2bpf_common("./freplace_void.bpf.o", + "./test_global_func7.bpf.o", + ARRAY_SIZE(prog_name), + prog_name, false, NULL); +} + static void test_obj_load_failure_common(const char *obj_file, const char *target_obj_file, const char *exp_msg) @@ -432,6 +443,15 @@ static void test_func_replace_global_func(void) prog_name, false, NULL); } +static void test_func_replace_int_with_void(void) +{ + /* Make sure we can't freplace with the wrong type */ + test_obj_load_failure_common("freplace_int_with_void.bpf.o", + "./test_global_func2.bpf.o", + "Return type UNKNOWN of test_freplace_int_with_void()" + " doesn't match type INT of global_func2()"); +} + static int find_prog_btf_id(const char *name, __u32 attach_prog_fd) { struct bpf_prog_info info = {}; @@ -597,4 +617,8 @@ void serial_test_fexit_bpf2bpf(void) test_fentry_to_cgroup_bpf(); if (test__start_subtest("func_replace_progmap")) test_func_replace_progmap(); + if (test__start_subtest("freplace_int_with_void")) + test_func_replace_int_with_void(); + if (test__start_subtest("freplace_void")) + test_func_replace_void(); } diff --git a/tools/testing/selftests/bpf/progs/exceptions.c b/tools/testing/selftests/bpf/progs/exceptions.c index f09cd14d8e04..4206f59d7b86 100644 --- a/tools/testing/selftests/bpf/progs/exceptions.c +++ b/tools/testing/selftests/bpf/progs/exceptions.c @@ -109,6 +109,20 @@ int exception_tail_call(struct __sk_buff *ctx) { return ret + 8; } +__weak +void throw_11(void) +{ + bpf_throw(11); +} + +SEC("tc") +int exception_throw_from_void_global(struct __sk_buff *ctx) +{ + throw_11(); + + return 0; +} + __noinline int exception_ext_global(struct __sk_buff *ctx) { volatile int ret = 0; diff --git a/tools/testing/selftests/bpf/progs/exceptions_fail.c b/tools/testing/selftests/bpf/progs/exceptions_fail.c index d28ecc4ee2d0..275ad6fe4a04 100644 --- a/tools/testing/selftests/bpf/progs/exceptions_fail.c +++ b/tools/testing/selftests/bpf/progs/exceptions_fail.c @@ -29,11 +29,15 @@ struct { private(A) struct bpf_spin_lock lock; private(A) struct bpf_rb_root rbtree __contains(foo, node); -__noinline void *exception_cb_bad_ret_type(u64 cookie) +__noinline void *exception_cb_bad_ret_type1(u64 cookie) { return NULL; } +__noinline void exception_cb_bad_ret_type2(u64 cookie) +{ +} + __noinline int exception_cb_bad_arg_0(void) { return 0; @@ -50,8 +54,8 @@ __noinline int exception_cb_ok_arg_small(int a) } SEC("?tc") -__exception_cb(exception_cb_bad_ret_type) -__failure __msg("Global function exception_cb_bad_ret_type() return value not void or scalar.") +__exception_cb(exception_cb_bad_ret_type1) +__failure __msg("Global function exception_cb_bad_ret_type1() return value not void or scalar.") int reject_exception_cb_type_1(struct __sk_buff *ctx) { bpf_throw(0); @@ -85,6 +89,15 @@ int reject_exception_cb_type_4(struct __sk_buff *ctx) return 0; } +SEC("?tc") +__exception_cb(exception_cb_bad_ret_type2) +__failure __msg("exception cb cannot return void") +int reject_exception_cb_type_5(struct __sk_buff *ctx) +{ + bpf_throw(0); + return 0; +} + __noinline static int timer_cb(void *map, int *key, struct bpf_timer *timer) { @@ -346,4 +359,20 @@ int reject_exception_throw_cb_diff(struct __sk_buff *ctx) return 0; } +__weak +void foo(void) +{ + bpf_throw(1); +} + +SEC("?fentry/bpf_check") +__failure __msg("At program exit the register R1 has smin=1 smax=1 should") +int reject_out_of_range_global_throw(struct __sk_buff *skb) +{ + foo(); + + return 0; +} + + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/freplace_int_with_void.c b/tools/testing/selftests/bpf/progs/freplace_int_with_void.c new file mode 100644 index 000000000000..8b6f5776cf0f --- /dev/null +++ b/tools/testing/selftests/bpf/progs/freplace_int_with_void.c @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include +#include + +volatile int data; + +SEC("freplace/global_func2") +void test_freplace_int_with_void(struct __sk_buff *skb) +{ + data = 1; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/freplace_void.c b/tools/testing/selftests/bpf/progs/freplace_void.c new file mode 100644 index 000000000000..cfa4af00991d --- /dev/null +++ b/tools/testing/selftests/bpf/progs/freplace_void.c @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include + +volatile int data; + +SEC("freplace/foo") +void test_freplace_void(struct __sk_buff *skb) +{ +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c index 20904cd2baa2..f02012a2fbaa 100644 --- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c +++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c @@ -388,4 +388,23 @@ int arg_tag_dynptr(struct xdp_md *ctx) return subprog_dynptr(&dptr); } +__weak +void foo(void) +{ +} + +SEC("?tc") +__failure __msg("R0 !read_ok") +int return_from_void_global(struct __sk_buff *skb) +{ + foo(); + + asm volatile( + "r1 = r0;" + ::: + ); + + return 0; +} + char _license[] SEC("license") = "GPL"; -- 2.49.0