On Mon, 2023-12-04 at 15:39 -0800, Andrii Nakryiko wrote:
[...]
> @@ -6845,7 +6845,47 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog)
> * Only PTR_TO_CTX and SCALAR are supported atm.
> */
> for (i = 0; i < nargs; i++) {
> + bool is_nonnull = false;
> + const char *tag;
> +
> t = btf_type_by_id(btf, args[i].type);
> +
> + tag = btf_find_decl_tag_value(btf, fn_t, i, "arg:");
Nit: this does a linear scan over all BTF type ids for each
function parameter, which is kind of ugly.
> + if (IS_ERR(tag) && PTR_ERR(tag) == -ENOENT) {
> + tag = NULL;
> + } else if (IS_ERR(tag)) {
> + bpf_log(log, "arg#%d type's tag fetching failure: %ld\n", i, PTR_ERR(tag));
> + return PTR_ERR(tag);
> + }
> + /* 'arg:<tag>' decl_tag takes precedence over derivation of
> + * register type from BTF type itself
> + */
> + if (tag) {
> + /* disallow arg tags in static subprogs */
> + if (!is_global) {
> + bpf_log(log, "arg#%d type tag is not supported in static functions\n", i);
> + return -EOPNOTSUPP;
> + }
Nit: this would be annoying if someone would add/remove 'static' a few
times while developing BPF program. Are there safety reasons to
forbid this?
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5787b7fd16ba..61e778dbde10 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -9268,9 +9268,30 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
> ret = check_func_arg_reg_off(env, reg, regno, ARG_DONTCARE);
> if (ret < 0)
> return ret;
> -
> if (check_mem_reg(env, reg, regno, arg->mem_size))
> return -EINVAL;
> + if (!(arg->arg_type & PTR_MAYBE_NULL) && (reg->type & PTR_MAYBE_NULL)) {
> + bpf_log(log, "arg#%d is expected to be non-NULL\n", i);
> + return -EINVAL;
> + }
> + } else if (arg->arg_type == ARG_PTR_TO_PACKET_META) {
> + if (reg->type != PTR_TO_PACKET_META) {
> + bpf_log(log, "arg#%d expected pkt_meta, but got %s\n",
> + i, reg_type_str(env, reg->type));
> + return -EINVAL;
> + }
> + } else if (arg->arg_type == ARG_PTR_TO_PACKET_DATA) {
> + if (reg->type != PTR_TO_PACKET) {
I think it is necessary to check that 'reg->umax_value == 0'.
check_packet_access() uses reg->umax_value to bump
env->prog->aux->max_pkt_offset. When body of a global function is
verified it starts with 'umax_value == 0'.
Might be annoying from usability POV, however.
> + bpf_log(log, "arg#%d expected pkt, but got %s\n",
> + i, reg_type_str(env, reg->type));
> + return -EINVAL;
> + }
> + } else if (arg->arg_type == ARG_PTR_TO_PACKET_END) {
> + if (reg->type != PTR_TO_PACKET_END) {
> + bpf_log(log, "arg#%d expected pkt_end, but got %s\n",
> + i, reg_type_str(env, reg->type));
> + return -EINVAL;
> + }
> } else {
> bpf_log(log, "verifier bug: unrecognized arg#%d type %d\n",
> i, arg->arg_type);
[...]
