On Tue, Aug 27, 2024 at 01:37:21PM GMT, Andrii Nakryiko wrote:
> We do an ugly copying of options in bpf_object__open_skeleton() just to
> be able to set object name from skeleton's recorded name (while still
> allowing user to override it through opts->object_name).
>
> This is not just ugly, but it also is broken due to memcpy() that
> doesn't take into account potential skel_opts' and user-provided opts'
> sizes differences due to backward and forward compatibility. This leads
> to copying over extra bytes and then failing to validate options
> properly. It could, technically, lead also to SIGSEGV, if we are unlucky.
>
> So just get rid of that memory copy completely and instead pass
> default object name into bpf_object_open() directly, simplifying all
> this significantly. The rule now is that obj_name should be non-NULL for
> bpf_object_open() when called with in-memory buffer, so validate that
> explicitly as well.
>
> We adopt bpf_object__open_mem() to this as well and generate default
> name (based on buffer memory address and size) outside of bpf_object_open().
>
> Fixes: d66562fba1ce ("libbpf: Add BPF object skeleton support")
> Reported-by: Daniel Müller <deso@xxxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 52 +++++++++++++++---------------------------
> 1 file changed, 19 insertions(+), 33 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e55353887439..d3a542649e6b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -13761,29 +13763,13 @@ static int populate_skeleton_progs(const struct bpf_object *obj,
> int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> const struct bpf_object_open_opts *opts)
> {
> - DECLARE_LIBBPF_OPTS(bpf_object_open_opts, skel_opts,
> - .object_name = s->name,
> - );
> struct bpf_object *obj;
> int err;
>
> - /* Attempt to preserve opts->object_name, unless overriden by user
> - * explicitly. Overwriting object name for skeletons is discouraged,
> - * as it breaks global data maps, because they contain object name
> - * prefix as their own map name prefix. When skeleton is generated,
> - * bpftool is making an assumption that this name will stay the same.
> - */
> - if (opts) {
> - memcpy(&skel_opts, opts, sizeof(*opts));
> - if (!opts->object_name)
> - skel_opts.object_name = s->name;
> - }
> -
> - obj = bpf_object__open_mem(s->data, s->data_sz, &skel_opts);
> - err = libbpf_get_error(obj);
> - if (err) {
> - pr_warn("failed to initialize skeleton BPF object '%s': %d\n",
> - s->name, err);
> + obj = bpf_object_open(NULL, s->data, s->data_sz, s->name, opts);
> + if (IS_ERR(obj)) {
> + err = PTR_ERR(obj);
> + pr_warn("failed to initialize skeleton BPF object '%s': %d\n", s->name, err);
Ideally we'd do the same dance here for the name that we do in
bpf_object_open, right? Otherwise the warning may be mildly confusing if
> pr_debug("loading object '%s' from buffer\n", obj_name)
earlier refers to a potentially different name?
Seems minor, though. Thanks for the fix.
Reviewed-by: Daniel Müller <deso@xxxxxxxxxx>
