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

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

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