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

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

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;

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