[PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Eduard Zingerman]

In case of malformed relocation record of kind BPF_CORE_TYPE_ID_LOCAL
referencing a non-existing BTF type, function bpf_core_calc_relo_insn
would cause a null pointer deference.

Fix this by adding a proper check, as malformed relocation records
could be passed from user space.

Simplest reproducer is a program:

    r0 = 0
    exit

With a single relocation record:

    .insn_off = 0,          /* patch first instruction */
    .type_id = 100500,      /* this type id does not exist */
    .access_str_off = 6,    /* offset of string "0" */
    .kind = BPF_CORE_TYPE_ID_LOCAL,

See the link for original reproducer.

Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
Reported-by: Liu RuiTong <cnitlrt@gmail.com>
Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/lib/bpf/relo_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
index 63a4d5ad12d1..a04724831ebc 100644
--- a/tools/lib/bpf/relo_core.c
+++ b/tools/lib/bpf/relo_core.c
@@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
 
 	local_id = relo->type_id;
 	local_type = btf_type_by_id(local_btf, local_id);
+	if (!local_type) {
+		pr_warn("prog '%s': relo #%d: bad type id %u\n",
+			prog_name, relo_idx, local_id);
+		return -EINVAL;
+	}
 	local_name = btf__name_by_offset(local_btf, local_type->name_off);
 	if (!local_name)
 		return -EINVAL;
-- 
2.45.2

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Andrii Nakryiko]

On Wed, Aug 21, 2024 at 9:46 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> In case of malformed relocation record of kind BPF_CORE_TYPE_ID_LOCAL
> referencing a non-existing BTF type, function bpf_core_calc_relo_insn
> would cause a null pointer deference.
>
> Fix this by adding a proper check, as malformed relocation records
> could be passed from user space.
>
> Simplest reproducer is a program:
>
>     r0 = 0
>     exit
>
> With a single relocation record:
>
>     .insn_off = 0,          /* patch first instruction */
>     .type_id = 100500,      /* this type id does not exist */
>     .access_str_off = 6,    /* offset of string "0" */
>     .kind = BPF_CORE_TYPE_ID_LOCAL,
>
> See the link for original reproducer.
>
> Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
> Reported-by: Liu RuiTong <cnitlrt@gmail.com>
> Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  tools/lib/bpf/relo_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> index 63a4d5ad12d1..a04724831ebc 100644
> --- a/tools/lib/bpf/relo_core.c
> +++ b/tools/lib/bpf/relo_core.c
> @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
>
>         local_id = relo->type_id;
>         local_type = btf_type_by_id(local_btf, local_id);
> +       if (!local_type) {

This is a meaningless check at least for libbpf's implementation of
btf_type_by_id(), it never returns NULL. Commit you point to in Fixes
tag clearly states the differences.

So you'd need to validate local_id directly against number of types in
local_btf.

pw-bot: cr


> +               pr_warn("prog '%s': relo #%d: bad type id %u\n",

nit: this part of CO-RE-related code normally uses [%u] "syntax" to
point to BTF type IDs, please adjust for consistency

> +                       prog_name, relo_idx, local_id);
> +               return -EINVAL;
> +       }
>         local_name = btf__name_by_offset(local_btf, local_type->name_off);
>         if (!local_name)
>                 return -EINVAL;
> --
> 2.45.2
>

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Eduard Zingerman]

On Wed, 2024-08-21 at 09:59 -0700, Andrii Nakryiko wrote:

[...]

> > Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
> > Reported-by: Liu RuiTong <cnitlrt@gmail.com>
> > Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  tools/lib/bpf/relo_core.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > index 63a4d5ad12d1..a04724831ebc 100644
> > --- a/tools/lib/bpf/relo_core.c
> > +++ b/tools/lib/bpf/relo_core.c
> > @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > 
> >         local_id = relo->type_id;
> >         local_type = btf_type_by_id(local_btf, local_id);
> > +       if (!local_type) {
> 
> This is a meaningless check at least for libbpf's implementation of
> btf_type_by_id(), it never returns NULL. Commit you point to in Fixes
> tag clearly states the differences.

That is not true on kernel side.
bpf_core_calc_relo_insn() is called from bpf_core_apply():

int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
                   int relo_idx, void *insn)
{
        bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
        ...
        if (need_cands) {
                ...
                // code below would report an error if relo->type_id is bogus
                cc = bpf_core_find_cands(ctx, relo->type_id);
                if (IS_ERR(cc)) {
                        bpf_log(ctx->log, "target candidate search failed for %d\n",
                                relo->type_id);
                        err = PTR_ERR(cc);
                        goto out;
                }
                ...
        }

        err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs,
                                      &targ_res);
        ...
}

If `need_cands` is false the bogus type_id could reach into bpf_core_calc_relo_insn().
Which is exactly the case with repro submitted by Liu.
There is also a simplified repro here:
https://github.com/kernel-patches/bpf/commit/017a9dcf17e572f9b7c32aa62a81df8ef41cef17
But I can't submit it as a test yet.

> 
> So you'd need to validate local_id directly against number of types in
> local_btf.

How is this better than a null check?

> 
> pw-bot: cr
> 
> 
> > +               pr_warn("prog '%s': relo #%d: bad type id %u\n",
> 
> nit: this part of CO-RE-related code normally uses [%u] "syntax" to
> point to BTF type IDs, please adjust for consistency

Ok

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Andrii Nakryiko]

On Wed, Aug 21, 2024 at 10:46 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-08-21 at 09:59 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
> > > Reported-by: Liu RuiTong <cnitlrt@gmail.com>
> > > Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
> > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > ---
> > >  tools/lib/bpf/relo_core.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > > index 63a4d5ad12d1..a04724831ebc 100644
> > > --- a/tools/lib/bpf/relo_core.c
> > > +++ b/tools/lib/bpf/relo_core.c
> > > @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > >
> > >         local_id = relo->type_id;
> > >         local_type = btf_type_by_id(local_btf, local_id);
> > > +       if (!local_type) {
> >
> > This is a meaningless check at least for libbpf's implementation of
> > btf_type_by_id(), it never returns NULL. Commit you point to in Fixes
> > tag clearly states the differences.
>
> That is not true on kernel side.
> bpf_core_calc_relo_insn() is called from bpf_core_apply():
>
> int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
>                    int relo_idx, void *insn)
> {
>         bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
>         ...
>         if (need_cands) {
>                 ...
>                 // code below would report an error if relo->type_id is bogus
>                 cc = bpf_core_find_cands(ctx, relo->type_id);
>                 if (IS_ERR(cc)) {
>                         bpf_log(ctx->log, "target candidate search failed for %d\n",
>                                 relo->type_id);
>                         err = PTR_ERR(cc);
>                         goto out;
>                 }
>                 ...
>         }
>
>         err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs,
>                                       &targ_res);
>         ...
> }
>
> If `need_cands` is false the bogus type_id could reach into bpf_core_calc_relo_insn().
> Which is exactly the case with repro submitted by Liu.
> There is also a simplified repro here:
> https://github.com/kernel-patches/bpf/commit/017a9dcf17e572f9b7c32aa62a81df8ef41cef17
> But I can't submit it as a test yet.
>
> >
> > So you'd need to validate local_id directly against number of types in
> > local_btf.
>
> How is this better than a null check?
>

because id check will be useful for both kernel and libbpf sides?..

> >
> > pw-bot: cr
> >
> >
> > > +               pr_warn("prog '%s': relo #%d: bad type id %u\n",
> >
> > nit: this part of CO-RE-related code normally uses [%u] "syntax" to
> > point to BTF type IDs, please adjust for consistency
>
> Ok
>

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Eduard Zingerman]

