Re: [PATCH bpf-next] libbpf: Fix null pointer check in btf__add_str

[John Fastabend <john.fastabend@gmail.com>]

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