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@xxxxxxxxxxxxx>
> > ---
> > 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) {