On Wed, 2024-08-21 at 12:10 -0700, Andrii Nakryiko wrote:
> On Wed, Aug 21, 2024 at 10:46 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2024-08-21 at 09:59 -0700, Andrii Nakryiko wrote:
> > 
> > [...]
> > 
> > > > Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
> > > > Reported-by: Liu RuiTong <cnitlrt@gmail.com>
> > > > Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
> > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > > ---
> > > >  tools/lib/bpf/relo_core.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > > > index 63a4d5ad12d1..a04724831ebc 100644
> > > > --- a/tools/lib/bpf/relo_core.c
> > > > +++ b/tools/lib/bpf/relo_core.c
> > > > @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > > > 
> > > >         local_id = relo->type_id;
> > > >         local_type = btf_type_by_id(local_btf, local_id);
> > > > +       if (!local_type) {
> > > 
> > > This is a meaningless check at least for libbpf's implementation of
> > > btf_type_by_id(), it never returns NULL. Commit you point to in Fixes
> > > tag clearly states the differences.
> > 
> > That is not true on kernel side.
> > bpf_core_calc_relo_insn() is called from bpf_core_apply():
> > 
> > int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
> >                    int relo_idx, void *insn)
> > {
> >         bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
> >         ...
> >         if (need_cands) {
> >                 ...
> >                 // code below would report an error if relo->type_id is bogus
> >                 cc = bpf_core_find_cands(ctx, relo->type_id);
> >                 if (IS_ERR(cc)) {
> >                         bpf_log(ctx->log, "target candidate search failed for %d\n",
> >                                 relo->type_id);
> >                         err = PTR_ERR(cc);
> >                         goto out;
> >                 }
> >                 ...
> >         }
> > 
> >         err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs,
> >                                       &targ_res);
> >         ...
> > }
> > 
> > If `need_cands` is false the bogus type_id could reach into bpf_core_calc_relo_insn().
> > Which is exactly the case with repro submitted by Liu.
> > There is also a simplified repro here:
> > https://github.com/kernel-patches/bpf/commit/017a9dcf17e572f9b7c32aa62a81df8ef41cef17
> > But I can't submit it as a test yet.
> > 
> > > 
> > > So you'd need to validate local_id directly against number of types in
> > > local_btf.
> > 
> > How is this better than a null check?
> > 
> 
> because id check will be useful for both kernel and libbpf sides?..

This would require a special case for BPF_CORE_TYPE_ID_LOCAL in
bpf_core_calc_relo_insn(). If you don't like this null check I'll
modify bpf_core_apply() instead to always check relo->type_id and
report and error.

This would also be in the line with what bpf_core_resolve_relo()
does on libbpf side.

[...]

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Andrii Nakryiko]

