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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221220015635.4394-1-liuxin350@huawei.com>
Date:   Tue, 20 Dec 2022 09:56:35 +0800
From:   Xin Liu <liuxin350@...wei.com>
To:     <sdf@...gle.com>
CC:     <andrii@...nel.org>, <ast@...nel.org>, <bpf@...r.kernel.org>,
        <daniel@...earbox.net>, <haoluo@...gle.com>,
        <john.fastabend@...il.com>, <jolsa@...nel.org>,
        <kongweibin2@...wei.com>, <kpsingh@...nel.org>,
        <linux-kernel@...r.kernel.org>, <liuxin350@...wei.com>,
        <martin.lau@...ux.dev>, <song@...nel.org>, <wuchangye@...wei.com>,
        <xiesongyang@...wei.com>, <yanan@...wei.com>, <yhs@...com>,
        <zhangmingyi5@...wei.com>
Subject: Re: [PATCH] libbpf: fix crash when input null program point in USDT API

On Tue, 20 Dec 2022 2:50:18 +0800 sdf<sdf@...gle.com> 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?

> > Signed-off-by: Xin Liu <liuxin350@...wei.com>
> > ---
> >   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