Re: [PATCH] libbpf: fix crash when input null program point in USDT API

[Xin Liu]

On Tue, 20 Dec 2022 15:53:18 -0800 Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> On Mon, Dec 19, 2022 at 5:57 PM Xin Liu <liuxin350@xxxxxxxxxx> wrote:
> >
> > On Tue, 20 Dec 2022 2:50:18 +0800 sdf<sdf@xxxxxxxxxx> wrote:
> > > On 12/19, Xin Liu wrote:
> > > > The API functions bpf_program__attach_perf_event_opts and
> > > > bpf_program_attach_usdt can be invoked by users. However, when the
> > > > input prog parameter is null, the API uses name and obj without
> > > > check. This will cause program to crash directly.
> > >
> > > Why do we care about these only? We have a lot of functions invoked
> > > by the users which don't check the arguments. Can the caller ensure
> > > the prog is valid/consistent before calling these?
> > >
> >
> > Thanks to sdf for this suggestions.
> >
> > But I don't think it's a good idea to let the user guarantee:
> > 1.We can't require all users to verify parameters before transferring
> >   parameters. Some parameters may be omitted. If the user forgets to check
> >   the program pointer and it happens to be NULL, the program will crash
> >   without any last words, and the user can only use the debugging tool to
> >   collect relevant clues, which is a disaster for the user.
> > 2.Code changes are required for completed user programs and places where
> >   the API is invoked. For users, the cost of ensuring that each parameter
> >   check result is correct is high, which is much higher than that of
> >   directly verifying the parameter in libbpf.
> >
> > So I think we should do some validation at the API entrance, whick is a
> > big benefit at the minimum cost, and in fact we do that, for example,
> > OPTS_VALID validation, right?
> >
> 
> I agree with Stanislav. There is no reason for user to assume that
> passing NULL works as a general rule. We do not check for NULL
> everywhere. If user doesn't follow API contract, yes, they will get
> crashes or confusing behavior, unfortunately.
> 
> For APIs that explicitly allow passing NULL for strings, documentation
> clearly states that. And if not, we should improve documentation.
> 
> > > > Signed-off-by: Xin Liu <liuxin350@xxxxxxxxxx>
> > > > ---
> > > >   tools/lib/bpf/libbpf.c | 13 ++++++++++++-
> > > >   1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 2a82f49ce16f..0d21de4f7d5c 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -9764,6 +9764,11 @@ struct bpf_link
> > > > *bpf_program__attach_perf_event_opts(const struct bpf_program *p
> > > >     if (!OPTS_VALID(opts, bpf_perf_event_opts))
> > > >             return libbpf_err_ptr(-EINVAL);
> > > >
> > > > +   if (!prog || !prog->name) {
> > > > +           pr_warn("prog: invalid prog\n");
> > > > +           return libbpf_err_ptr(-EINVAL);
> > > > +   }
> > > > +
> > > >     if (pfd < 0) {
> > > >             pr_warn("prog '%s': invalid perf event FD %d\n",
> > > >                     prog->name, pfd);
> > > > @@ -10967,7 +10972,7 @@ struct bpf_link *bpf_program__attach_usdt(const
> > > > struct bpf_program *prog,
> > > >                                       const struct bpf_usdt_opts *opts)
> > > >   {
> > > >     char resolved_path[512];
> > > > -   struct bpf_object *obj = prog->obj;
> > > > +   struct bpf_object *obj;
> > > >     struct bpf_link *link;
> > > >     __u64 usdt_cookie;
> > > >     int err;
> > > > @@ -10975,6 +10980,11 @@ struct bpf_link *bpf_program__attach_usdt(const
> > > > struct bpf_program *prog,
> > > >     if (!OPTS_VALID(opts, bpf_uprobe_opts))
> > > >             return libbpf_err_ptr(-EINVAL);
> > > >
> > > > +   if (!prog || !prog->name || !prog->obj) {
> > > > +           pr_warn("prog: invalid prog\n");
> > > > +           return libbpf_err_ptr(-EINVAL);
> > > > +   }
> > > > +
> > > >     if (bpf_program__fd(prog) < 0) {
> > > >             pr_warn("prog '%s': can't attach BPF program w/o FD (did you load
> > > > it?)\n",
> > > >                     prog->name);
> > > > @@ -10997,6 +11007,7 @@ struct bpf_link *bpf_program__attach_usdt(const
> > > > struct bpf_program *prog,
> > > >     /* USDT manager is instantiated lazily on first USDT attach. It will
> > > >      * be destroyed together with BPF object in bpf_object__close().
> > > >      */
> > > > +   obj = prog->obj;
> > > >     if (IS_ERR(obj->usdt_man))
> > > >             return libbpf_ptr(obj->usdt_man);
> > > >     if (!obj->usdt_man) {
> > > > --
> > > > 2.33.0

Thanks to Andrii and sdf.
I will resubmit a patch to update the API documentation.