| Message ID | 20240821164620.1056362-1-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: bpf_core_calc_relo_insn() should verify relocation type id | expand |
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 >
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
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 >
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. [...]
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. > > [...] >
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.
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.
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;
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(+)