| Message ID | 20220422164027.GA7862@pi3.com.pl (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set | expand |
On 4/22/22 6:40 PM, Adam Zabrocki wrote: > [PATCH bpf] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set > > The recent kernel change "kprobes: Use rethook for kretprobe if possible", > introduced a potential NULL pointer dereference bug in the KRETPROBE > mechanism. The official Kprobes documentation defines that "Any or all > handlers can be NULL". Unfortunately, there is a missing return handler > verification to fulfill these requirements and can result in a NULL pointer > dereference bug. > > This patch adds such verification in kretprobe_rethook_handler() function. > > Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible") > Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl> > Acked-by: Masami Hiramatsu <mhiramat@kernel.org> I don't mind if this fix gets routed via bpf tree if all parties are okay with it (Masami? Steven?). Just noting that there is currently no specific dependency in bpf tree for it, but if it's easier to route this way, happy to take it. > --- > kernel/kprobes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index dbe57df2e199..dd58c0be9ce2 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -2126,7 +2126,7 @@ static void kretprobe_rethook_handler(struct rethook_node *rh, void *data, > struct kprobe_ctlblk *kcb; > > /* The data must NOT be null. This means rethook data structure is broken. */ > - if (WARN_ON_ONCE(!data)) > + if (WARN_ON_ONCE(!data) || !rp->handler) > return; > > __this_cpu_write(current_kprobe, &rp->kp); > Thanks, Daniel
On Mon, 25 Apr 2022 16:42:12 +0200 Daniel Borkmann <daniel@iogearbox.net> wrote: > On 4/22/22 6:40 PM, Adam Zabrocki wrote: > > [PATCH bpf] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set > > > > The recent kernel change "kprobes: Use rethook for kretprobe if possible", > > introduced a potential NULL pointer dereference bug in the KRETPROBE > > mechanism. The official Kprobes documentation defines that "Any or all > > handlers can be NULL". Unfortunately, there is a missing return handler > > verification to fulfill these requirements and can result in a NULL pointer > > dereference bug. > > > > This patch adds such verification in kretprobe_rethook_handler() function. > > > > Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible") > > Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl> > > Acked-by: Masami Hiramatsu <mhiramat@kernel.org> > > I don't mind if this fix gets routed via bpf tree if all parties are okay with > it (Masami? Steven?). Just noting that there is currently no specific dependency > in bpf tree for it, but if it's easier to route this way, happy to take it. Yeah, I and Steve talked about it and he suggested this to be merged via BPF tree since the original commit came from the BPF tree. Thank you, > > > --- > > kernel/kprobes.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > index dbe57df2e199..dd58c0be9ce2 100644 > > --- a/kernel/kprobes.c > > +++ b/kernel/kprobes.c > > @@ -2126,7 +2126,7 @@ static void kretprobe_rethook_handler(struct rethook_node *rh, void *data, > > struct kprobe_ctlblk *kcb; > > > > /* The data must NOT be null. This means rethook data structure is broken. */ > > - if (WARN_ON_ONCE(!data)) > > + if (WARN_ON_ONCE(!data) || !rp->handler) > > return; > > > > __this_cpu_write(current_kprobe, &rp->kp); > > > > Thanks, > Daniel
On 4/26/22 10:50 AM, Masami Hiramatsu wrote: > On Mon, 25 Apr 2022 16:42:12 +0200 > Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 4/22/22 6:40 PM, Adam Zabrocki wrote: >>> [PATCH bpf] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set >>> >>> The recent kernel change "kprobes: Use rethook for kretprobe if possible", >>> introduced a potential NULL pointer dereference bug in the KRETPROBE >>> mechanism. The official Kprobes documentation defines that "Any or all >>> handlers can be NULL". Unfortunately, there is a missing return handler >>> verification to fulfill these requirements and can result in a NULL pointer >>> dereference bug. >>> >>> This patch adds such verification in kretprobe_rethook_handler() function. >>> >>> Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible") >>> Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl> >>> Acked-by: Masami Hiramatsu <mhiramat@kernel.org> >> >> I don't mind if this fix gets routed via bpf tree if all parties are okay with >> it (Masami? Steven?). Just noting that there is currently no specific dependency >> in bpf tree for it, but if it's easier to route this way, happy to take it. > > Yeah, I and Steve talked about it and he suggested this to be merged > via BPF tree since the original commit came from the BPF tree. Okay, I just applied it to bpf tree then. Thanks everyone, Daniel
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index dbe57df2e199..dd58c0be9ce2 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2126,7 +2126,7 @@ static void kretprobe_rethook_handler(struct rethook_node *rh, void *data, struct kprobe_ctlblk *kcb; /* The data must NOT be null. This means rethook data structure is broken. */ - if (WARN_ON_ONCE(!data)) + if (WARN_ON_ONCE(!data) || !rp->handler) return; __this_cpu_write(current_kprobe, &rp->kp);
[PATCH bpf] x86/kprobes: Fix KRETPROBES when CONFIG_KRETPROBE_ON_RETHOOK is set The recent kernel change "kprobes: Use rethook for kretprobe if possible", introduced a potential NULL pointer dereference bug in the KRETPROBE mechanism. The official Kprobes documentation defines that "Any or all handlers can be NULL". Unfortunately, there is a missing return handler verification to fulfill these requirements and can result in a NULL pointer dereference bug. This patch adds such verification in kretprobe_rethook_handler() function. Fixes: 73f9b911faa7 ("kprobes: Use rethook for kretprobe if possible") Signed-off-by: Adam Zabrocki <pi3@pi3.com.pl> Acked-by: Masami Hiramatsu <mhiramat@kernel.org> --- kernel/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)