On Wed, Aug 28, 2024 at 10:29 AM Daniel Müller <deso@xxxxxxxxxx> wrote:
>
> 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?
Yeah, true, but I'm not really happy to add this "name resolution"
duplication of logic here, tbh. Also validation of options, etc. Let's
keep it as is, it's very unlikely someone will be overriding the
object name.
>
> Seems minor, though. Thanks for the fix.
>
> Reviewed-by: Daniel Müller <deso@xxxxxxxxxx>
