On Fri, Sep 30, 2022 at 6:39 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > On Fri, Sep 30, 2022 at 6:00 AM Anne Macedo > <annemacedo@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx/ > > >> > > >> Reviewed-by: Isabella Basso <isabbasso@xxxxxxxxxx> > > >> Signed-off-by: Anne Macedo <annemacedo@xxxxxxxxxxxxxxxxxxx> > > >> --- > > >> 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
