* [PATCH] libbpf: fix crash when input null program point in USDT API
@ 2022-12-19 6:46 Xin Liu
2022-12-19 18:50 ` sdf
0 siblings, 1 reply; 5+ messages in thread
From: Xin Liu @ 2022-12-19 6:46 UTC (permalink / raw)
To: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa
Cc: bpf, linux-kernel, yanan, wuchangye, xiesongyang, kongweibin2,
liuxin350, zhangmingyi5
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.
Signed-off-by: Xin Liu <liuxin350@huawei.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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] libbpf: fix crash when input null program point in USDT API
2022-12-19 6:46 [PATCH] libbpf: fix crash when input null program point in USDT API Xin Liu
@ 2022-12-19 18:50 ` sdf
2022-12-20 1:56 ` Xin Liu
0 siblings, 1 reply; 5+ messages in thread
From: sdf @ 2022-12-19 18:50 UTC (permalink / raw)
To: Xin Liu
Cc: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, bpf, linux-kernel, yanan, wuchangye,
xiesongyang, kongweibin2, zhangmingyi5
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?
> Signed-off-by: Xin Liu <liuxin350@huawei.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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libbpf: fix crash when input null program point in USDT API
2022-12-19 18:50 ` sdf
@ 2022-12-20 1:56 ` Xin Liu
2022-12-20 23:53 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Xin Liu @ 2022-12-20 1:56 UTC (permalink / raw)
To: sdf
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa,
kongweibin2, kpsingh, linux-kernel, liuxin350, martin.lau, song,
wuchangye, xiesongyang, yanan, yhs, zhangmingyi5
On Tue, 20 Dec 2022 2:50:18 +0800 sdf<sdf@google.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@huawei.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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libbpf: fix crash when input null program point in USDT API
2022-12-20 1:56 ` Xin Liu
@ 2022-12-20 23:53 ` Andrii Nakryiko
2022-12-23 1:09 ` Xin Liu
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2022-12-20 23:53 UTC (permalink / raw)
To: Xin Liu
Cc: sdf, andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa,
kongweibin2, kpsingh, linux-kernel, martin.lau, song, wuchangye,
xiesongyang, yanan, yhs, zhangmingyi5
On Mon, Dec 19, 2022 at 5:57 PM Xin Liu <liuxin350@huawei.com> wrote:
>
> On Tue, 20 Dec 2022 2:50:18 +0800 sdf<sdf@google.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?
>
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@huawei.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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] libbpf: fix crash when input null program point in USDT API
2022-12-20 23:53 ` Andrii Nakryiko
@ 2022-12-23 1:09 ` Xin Liu
0 siblings, 0 replies; 5+ messages in thread
From: Xin Liu @ 2022-12-23 1:09 UTC (permalink / raw)
To: andrii.nakryiko
Cc: andrii, ast, bpf, daniel, haoluo, john.fastabend, jolsa,
kongweibin2, kpsingh, linux-kernel, liuxin350, martin.lau, sdf,
song, wuchangye, xiesongyang, yanan, yhs, zhangmingyi5
On Tue, 20 Dec 2022 15:53:18 -0800 Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Mon, Dec 19, 2022 at 5:57 PM Xin Liu <liuxin350@huawei.com> wrote:
> >
> > On Tue, 20 Dec 2022 2:50:18 +0800 sdf<sdf@google.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?
> >
>
> 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@huawei.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
Thanks to Andrii and sdf.
I will resubmit a patch to update the API documentation.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-23 1:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-19 6:46 [PATCH] libbpf: fix crash when input null program point in USDT API Xin Liu
2022-12-19 18:50 ` sdf
2022-12-20 1:56 ` Xin Liu
2022-12-20 23:53 ` Andrii Nakryiko
2022-12-23 1:09 ` Xin Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.