On Sat, Nov 19, 2022 at 03:07:46PM -0600, David Vernet wrote:
> @@ -6887,6 +6895,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,
> }
>
> reg->type = PTR_TO_MEM | PTR_MAYBE_NULL;
> +
No need to add empty line here.
> reg->id = ++env->id_gen;
>
> continue;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 195d24316750..3a90a1c7613f 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -557,7 +557,7 @@ static bool is_cmpxchg_insn(const struct bpf_insn *insn)
> static const char *reg_type_str(struct bpf_verifier_env *env,
> enum bpf_reg_type type)
> {
> - char postfix[16] = {0}, prefix[32] = {0};
> + char postfix[16] = {0}, prefix[64] = {0};
> static const char * const str[] = {
> [NOT_INIT] = "?",
> [SCALAR_VALUE] = "scalar",
> @@ -589,16 +589,14 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
> strncpy(postfix, "_or_null", 16);
> }
>
> - if (type & MEM_RDONLY)
> - strncpy(prefix, "rdonly_", 32);
> - if (type & MEM_RINGBUF)
> - strncpy(prefix, "ringbuf_", 32);
> - if (type & MEM_USER)
> - strncpy(prefix, "user_", 32);
> - if (type & MEM_PERCPU)
> - strncpy(prefix, "percpu_", 32);
> - if (type & PTR_UNTRUSTED)
> - strncpy(prefix, "untrusted_", 32);
> + snprintf(prefix, sizeof(prefix), "%s%s%s%s%s%s",
> + type & MEM_RDONLY ? "rdonly_" : "",
> + type & MEM_RINGBUF ? "ringbuf_" : "",
> + type & MEM_USER ? "user_" : "",
> + type & MEM_PERCPU ? "percpu_" : "",
> + type & PTR_UNTRUSTED ? "untrusted_" : "",
> + type & PTR_TRUSTED ? "trusted_" : ""
> + );
Nice. Could have been a separate patch, but ok.
>
> found:
> - if (reg->type == PTR_TO_BTF_ID) {
> + if (reg->type == PTR_TO_BTF_ID || (reg->type & PTR_TRUSTED)) {
No need for (). The operator precedence is pretty clear.
> /* For bpf_sk_release, it needs to match against first member
> * 'struct sock_common', hence make an exception for it. This
> * allows bpf_sk_release to work for multiple socket types.
> @@ -6058,6 +6070,8 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> */
> case PTR_TO_BTF_ID:
> case PTR_TO_BTF_ID | MEM_ALLOC:
> + case PTR_TO_BTF_ID | PTR_TRUSTED:
> + case PTR_TO_BTF_ID | MEM_ALLOC | PTR_TRUSTED:
> /* When referenced PTR_TO_BTF_ID is passed to release function,
> * it's fixed offset must be 0. In the other cases, fixed offset
> * can be non-zero.
> @@ -7942,6 +7956,25 @@ static bool is_kfunc_arg_kptr_get(struct bpf_kfunc_call_arg_meta *meta, int arg)
> return arg == 0 && (meta->kfunc_flags & KF_KPTR_GET);
> }
>
> +static bool is_trusted_reg(const struct bpf_reg_state *reg)
> +{
> + /* A referenced register is always trusted. */
> + if (reg->ref_obj_id)
> + return true;
> +
> + /* If a register is not referenced, it is trusted if it has either the
> + * MEM_ALLOC or PTR_TRUSTED type modifiers, and no others. Some of the
> + * other type modifiers may be safe, but we elect to take an opt-in
> + * approach here as some (e.g. PTR_UNTRUSTED and PTR_MAYBE_NULL) are
> + * not.
> + *
> + * Eventually, we should make PTR_TRUSTED the single source of truth
> + * for whether a register is trusted.
> + */
> + return (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS) &&
No need for ().
> + !bpf_type_has_unsafe_modifiers(reg->type);
> +}
> +
...
> - if (is_kfunc_release(meta) && reg->ref_obj_id)
> + if (is_kfunc_release(meta) && reg->ref_obj_id) {
> arg_type |= OBJ_RELEASE;
> + if (bpf_type_has_unsafe_modifiers(reg->type)) {
> + verbose(env, "R%d release reg has unsafe modifiers\n", i);
> + return -EINVAL;
> + }
This part is a bit controversial, sicne it messes up the verifier messages.
Meaning that doing the check that early is losing important context.
> + }
> ret = check_func_arg_reg_off(env, reg, regno, arg_type);
> if (ret < 0)
> return ret;
> @@ -8705,7 +8745,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> break;
> case KF_ARG_PTR_TO_BTF_ID:
> /* Only base_type is checked, further checks are done here */
> - if (reg->type != PTR_TO_BTF_ID &&
> + if (base_type(reg->type) != PTR_TO_BTF_ID &&
> (!reg2btf_ids[base_type(reg->type)] || type_flag(reg->type))) {
> verbose(env, "arg#%d expected pointer to btf or socket\n", i);
With base_type() addition maybe the bpf_type_has_unsafe_modifiers() check
should be done here ?
Then test_verifier wouldn't need to change.
It's not the change itself that is a concern, but the loss of context in the messages.
I guess one can argue that erroring on PTR_TO_BTF_ID | PTR_MAYBE_NULL
with "reg has unsafe modifiers" is just as correct as saying
"expected pointer to btf or socket" a bit later.
Both could be improved.
If we keep it early while doing is_kfunc_release(meta) && reg->ref_obj_id
we could say:
"%s is not allowed in release function"
reg_type_str(env,reg->type)
Which for verifier/calls.c test case will be:
"ptr_prog_test_ref_kfunc_or_null is not allowed in release function"
If we do it later here it could be:
"arg#$d is %s. Expected %s or socket",
reg_type_str(env,reg->type)
reg_type_str(env,base_type(reg->type) | type_flag(reg->type) & ~BPF_REG_TRUSTED_MODIFIERS)
"arg#0 is ptr_prog_test_ref_kfunc_or_null. Expected ptr_prog_test_ref_kfunc or socket"
which is even better and it will make it easier for user to fix the code.
wdyt?
