Message-ID: <b9258020-cd38-b818-e3a9-4f6d9cdf6b88@fb.com>
Date:   Fri, 26 Jun 2020 16:29:23 -0700
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Andrii Nakryiko <andriin@...com>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving



On 6/26/20 2:40 PM, Andrii Nakryiko wrote:
> On Fri, Jun 26, 2020 at 2:37 PM Yonghong Song <yhs@...com> wrote:
>>
>>
>>
>> On 6/25/20 3:12 PM, Jiri Olsa wrote:
>>> Now when we moved the helpers btf_id arrays into .BTF_ids section,
>>> we can remove the code that resolve those IDs in runtime.
>>>
>>> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
>>> ---
>>>    kernel/bpf/btf.c | 90 +++++-------------------------------------------
>>>    1 file changed, 8 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index 4c3007f428b1..4da6b0770ff9 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
>>> @@ -4079,96 +4079,22 @@ int btf_struct_access(struct bpf_verifier_log *log,
>>>        return -EINVAL;
>>>    }
>>>
>>> -static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
>>> -                                int arg)
>>> +int btf_resolve_helper_id(struct bpf_verifier_log *log,
>>> +                       const struct bpf_func_proto *fn, int arg)
>>>    {
>>> -     char fnname[KSYM_SYMBOL_LEN + 4] = "btf_";
>>> -     const struct btf_param *args;
>>> -     const struct btf_type *t;
>>> -     const char *tname, *sym;
>>> -     u32 btf_id, i;
>>> +     int id;
>>>
>>> -     if (IS_ERR(btf_vmlinux)) {
>>> -             bpf_log(log, "btf_vmlinux is malformed\n");
>>> +     if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
>>>                return -EINVAL;
>>> -     }
>>> -
>>> -     sym = kallsyms_lookup((long)fn, NULL, NULL, NULL, fnname + 4);
>>> -     if (!sym) {
>>> -             bpf_log(log, "kernel doesn't have kallsyms\n");
>>> -             return -EFAULT;
>>> -     }
>>>
>>> -     for (i = 1; i <= btf_vmlinux->nr_types; i++) {
>>> -             t = btf_type_by_id(btf_vmlinux, i);
>>> -             if (BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF)
>>> -                     continue;
>>> -             tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
>>> -             if (!strcmp(tname, fnname))
>>> -                     break;
>>> -     }
>>> -     if (i > btf_vmlinux->nr_types) {
>>> -             bpf_log(log, "helper %s type is not found\n", fnname);
>>> -             return -ENOENT;
>>> -     }
>>> -
>>> -     t = btf_type_by_id(btf_vmlinux, t->type);
>>> -     if (!btf_type_is_ptr(t))
>>> -             return -EFAULT;
>>> -     t = btf_type_by_id(btf_vmlinux, t->type);
>>> -     if (!btf_type_is_func_proto(t))
>>> -             return -EFAULT;
>>> -
>>> -     args = (const struct btf_param *)(t + 1);
>>> -     if (arg >= btf_type_vlen(t)) {
>>> -             bpf_log(log, "bpf helper %s doesn't have %d-th argument\n",
>>> -                     fnname, arg);
>>> +     if (WARN_ON_ONCE(!fn->btf_id))
>>
>> The original code does not have this warning. It directly did
>> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID.
>>
>>>                return -EINVAL;
>>> -     }
>>>
>>> -     t = btf_type_by_id(btf_vmlinux, args[arg].type);
>>> -     if (!btf_type_is_ptr(t) || !t->type) {
>>> -             /* anything but the pointer to struct is a helper config bug */
>>> -             bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n");
>>> -             return -EFAULT;
>>> -     }
>>> -     btf_id = t->type;
>>> -     t = btf_type_by_id(btf_vmlinux, t->type);
>>> -     /* skip modifiers */
>>> -     while (btf_type_is_modifier(t)) {
>>> -             btf_id = t->type;
>>> -             t = btf_type_by_id(btf_vmlinux, t->type);
>>> -     }
>>> -     if (!btf_type_is_struct(t)) {
>>> -             bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n");
>>> -             return -EFAULT;
>>> -     }
>>> -     bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4,
>>> -             arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off));
>>> -     return btf_id;
>>> -}
>>> +     id = fn->btf_id[arg];
>>
>> The corresponding BTF_ID definition here is:
>>     BTF_ID_LIST(bpf_skb_output_btf_ids)
>>     BTF_ID(struct, sk_buff)
>>
>> The bpf helper writer needs to ensure proper declarations
>> of BTF_IDs like the above matching helpers definition.
>> Support we have arg1 and arg3 as BTF_ID. then the list
>> definition may be
>>
>>     BTF_ID_LIST(bpf_skb_output_btf_ids)
>>     BTF_ID(struct, sk_buff)
>>     BTF_ID(struct, __unused)
>>     BTF_ID(struct, task_struct)
>>
>> This probably okay, I guess.
>>
>>>
>>> -int btf_resolve_helper_id(struct bpf_verifier_log *log,
>>> -                       const struct bpf_func_proto *fn, int arg)
>>> -{
>>> -     int *btf_id = &fn->btf_id[arg];
>>> -     int ret;
>>> -
>>> -     if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
>>> +     if (!id || id > btf_vmlinux->nr_types)
>>>                return -EINVAL;
>>
>> id == 0 if btf_id cannot be resolved by resolve_btfids, right?
>> when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
>> application did incorrect transformation?
>>
>> Anyway, this is to resolve helper meta btf_id. Even if you
>> return a btf_id > btf_vmlinux->nr_types, verifier will reject
>> since it will never be the same as the real parameter btf_id.
>> I would drop id > btf_vmlinux->nr_types here. This should never
>> happen for a correct tool. Even if it does, verifier will take
>> care of it.
>>
> 
> I'd love to hear Alexei's thoughts about this change as well. Jiri
> removed not just BTF ID resolution, but also all the sanity checks.
> This now means more trust in helper definitions to not screw up
> anything. It's probably OK, but still something to consciously think
> about.

The kernel will have to trust the result. Otherwise, things may go bad.
For example, if the tool incorrectly calculated the type_id to be 100
although the correct id is 50, and the bpf program happens to have
the type id 100 for the parameter. Then the helper is allowed, this
could crash the kernel... I do not know whether we have security
issue here or not.

> 
>>> -
>>> -     ret = READ_ONCE(*btf_id);
>>> -     if (ret)
>>> -             return ret;
>>> -     /* ok to race the search. The result is the same */
>>> -     ret = __btf_resolve_helper_id(log, fn->func, arg);
>>> -     if (!ret) {
>>> -             /* Function argument cannot be type 'void' */
>>> -             bpf_log(log, "BTF resolution bug\n");
>>> -             return -EFAULT;
>>> -     }
>>> -     WRITE_ONCE(*btf_id, ret);
>>> -     return ret;
>>> +     return id;
>>>    }
>>>
>>>    static int __get_type_size(struct btf *btf, u32 btf_id,
>>>

Powered by blists - more mailing lists