* [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str @ 2023-12-14 7:50 Wentao Zhang 2023-12-14 15:34 ` Daniel Borkmann 2023-12-14 23:36 ` Andrii Nakryiko 0 siblings, 2 replies; 4+ messages in thread From: Wentao Zhang @ 2023-12-14 7:50 UTC (permalink / raw) To: wentao.zhang, ast, daniel; +Cc: bpf The function btf_str_by_offset may return NULL when used as an input argument for btf_add_str in the context of btf_rewrite_str. The added check ensures that both the input string (s) and the BTF object (btf) are non-null before proceeding with the function logic. If either is null, the function returns an error code indicating an invalid argument. Found by our static analysis tool. Signed-off-by: Wentao Zhang <wentao.zhang@windriver.com> --- tools/lib/bpf/btf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index fd2309512978..a6a00bdc7151 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -1612,6 +1612,8 @@ int btf__find_str(struct btf *btf, const char *s) int btf__add_str(struct btf *btf, const char *s) { int off; + if(!s || !btf) + return libbpf_err(-EINVAL); if (btf->base_btf) { off = btf__find_str(btf->base_btf, s); -- 2.35.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str 2023-12-14 7:50 [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str Wentao Zhang @ 2023-12-14 15:34 ` Daniel Borkmann 2023-12-14 19:34 ` John Fastabend 2023-12-14 23:36 ` Andrii Nakryiko 1 sibling, 1 reply; 4+ messages in thread From: Daniel Borkmann @ 2023-12-14 15:34 UTC (permalink / raw) To: Wentao Zhang, ast; +Cc: bpf On 12/14/23 8:50 AM, Wentao Zhang wrote: > The function btf_str_by_offset may return NULL when used as an > input argument for btf_add_str in the context of btf_rewrite_str. > The added check ensures that both the input string (s) and the > BTF object (btf) are non-null before proceeding with the function > logic. If either is null, the function returns an error code > indicating an invalid argument. > > Found by our static analysis tool. > > Signed-off-by: Wentao Zhang <wentao.zhang@windriver.com> > --- > tools/lib/bpf/btf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index fd2309512978..a6a00bdc7151 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -1612,6 +1612,8 @@ int btf__find_str(struct btf *btf, const char *s) > int btf__add_str(struct btf *btf, const char *s) > { > int off; (nit: empty line after declaration) > + if(!s || !btf) > + return libbpf_err(-EINVAL); > > if (btf->base_btf) { > off = btf__find_str(btf->base_btf, s); > If feels a bit off that in this library helper function we'd validate the inputs but not in others. Alternative could be : diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 63033c334320..18574fc017d9 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -1735,6 +1735,7 @@ static int btf_rewrite_str(__u32 *str_off, void *ctx) { struct btf_pipe *p = ctx; long mapped_off; + const char *s; int off, err; if (!*str_off) /* nothing to do for empty strings */ @@ -1746,7 +1747,11 @@ static int btf_rewrite_str(__u32 *str_off, void *ctx) return 0; } - off = btf__add_str(p->dst, btf__str_by_offset(p->src, *str_off)); + s = btf__str_by_offset(p->src, *str_off); + if (!s) + return -EINVAL; + + off = btf__add_str(p->dst, s); if (off < 0) return off; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str 2023-12-14 15:34 ` Daniel Borkmann @ 2023-12-14 19:34 ` John Fastabend 0 siblings, 0 replies; 4+ messages in thread From: John Fastabend @ 2023-12-14 19:34 UTC (permalink / raw) To: Daniel Borkmann, Wentao Zhang, ast; +Cc: bpf Daniel Borkmann wrote: > On 12/14/23 8:50 AM, Wentao Zhang wrote: > > The function btf_str_by_offset may return NULL when used as an > > input argument for btf_add_str in the context of btf_rewrite_str. > > The added check ensures that both the input string (s) and the > > BTF object (btf) are non-null before proceeding with the function > > logic. If either is null, the function returns an error code > > indicating an invalid argument. > > > > Found by our static analysis tool. > > > > Signed-off-by: Wentao Zhang <wentao.zhang@windriver.com> > > --- > > tools/lib/bpf/btf.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > > index fd2309512978..a6a00bdc7151 100644 > > --- a/tools/lib/bpf/btf.c > > +++ b/tools/lib/bpf/btf.c > > @@ -1612,6 +1612,8 @@ int btf__find_str(struct btf *btf, const char *s) > > int btf__add_str(struct btf *btf, const char *s) > > { > > int off; > > (nit: empty line after declaration) > > > + if(!s || !btf) > > + return libbpf_err(-EINVAL); > > > > if (btf->base_btf) { > > off = btf__find_str(btf->base_btf, s); > > > > If feels a bit off that in this library helper function we'd validate the inputs > but not in others. Alternative could be : > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index 63033c334320..18574fc017d9 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -1735,6 +1735,7 @@ static int btf_rewrite_str(__u32 *str_off, void *ctx) > { > struct btf_pipe *p = ctx; > long mapped_off; > + const char *s; > int off, err; > > if (!*str_off) /* nothing to do for empty strings */ > @@ -1746,7 +1747,11 @@ static int btf_rewrite_str(__u32 *str_off, void *ctx) > return 0; > } > > - off = btf__add_str(p->dst, btf__str_by_offset(p->src, *str_off)); > + s = btf__str_by_offset(p->src, *str_off); > + if (!s) > + return -EINVAL; > + > + off = btf__add_str(p->dst, s); > if (off < 0) > return off; > > Agreed its a odd way to check input. Also I found a few cases where even if you did check it in btf__find_str it also deref'd a few lines down in the main code. I think the assumption throughout is that btf is !null. Or if it is null we abort much earlier in the code. Didn't study too thoroughly though so perhaps I missed some cases. One in the below diff for example would be nice I think. Could likely get a more meaningful error up to user as well by failing early. diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 63033c334320..5e70dcecab27 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -3470,6 +3470,9 @@ static int strs_dedup_remap_str_off(__u32 *str_off_ptr, void *ctx) return 0; s = btf__str_by_offset(d->btf, str_off); + if (s < 0) + return s; + if (d->btf->base_btf) { err = btf__find_str(d->btf->base_btf, s); if (err >= 0) { ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str 2023-12-14 7:50 [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str Wentao Zhang 2023-12-14 15:34 ` Daniel Borkmann @ 2023-12-14 23:36 ` Andrii Nakryiko 1 sibling, 0 replies; 4+ messages in thread From: Andrii Nakryiko @ 2023-12-14 23:36 UTC (permalink / raw) To: Wentao Zhang; +Cc: ast, daniel, bpf On Wed, Dec 13, 2023 at 11:51 PM Wentao Zhang <wentao.zhang@windriver.com> wrote: > > The function btf_str_by_offset may return NULL when used as an > input argument for btf_add_str in the context of btf_rewrite_str. > The added check ensures that both the input string (s) and the > BTF object (btf) are non-null before proceeding with the function > logic. If either is null, the function returns an error code > indicating an invalid argument. > > Found by our static analysis tool. > > Signed-off-by: Wentao Zhang <wentao.zhang@windriver.com> > --- > tools/lib/bpf/btf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index fd2309512978..a6a00bdc7151 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -1612,6 +1612,8 @@ int btf__find_str(struct btf *btf, const char *s) > int btf__add_str(struct btf *btf, const char *s) > { > int off; > + if(!s || !btf) > + return libbpf_err(-EINVAL); > while for some old libbpf code we sometimes did NULL validation, generally speaking for all new code we assume that passed objects are non-NULL, unless otherwise explicitly pointed out in API for cases where NULL is actually meaningful (like no options are specified, etc). In this case, both btf and s are expected to be non-NULL by the API and NULL values for either btf or s are meaningless. So I don't think we need to check these conditions. > if (btf->base_btf) { > off = btf__find_str(btf->base_btf, s); > -- > 2.35.5 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-14 23:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-14 7:50 [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str Wentao Zhang 2023-12-14 15:34 ` Daniel Borkmann 2023-12-14 19:34 ` John Fastabend 2023-12-14 23:36 ` Andrii Nakryiko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.