* [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
@ 2023-06-05 3:34 Xin Liu
2023-06-05 17:26 ` Stanislav Fomichev
0 siblings, 1 reply; 8+ messages in thread
From: Xin Liu @ 2023-06-05 3:34 UTC (permalink / raw)
To: daniel
Cc: andrii, ast, bpf, davem, edumazet, hsinweih, jakub,
john.fastabend, kuba, linux-kernel, liuxin350, netdev, pabeni,
syzbot+49f6cef45247ff249498, syzkaller-bugs, yanan, wuchangye,
xiesongyang, kongweibin2, zhangmingyi5
From: zhangmingyi <zhangmingyi5@huawei.com>
The sample_cb of the ring_buffer__new interface can transfer NULL. However,
the system does not check whether sample_cb is NULL during
ring_buffer__poll, null pointer is used.
Signed-off-by: zhangmingyi <zhangmingyi5@huawei.com>
---
tools/lib/bpf/ringbuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
index 02199364db13..3661338a1d2e 100644
--- a/tools/lib/bpf/ringbuf.c
+++ b/tools/lib/bpf/ringbuf.c
@@ -248,7 +248,7 @@ static int64_t ringbuf_process_ring(struct ring *r)
got_new_data = true;
cons_pos += roundup_len(len);
- if ((len & BPF_RINGBUF_DISCARD_BIT) == 0) {
+ if (r->sample_cb && ((len & BPF_RINGBUF_DISCARD_BIT) == 0)) {
sample = (void *)len_ptr + BPF_RINGBUF_HDR_SZ;
err = r->sample_cb(r->ctx, sample, len);
if (err < 0) {
--
2.33.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-05 3:34 [PATCH] libbpf:fix use empty function pointers in ringbuf_poll Xin Liu
@ 2023-06-05 17:26 ` Stanislav Fomichev
2023-06-08 12:58 ` zhangmingyi
0 siblings, 1 reply; 8+ messages in thread
From: Stanislav Fomichev @ 2023-06-05 17:26 UTC (permalink / raw)
To: Xin Liu
Cc: daniel, andrii, ast, bpf, davem, edumazet, hsinweih, jakub,
john.fastabend, kuba, linux-kernel, netdev, pabeni,
syzbot+49f6cef45247ff249498, syzkaller-bugs, yanan, wuchangye,
xiesongyang, kongweibin2, zhangmingyi5
On 06/05, Xin Liu wrote:
> From: zhangmingyi <zhangmingyi5@huawei.com>
>
> The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> the system does not check whether sample_cb is NULL during
> ring_buffer__poll, null pointer is used.
What is the point of calling ring_buffer__new with sample_cb == NULL?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-05 17:26 ` Stanislav Fomichev
@ 2023-06-08 12:58 ` zhangmingyi
2023-06-08 16:27 ` Stanislav Fomichev
0 siblings, 1 reply; 8+ messages in thread
From: zhangmingyi @ 2023-06-08 12:58 UTC (permalink / raw)
To: sdf
Cc: andrii, ast, bpf, daniel, davem, edumazet, hsinweih, jakub,
john.fastabend, kongweibin2, kuba, linux-kernel, liuxin350,
netdev, pabeni, syzbot+49f6cef45247ff249498, syzkaller-bugs,
wuchangye, xiesongyang, yanan, zhangmingyi5
On 06/06,Stanislav Fomichev wrote:
> On 06/05, Xin Liu wrote:
> > From: zhangmingyi <zhangmingyi5@huawei.com>
>
> > The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> > the system does not check whether sample_cb is NULL during
> > ring_buffer__poll, null pointer is used.
> What is the point of calling ring_buffer__new with sample_cb == NULL?
Yes, as you said, passing sample_cb in ring_buffer__new to NULL doesn't
make sense, and few people use it that way, but that doesn't prevent this
from being a allowed and supported scenario. And when ring_buffer__poll is
called, it leads to a segmentation fault (core dump), which I think needs
to be fixed to ensure the security quality of libbpf.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-08 12:58 ` zhangmingyi
@ 2023-06-08 16:27 ` Stanislav Fomichev
2023-06-08 17:38 ` Andrii Nakryiko
0 siblings, 1 reply; 8+ messages in thread
From: Stanislav Fomichev @ 2023-06-08 16:27 UTC (permalink / raw)
To: zhangmingyi
Cc: andrii, ast, bpf, daniel, davem, edumazet, hsinweih, jakub,
john.fastabend, kongweibin2, kuba, linux-kernel, liuxin350,
netdev, pabeni, syzbot+49f6cef45247ff249498, syzkaller-bugs,
wuchangye, xiesongyang, yanan
On Thu, Jun 8, 2023 at 6:00 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
>
> On 06/06,Stanislav Fomichev wrote:
>
> > On 06/05, Xin Liu wrote:
> > > From: zhangmingyi <zhangmingyi5@huawei.com>
> >
> > > The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> > > the system does not check whether sample_cb is NULL during
> > > ring_buffer__poll, null pointer is used.
>
> > What is the point of calling ring_buffer__new with sample_cb == NULL?
>
> Yes, as you said, passing sample_cb in ring_buffer__new to NULL doesn't
> make sense, and few people use it that way, but that doesn't prevent this
> from being a allowed and supported scenario. And when ring_buffer__poll is
> called, it leads to a segmentation fault (core dump), which I think needs
> to be fixed to ensure the security quality of libbpf.
I dunno. I'd argue that passing a NULL to ring_buffer__new is an API
misuse. Maybe ring_buffer__new should return -EINVAL instead when
passed NULL sample_cb? Although, we don't usually have those checks
for the majority of the arguments in libbpf...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-08 16:27 ` Stanislav Fomichev
@ 2023-06-08 17:38 ` Andrii Nakryiko
2023-06-09 9:36 ` zhangmingyi
0 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2023-06-08 17:38 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: zhangmingyi, andrii, ast, bpf, daniel, davem, edumazet, hsinweih,
jakub, john.fastabend, kongweibin2, kuba, linux-kernel, liuxin350,
netdev, pabeni, syzbot+49f6cef45247ff249498, syzkaller-bugs,
wuchangye, xiesongyang, yanan
On Thu, Jun 8, 2023 at 9:27 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Thu, Jun 8, 2023 at 6:00 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
> >
> > On 06/06,Stanislav Fomichev wrote:
> >
> > > On 06/05, Xin Liu wrote:
> > > > From: zhangmingyi <zhangmingyi5@huawei.com>
> > >
> > > > The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> > > > the system does not check whether sample_cb is NULL during
> > > > ring_buffer__poll, null pointer is used.
> >
> > > What is the point of calling ring_buffer__new with sample_cb == NULL?
> >
> > Yes, as you said, passing sample_cb in ring_buffer__new to NULL doesn't
> > make sense, and few people use it that way, but that doesn't prevent this
> > from being a allowed and supported scenario. And when ring_buffer__poll is
> > called, it leads to a segmentation fault (core dump), which I think needs
> > to be fixed to ensure the security quality of libbpf.
>
> I dunno. I'd argue that passing a NULL to ring_buffer__new is an API
> misuse. Maybe ring_buffer__new should return -EINVAL instead when
> passed NULL sample_cb? Although, we don't usually have those checks
> for the majority of the arguments in libbpf...
Right. I'd say we should add a proper doc comment specifying all
arguments and which ones are optional or not. And make it explicit
that callback is not optional. If we start checking every possible
pointer for NULL, libbpf will be littered with NULL checks, I'm not
sure that's good.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-08 17:38 ` Andrii Nakryiko
@ 2023-06-09 9:36 ` zhangmingyi
2023-06-09 16:55 ` Stanislav Fomichev
0 siblings, 1 reply; 8+ messages in thread
From: zhangmingyi @ 2023-06-09 9:36 UTC (permalink / raw)
To: andrii.nakryiko
Cc: andrii, ast, bpf, daniel, davem, edumazet, hsinweih, jakub,
john.fastabend, kongweibin2, kuba, linux-kernel, liuxin350,
netdev, pabeni, sdf, syzbot+49f6cef45247ff249498, syzkaller-bugs,
wuchangye, xiesongyang, yanan, zhangmingyi5
On Fri, Jun 9, 2023 at 1:39 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Thu, Jun 8, 2023 at 9:27 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > On Thu, Jun 8, 2023 at 6:00 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
> > >
> > > On 06/06,Stanislav Fomichev wrote:
> > >
> > > > On 06/05, Xin Liu wrote:
> > > > > From: zhangmingyi <zhangmingyi5@huawei.com>
> > > >
> > > > > The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> > > > > the system does not check whether sample_cb is NULL during
> > > > > ring_buffer__poll, null pointer is used.
> > >
> > > > What is the point of calling ring_buffer__new with sample_cb == NULL?
> > >
> > > Yes, as you said, passing sample_cb in ring_buffer__new to NULL doesn't
> > > make sense, and few people use it that way, but that doesn't prevent this
> > > from being a allowed and supported scenario. And when ring_buffer__poll is
> > > called, it leads to a segmentation fault (core dump), which I think needs
> > > to be fixed to ensure the security quality of libbpf.
> >
> > I dunno. I'd argue that passing a NULL to ring_buffer__new is an API
> > misuse. Maybe ring_buffer__new should return -EINVAL instead when
> > passed NULL sample_cb? Although, we don't usually have those checks
> > for the majority of the arguments in libbpf...
>
> Right. I'd say we should add a proper doc comment specifying all
> arguments and which ones are optional or not. And make it explicit
> that callback is not optional. If we start checking every possible
> pointer for NULL, libbpf will be littered with NULL checks, I'm not
> sure that's good.
I agree, we should add a proper doc comment specifying all
arguments and which ones are optional or not.
However, why does the external interface API in libbpf not verify input
parameters or add verification where risky operations may exist?
What's more, i think sample_cb=NULL is not strictly a mistake or
prohibited use, and is meaningless.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-09 9:36 ` zhangmingyi
@ 2023-06-09 16:55 ` Stanislav Fomichev
2023-06-09 18:23 ` Andrii Nakryiko
0 siblings, 1 reply; 8+ messages in thread
From: Stanislav Fomichev @ 2023-06-09 16:55 UTC (permalink / raw)
To: zhangmingyi
Cc: andrii.nakryiko, andrii, ast, bpf, daniel, davem, edumazet,
hsinweih, jakub, john.fastabend, kongweibin2, kuba, linux-kernel,
liuxin350, netdev, pabeni, syzbot+49f6cef45247ff249498,
syzkaller-bugs, wuchangye, xiesongyang, yanan
On Fri, Jun 9, 2023 at 2:38 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
>
> On Fri, Jun 9, 2023 at 1:39 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Thu, Jun 8, 2023 at 9:27 AM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > On Thu, Jun 8, 2023 at 6:00 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
> > > >
> > > > On 06/06,Stanislav Fomichev wrote:
> > > >
> > > > > On 06/05, Xin Liu wrote:
> > > > > > From: zhangmingyi <zhangmingyi5@huawei.com>
> > > > >
> > > > > > The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> > > > > > the system does not check whether sample_cb is NULL during
> > > > > > ring_buffer__poll, null pointer is used.
> > > >
> > > > > What is the point of calling ring_buffer__new with sample_cb == NULL?
> > > >
> > > > Yes, as you said, passing sample_cb in ring_buffer__new to NULL doesn't
> > > > make sense, and few people use it that way, but that doesn't prevent this
> > > > from being a allowed and supported scenario. And when ring_buffer__poll is
> > > > called, it leads to a segmentation fault (core dump), which I think needs
> > > > to be fixed to ensure the security quality of libbpf.
> > >
> > > I dunno. I'd argue that passing a NULL to ring_buffer__new is an API
> > > misuse. Maybe ring_buffer__new should return -EINVAL instead when
> > > passed NULL sample_cb? Although, we don't usually have those checks
> > > for the majority of the arguments in libbpf...
> >
> > Right. I'd say we should add a proper doc comment specifying all
> > arguments and which ones are optional or not. And make it explicit
> > that callback is not optional. If we start checking every possible
> > pointer for NULL, libbpf will be littered with NULL checks, I'm not
> > sure that's good.
>
> I agree, we should add a proper doc comment specifying all
> arguments and which ones are optional or not.
> However, why does the external interface API in libbpf not verify input
> parameters or add verification where risky operations may exist?
> What's more, i think sample_cb=NULL is not strictly a mistake or
> prohibited use, and is meaningless.
It's not really customary in C to do it? So maybe you can follow up
with the update to the doc?
The kindergarten is over, you pass NULL you get SIGSEGV :-D
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
2023-06-09 16:55 ` Stanislav Fomichev
@ 2023-06-09 18:23 ` Andrii Nakryiko
0 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2023-06-09 18:23 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: zhangmingyi, andrii, ast, bpf, daniel, davem, edumazet, hsinweih,
jakub, john.fastabend, kongweibin2, kuba, linux-kernel, liuxin350,
netdev, pabeni, syzbot+49f6cef45247ff249498, syzkaller-bugs,
wuchangye, xiesongyang, yanan
On Fri, Jun 9, 2023 at 9:55 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Fri, Jun 9, 2023 at 2:38 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
> >
> > On Fri, Jun 9, 2023 at 1:39 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > On Thu, Jun 8, 2023 at 9:27 AM Stanislav Fomichev <sdf@google.com> wrote:
> > > >
> > > > On Thu, Jun 8, 2023 at 6:00 AM zhangmingyi <zhangmingyi5@huawei.com> wrote:
> > > > >
> > > > > On 06/06,Stanislav Fomichev wrote:
> > > > >
> > > > > > On 06/05, Xin Liu wrote:
> > > > > > > From: zhangmingyi <zhangmingyi5@huawei.com>
> > > > > >
> > > > > > > The sample_cb of the ring_buffer__new interface can transfer NULL. However,
> > > > > > > the system does not check whether sample_cb is NULL during
> > > > > > > ring_buffer__poll, null pointer is used.
> > > > >
> > > > > > What is the point of calling ring_buffer__new with sample_cb == NULL?
> > > > >
> > > > > Yes, as you said, passing sample_cb in ring_buffer__new to NULL doesn't
> > > > > make sense, and few people use it that way, but that doesn't prevent this
> > > > > from being a allowed and supported scenario. And when ring_buffer__poll is
> > > > > called, it leads to a segmentation fault (core dump), which I think needs
> > > > > to be fixed to ensure the security quality of libbpf.
> > > >
> > > > I dunno. I'd argue that passing a NULL to ring_buffer__new is an API
> > > > misuse. Maybe ring_buffer__new should return -EINVAL instead when
> > > > passed NULL sample_cb? Although, we don't usually have those checks
> > > > for the majority of the arguments in libbpf...
> > >
> > > Right. I'd say we should add a proper doc comment specifying all
> > > arguments and which ones are optional or not. And make it explicit
> > > that callback is not optional. If we start checking every possible
> > > pointer for NULL, libbpf will be littered with NULL checks, I'm not
> > > sure that's good.
> >
> > I agree, we should add a proper doc comment specifying all
> > arguments and which ones are optional or not.
> > However, why does the external interface API in libbpf not verify input
> > parameters or add verification where risky operations may exist?
> > What's more, i think sample_cb=NULL is not strictly a mistake or
> > prohibited use, and is meaningless.
>
> It's not really customary in C to do it? So maybe you can follow up
> with the update to the doc?
Yep, we do not check every `struct bpf_object *` pointer to be non-NULL.
Having said that, I don't think it's such a big deal to make this
callback optional by assigning a no-op callback.
So let's definitely update doc comments to be explicit about one way
or the other. For the callback, let's just not do it on every record.
Just once during initialization would be better.
>
> The kindergarten is over, you pass NULL you get SIGSEGV :-D
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-09 18:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-05 3:34 [PATCH] libbpf:fix use empty function pointers in ringbuf_poll Xin Liu
2023-06-05 17:26 ` Stanislav Fomichev
2023-06-08 12:58 ` zhangmingyi
2023-06-08 16:27 ` Stanislav Fomichev
2023-06-08 17:38 ` Andrii Nakryiko
2023-06-09 9:36 ` zhangmingyi
2023-06-09 16:55 ` Stanislav Fomichev
2023-06-09 18:23 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).