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

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

[Wentao Zhang]

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

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

[Daniel Borkmann]

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;

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

[John Fastabend]

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

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

[Andrii Nakryiko]

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