Re: [PATCH] libbpf: add validation to BTF's variable type ID

[Anne Macedo <annemacedo@linux.microsoft.com>]

On 06/10/22 14:07, Andrii Nakryiko wrote:
> On Thu, Oct 6, 2022 at 10:02 AM Anne Macedo
> <annemacedo@linux.microsoft.com> wrote:
>>
>>
>>
>> On 05/10/22 19:42, Andrii Nakryiko wrote:
>>> On Mon, Oct 3, 2022 at 2:26 PM Paul Moore <paul@paul-moore.com> wrote:
>>>>
>>>> On Fri, Sep 30, 2022 at 6:39 PM Andrii Nakryiko
>>>> <andrii.nakryiko@gmail.com> wrote:
>>>>> On Fri, Sep 30, 2022 at 6:00 AM Anne Macedo
>>>>> <annemacedo@linux.microsoft.com> wrote:
>>>>>>
>>>>>> On 29/09/22 23:32, John Fastabend wrote:
>>>>>>> Anne Macedo wrote:
>>>>>>>> If BTF is corrupted, a SEGV may occur due to a null pointer dereference on
>>>>>>>> bpf_object__init_user_btf_map.
>>>>>>>>
>>>>>>>> This patch adds a validation that checks whether the DATASEC's variable
>>>>>>>> type ID is null. If so, it raises a warning.
>>>>>>>>
>>>>>>>> Reported by oss-fuzz project [1].
>>>>>>>>
>>>>>>>> A similar patch for the same issue exists on [2]. However, the code is
>>>>>>>> unreachable when using oss-fuzz data.
>>>>>>>>
>>>>>>>> [1] https://github.com/libbpf/libbpf/issues/484
>>>>>>>> [2] https://patchwork.kernel.org/project/netdevbpf/patch/20211103173213.1376990-3-andrii@kernel.org/
>>>>>>>>
>>>>>>>> Reviewed-by: Isabella Basso <isabbasso@riseup.net>
>>>>>>>> Signed-off-by: Anne Macedo <annemacedo@linux.microsoft.com>
>>>>>>>> ---
>>>>>>>>     tools/lib/bpf/libbpf.c | 4 ++++
>>>>>>>>     1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>>>>>> index 184ce1684dcd..0c88612ab7c4 100644
>>>>>>>> --- a/tools/lib/bpf/libbpf.c
>>>>>>>> +++ b/tools/lib/bpf/libbpf.c
>>>>>>>> @@ -2464,6 +2464,10 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
>>>>>>>>
>>>>>>>>        vi = btf_var_secinfos(sec) + var_idx;
>>>>>>>>        var = btf__type_by_id(obj->btf, vi->type);
>>>>>>>> +    if (!var || !btf_is_var(var)) {
>>>>>>>> +            pr_warn("map #%d: non-VAR type seen", var_idx);
>>>>>>>> +            return -EINVAL;
>>>>>>>> +    }
>>>>>>>>        var_extra = btf_var(var);
>>>>>>>>        map_name = btf__name_by_offset(obj->btf, var->name_off);
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.30.2
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I don't know abouut this. A quick scan looks like this type_by_id is
>>>>>>> used lots of places. And seems corrupted BTF could cause faults
>>>>>>> and confusiuon in other spots as well. I'm not sure its worth making
>>>>>>> libbpf survive corrupted BTF. OTOH this specific patch looks ok.
>>>>>>>
>>>>>>
>>>>>> I was planning on creating a function to validate BTF for these kinds of
>>>>>> corruptions, but decided to keep this patch simple. This could be a good
>>>>>> idea for some future work – moving all of the validations to
>>>>>> bpf_object__init_btf() or to a helper function.
>>>>>
>>>>> This whack-a-mole game of fixing up BTF checks to avoid one specific
>>>>> corruption case is too burdensome. There is plenty of BTF usage before
>>>>> the point which you are fixing, so with some other specific corruption
>>>>> to BTF you can trigger even sooner corruption.
>>>>>
>>>>> As I mentioned on Github. I'm not too worried about ossfuzz generating
>>>>> corrupted BTF because that's not a very realistic scenario. But it
>>>>> would be nice to add some reasonable validation logic for BTF in
>>>>> general, so let's better concentrate on that instead of adding these
>>>>> extra checks.
>>>>
>>>> Reading the comments here and on the associated GH issue, it sounds
>>>> like you would be supportive of this check so long as it was placed in
>>>> bpf_object__init_btf(), is that correct?  Or do you feel this
>>>> particular check falls outside the scope of "reasonable validation
>>>> logic"?  I'm trying to understand what the best next step would be for
>>>> this patch ...
>>>
>>> I think we should bite the bullet and do BTF validation in libbpf. It
>>> doesn't have to be as thorough as what kernel does, but validating
>>> general "structural integrity" of BTF as a first step would make all
>>> these one-off checks throughout entire libbpf source code unnecessary.
>>> I.e., we'll need to check things like: no out of range type IDs, no
>>> out-of-range string offsets, FUNC -> FUNC_PROTO references, DATASEC ->
>>> VAR | FUNC references, etc, etc. Probably make sure we don't have a
>>> loop of struct referencing to itself not through pointer, etc. It's a
>>> bit more upfront work, but it's will make the rest of the code simpler
>>> and will eliminate a bunch of those fuzzer crashes as well.
>>>
>>
>> Thanks for the feedback, I think that sounds like a good plan. I will
>> work on another patch and I wanted to summarize what I should do.
>>
>> So basically, I should place the BTF validation on
>> bpf_object__init_btf(), that should contain validations for:
>>
>> - out of range type IDs;
>> - out of range string offsets;
>> - FUNC -> FUNC_PROTO references;
>> - DATASEC -> VAR | FUNC references;
>> - structs referencing themselves;
>>
> 
> This is just specific things that I could recall immediately. Please
> look at what kernel is validating in kernel/bpf/btf.c. I don't think
> libbpf should be as strict as kernel (e.g., I would reject BTF because
> it has unexpected kflag and stuff like that), we should validate stuff
> that libbpf relies on, but not be overzealous overall (e.g., rejecting
> BTF because kflag is unexpectedly set might be an overkill for libbpf,
> while it makes sense for kernel to be stricter).
> 

Acked. Will start working on that.

>>>>
>>>> --
>>>> paul-moore.com