| Message ID | 20220801033719.228248-1-chenzhongjin@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3] kprobes: Forbid probing on trampoline and bpf prog | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
On Mon, Aug 01, 2022 at 11:37:19AM +0800, Chen Zhongjin wrote: > kernel_text_address returns ftrace_trampoline, kprobe_insn_slot > and bpf_text_address as kprobe legal address. > > These text are removable and changeable without any notifier to > kprobes. Probing on them can trigger some unexpected behavior[1]. > > Considering that jump_label and static_call text are already be > forbiden to probe, kernel_text_address should be replaced with > core_kernel_text and is_module_text_address to check other text > which is unsafe to kprobe. > > [1] https://lkml.org/lkml/2022/7/26/1148 > > Fixes: 5b485629ba0d ("kprobes, extable: Identify kprobes trampolines as kernel text area") > Fixes: 74451e66d516 ("bpf: make jited programs visible in traces") > Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com> > --- > v2 -> v3: > Remove '-next' carelessly added in title. LGTM cc-ing Steven because it affects ftrace as well jirka > > v1 -> v2: > Check core_kernel_text and is_module_text_address rather than > only kprobe_insn. > Also fix title and commit message for this. See old patch at [1]. > --- > kernel/kprobes.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index f214f8c088ed..80697e5e03e4 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, > preempt_disable(); > > /* Ensure it is not in reserved area nor out of text */ > - if (!kernel_text_address((unsigned long) p->addr) || > + if (!(core_kernel_text((unsigned long) p->addr) || > + is_module_text_address((unsigned long) p->addr)) || > within_kprobe_blacklist((unsigned long) p->addr) || > jump_label_text_reserved(p->addr, p->addr) || > static_call_text_reserved(p->addr, p->addr) || > -- > 2.17.1 >
On Mon, 1 Aug 2022 22:41:19 +0200 Jiri Olsa <olsajiri@gmail.com> wrote: > LGTM cc-ing Steven because it affects ftrace as well Thanks for the Cc, but I don't quite see how it affects ftrace. Unless you are just saying how it can affect kprobe_events? -- Steve > > jirka > > > > > v1 -> v2: > > Check core_kernel_text and is_module_text_address rather than > > only kprobe_insn. > > Also fix title and commit message for this. See old patch at [1]. > > --- > > kernel/kprobes.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > index f214f8c088ed..80697e5e03e4 100644 > > --- a/kernel/kprobes.c > > +++ b/kernel/kprobes.c > > @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, > > preempt_disable(); > > > > /* Ensure it is not in reserved area nor out of text */ > > - if (!kernel_text_address((unsigned long) p->addr) || > > + if (!(core_kernel_text((unsigned long) p->addr) || > > + is_module_text_address((unsigned long) p->addr)) || > > within_kprobe_blacklist((unsigned long) p->addr) || > > jump_label_text_reserved(p->addr, p->addr) || > > static_call_text_reserved(p->addr, p->addr) || > > -- > > 2.17.1 > >
On Mon, 1 Aug 2022 11:37:19 +0800 Chen Zhongjin <chenzhongjin@huawei.com> wrote: > kernel_text_address returns ftrace_trampoline, kprobe_insn_slot > and bpf_text_address as kprobe legal address. > > These text are removable and changeable without any notifier to > kprobes. Probing on them can trigger some unexpected behavior[1]. > > Considering that jump_label and static_call text are already be > forbiden to probe, kernel_text_address should be replaced with > core_kernel_text and is_module_text_address to check other text > which is unsafe to kprobe. > > [1] https://lkml.org/lkml/2022/7/26/1148 > > Fixes: 5b485629ba0d ("kprobes, extable: Identify kprobes trampolines as kernel text area") > Fixes: 74451e66d516 ("bpf: make jited programs visible in traces") > Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com> Thanks! this looks good to me. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > --- > v2 -> v3: > Remove '-next' carelessly added in title. > > v1 -> v2: > Check core_kernel_text and is_module_text_address rather than > only kprobe_insn. > Also fix title and commit message for this. See old patch at [1]. > --- > kernel/kprobes.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > index f214f8c088ed..80697e5e03e4 100644 > --- a/kernel/kprobes.c > +++ b/kernel/kprobes.c > @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, > preempt_disable(); > > /* Ensure it is not in reserved area nor out of text */ > - if (!kernel_text_address((unsigned long) p->addr) || > + if (!(core_kernel_text((unsigned long) p->addr) || > + is_module_text_address((unsigned long) p->addr)) || > within_kprobe_blacklist((unsigned long) p->addr) || > jump_label_text_reserved(p->addr, p->addr) || > static_call_text_reserved(p->addr, p->addr) || > -- > 2.17.1 >
On Mon, 1 Aug 2022 16:51:46 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 1 Aug 2022 22:41:19 +0200 > Jiri Olsa <olsajiri@gmail.com> wrote: > > > LGTM cc-ing Steven because it affects ftrace as well > > Thanks for the Cc, but I don't quite see how it affects ftrace. > > Unless you are just saying how it can affect kprobe_events? Maybe kprobe_events can probe the ftrace trampoline buffer if CONFIG_KPROBE_EVENTS_ON_NOTRACE=y. > > -- Steve > > > > > > jirka > > > > > > > > v1 -> v2: > > > Check core_kernel_text and is_module_text_address rather than > > > only kprobe_insn. > > > Also fix title and commit message for this. See old patch at [1]. > > > --- > > > kernel/kprobes.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > > index f214f8c088ed..80697e5e03e4 100644 > > > --- a/kernel/kprobes.c > > > +++ b/kernel/kprobes.c > > > @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, > > > preempt_disable(); > > > > > > /* Ensure it is not in reserved area nor out of text */ > > > - if (!kernel_text_address((unsigned long) p->addr) || > > > + if (!(core_kernel_text((unsigned long) p->addr) || > > > + is_module_text_address((unsigned long) p->addr)) || > > > within_kprobe_blacklist((unsigned long) p->addr) || > > > jump_label_text_reserved(p->addr, p->addr) || > > > static_call_text_reserved(p->addr, p->addr) || > > > -- > > > 2.17.1 > > > >
On Mon, Aug 01, 2022 at 04:51:46PM -0400, Steven Rostedt wrote: > On Mon, 1 Aug 2022 22:41:19 +0200 > Jiri Olsa <olsajiri@gmail.com> wrote: > > > LGTM cc-ing Steven because it affects ftrace as well > > Thanks for the Cc, but I don't quite see how it affects ftrace. > > Unless you are just saying how it can affect kprobe_events? nope, I just saw the 'ftrace' in changelog ;-) anyway the patch makes check_kprobe_address_safe to fail on ftrace trampoline address.. but not sure you could make kprobe on ftrace trampoline before, probably not jirka > > -- Steve > > > > > > jirka > > > > > > > > v1 -> v2: > > > Check core_kernel_text and is_module_text_address rather than > > > only kprobe_insn. > > > Also fix title and commit message for this. See old patch at [1]. > > > --- > > > kernel/kprobes.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c > > > index f214f8c088ed..80697e5e03e4 100644 > > > --- a/kernel/kprobes.c > > > +++ b/kernel/kprobes.c > > > @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, > > > preempt_disable(); > > > > > > /* Ensure it is not in reserved area nor out of text */ > > > - if (!kernel_text_address((unsigned long) p->addr) || > > > + if (!(core_kernel_text((unsigned long) p->addr) || > > > + is_module_text_address((unsigned long) p->addr)) || > > > within_kprobe_blacklist((unsigned long) p->addr) || > > > jump_label_text_reserved(p->addr, p->addr) || > > > static_call_text_reserved(p->addr, p->addr) || > > > -- > > > 2.17.1 > > > >
On 2022/8/2 17:06, Jiri Olsa wrote: > On Mon, Aug 01, 2022 at 04:51:46PM -0400, Steven Rostedt wrote: >> On Mon, 1 Aug 2022 22:41:19 +0200 >> Jiri Olsa <olsajiri@gmail.com> wrote: >> >>> LGTM cc-ing Steven because it affects ftrace as well >> Thanks for the Cc, but I don't quite see how it affects ftrace. >> >> Unless you are just saying how it can affect kprobe_events? > nope, I just saw the 'ftrace' in changelog ;-) > > anyway the patch makes check_kprobe_address_safe to fail > on ftrace trampoline address.. but not sure you could make > kprobe on ftrace trampoline before, probably not > > jirka In fact with CONFIG_KPROBE_EVENTS_ON_NOTRACE=y it can happen. But I think ftrace has no responsibility to promise the address safety when this option open. Best, Chen >> -- Steve >> >> >>> jirka >>> >>>> v1 -> v2: >>>> Check core_kernel_text and is_module_text_address rather than >>>> only kprobe_insn. >>>> Also fix title and commit message for this. See old patch at [1]. >>>> --- >>>> kernel/kprobes.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c >>>> index f214f8c088ed..80697e5e03e4 100644 >>>> --- a/kernel/kprobes.c >>>> +++ b/kernel/kprobes.c >>>> @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, >>>> preempt_disable(); >>>> >>>> /* Ensure it is not in reserved area nor out of text */ >>>> - if (!kernel_text_address((unsigned long) p->addr) || >>>> + if (!(core_kernel_text((unsigned long) p->addr) || >>>> + is_module_text_address((unsigned long) p->addr)) || >>>> within_kprobe_blacklist((unsigned long) p->addr) || >>>> jump_label_text_reserved(p->addr, p->addr) || >>>> static_call_text_reserved(p->addr, p->addr) || >>>> -- >>>> 2.17.1 >>>>
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index f214f8c088ed..80697e5e03e4 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -1560,7 +1560,8 @@ static int check_kprobe_address_safe(struct kprobe *p, preempt_disable(); /* Ensure it is not in reserved area nor out of text */ - if (!kernel_text_address((unsigned long) p->addr) || + if (!(core_kernel_text((unsigned long) p->addr) || + is_module_text_address((unsigned long) p->addr)) || within_kprobe_blacklist((unsigned long) p->addr) || jump_label_text_reserved(p->addr, p->addr) || static_call_text_reserved(p->addr, p->addr) ||
kernel_text_address returns ftrace_trampoline, kprobe_insn_slot and bpf_text_address as kprobe legal address. These text are removable and changeable without any notifier to kprobes. Probing on them can trigger some unexpected behavior[1]. Considering that jump_label and static_call text are already be forbiden to probe, kernel_text_address should be replaced with core_kernel_text and is_module_text_address to check other text which is unsafe to kprobe. [1] https://lkml.org/lkml/2022/7/26/1148 Fixes: 5b485629ba0d ("kprobes, extable: Identify kprobes trampolines as kernel text area") Fixes: 74451e66d516 ("bpf: make jited programs visible in traces") Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com> --- v2 -> v3: Remove '-next' carelessly added in title. v1 -> v2: Check core_kernel_text and is_module_text_address rather than only kprobe_insn. Also fix title and commit message for this. See old patch at [1]. --- kernel/kprobes.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)