On Wed, Aug 21, 2024 at 12:29 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2024-08-21 at 12:10 -0700, Andrii Nakryiko wrote:
> > On Wed, Aug 21, 2024 at 10:46 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Wed, 2024-08-21 at 09:59 -0700, Andrii Nakryiko wrote:
> > >
> > > [...]
> > >
> > > > > Fixes: 74753e1462e7 ("libbpf: Replace btf__type_by_id() with btf_type_by_id().")
> > > > > Reported-by: Liu RuiTong <cnitlrt@gmail.com>
> > > > > Closes: https://lore.kernel.org/bpf/CAK55_s6do7C+DVwbwY_7nKfUz0YLDoiA1v6X3Y9+p0sWzipFSA@mail.gmail.com/
> > > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > > > ---
> > > > >  tools/lib/bpf/relo_core.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> > > > > index 63a4d5ad12d1..a04724831ebc 100644
> > > > > --- a/tools/lib/bpf/relo_core.c
> > > > > +++ b/tools/lib/bpf/relo_core.c
> > > > > @@ -1297,6 +1297,11 @@ int bpf_core_calc_relo_insn(const char *prog_name,
> > > > >
> > > > >         local_id = relo->type_id;
> > > > >         local_type = btf_type_by_id(local_btf, local_id);
> > > > > +       if (!local_type) {
> > > >
> > > > This is a meaningless check at least for libbpf's implementation of
> > > > btf_type_by_id(), it never returns NULL. Commit you point to in Fixes
> > > > tag clearly states the differences.
> > >
> > > That is not true on kernel side.
> > > bpf_core_calc_relo_insn() is called from bpf_core_apply():
> > >
> > > int bpf_core_apply(struct bpf_core_ctx *ctx, const struct bpf_core_relo *relo,
> > >                    int relo_idx, void *insn)
> > > {
> > >         bool need_cands = relo->kind != BPF_CORE_TYPE_ID_LOCAL;
> > >         ...
> > >         if (need_cands) {
> > >                 ...
> > >                 // code below would report an error if relo->type_id is bogus
> > >                 cc = bpf_core_find_cands(ctx, relo->type_id);
> > >                 if (IS_ERR(cc)) {
> > >                         bpf_log(ctx->log, "target candidate search failed for %d\n",
> > >                                 relo->type_id);
> > >                         err = PTR_ERR(cc);
> > >                         goto out;
> > >                 }
> > >                 ...
> > >         }
> > >
> > >         err = bpf_core_calc_relo_insn((void *)ctx->log, relo, relo_idx, ctx->btf, &cands, specs,
> > >                                       &targ_res);
> > >         ...
> > > }
> > >
> > > If `need_cands` is false the bogus type_id could reach into bpf_core_calc_relo_insn().
> > > Which is exactly the case with repro submitted by Liu.
> > > There is also a simplified repro here:
> > > https://github.com/kernel-patches/bpf/commit/017a9dcf17e572f9b7c32aa62a81df8ef41cef17
> > > But I can't submit it as a test yet.
> > >
> > > >
> > > > So you'd need to validate local_id directly against number of types in
> > > > local_btf.
> > >
> > > How is this better than a null check?
> > >
> >
> > because id check will be useful for both kernel and libbpf sides?..
>
> This would require a special case for BPF_CORE_TYPE_ID_LOCAL in
> bpf_core_calc_relo_insn(). If you don't like this null check I'll
> modify bpf_core_apply() instead to always check relo->type_id and
> report and error.
>
> This would also be in the line with what bpf_core_resolve_relo()
> does on libbpf side.

Ok, then let's do that. I don't want static analysers complaining
about this when checking libbpf code base.

>
> [...]
>

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Eduard Zingerman]

On Wed, 2024-08-21 at 13:08 -0700, Andrii Nakryiko wrote:

[...]

> Ok, then let's do that. I don't want static analysers complaining
> about this when checking libbpf code base.

Just want to rant a bit.
Here is a footgun in the relo_core.c:

    #ifdef __KERNEL__
    ...
    #undef pr_warn
    #undef pr_info
    #undef pr_debug
    #define pr_warn(fmt, log, ...)      bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
    #define pr_info(fmt, log, ...)      bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
    #define pr_debug(fmt, log, ...)     bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
                          ^^^                   ^^^^^^^^^^^       ^^
                     first format param,        prog_name         replacement for
                     usually prog_name          cast to           first param
                                                verifier log
    ...
    #else
    ...
    #endif

    int bpf_core_calc_relo_insn(const char *prog_name, ...)
    {
        ...
        pr_warn("prog '%s': relo #%d: bad type id %u\n",
                prog_name, relo_idx, local_id);
        ...
    }

And in the verifier.c:

    err = bpf_core_calc_relo_insn((void *)ctx->log, relo, ...);
                                  ^^^^^^^^^^^^^^^^
                                  This is a prog_name parameter

Just spent more than an hour trying to figure out why passing real
program name (char *) does not work.
I'll think on a refactoring, but that is for another series.

Re: [PATCH bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id

[Eduard Zingerman]

On Wed, 2024-08-21 at 15:32 -0700, Eduard Zingerman wrote:

[...]

> Just spent more than an hour trying to figure out why passing real
> program name (char *) does not work.
> I'll think on a refactoring, but that is for another series.

I passed env->prog->aux->name there, and pr_warn caused corruption
of the aux structure. Sigh.