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

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

[Anne Macedo]

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

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

[John Fastabend]

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.

How did it get corrupted in the first place? Curious to see if
others want to harden libbpf like this.

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

[Anne Macedo]

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.

> How did it get corrupted in the first place? Curious to see if
> others want to harden libbpf like this.
> 

There's a test case by oss-fuzz [1] that generated this corrupted BTF. 
There's also some C code for replicating this bug [2] using the oss-fuzz 
data.

On a side note, fixing this bug would help oss-fuzz find other, more 
relevant, bugs.

Found the original oss-fuzz report at [3].

[1] https://oss-fuzz.com/download?testcase_id=5041748798210048
[2] https://github.com/libbpf/libbpf/issues/484#issuecomment-1250020929
[3] https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=42345

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

[Andrii Nakryiko]

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.

>
> > How did it get corrupted in the first place? Curious to see if
> > others want to harden libbpf like this.
> >
>
> There's a test case by oss-fuzz [1] that generated this corrupted BTF.
> There's also some C code for replicating this bug [2] using the oss-fuzz
> data.
>
> On a side note, fixing this bug would help oss-fuzz find other, more
> relevant, bugs.
>
> Found the original oss-fuzz report at [3].
>
> [1] https://oss-fuzz.com/download?testcase_id=5041748798210048
> [2] https://github.com/libbpf/libbpf/issues/484#issuecomment-1250020929
> [3] https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=42345

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

[Paul Moore]

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 ...

-- 
paul-moore.com

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

[Andrii Nakryiko]

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.

>
> --
> paul-moore.com

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

[Anne Macedo]

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

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

[Andrii Nakryiko]

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).

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

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

[Anne Macedo]

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