[PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Chen Zhongjin]

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(-)

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

Re: [PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Jiri Olsa]

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
> 

Re: [PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Steven Rostedt]

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
> >   

Re: [PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Masami Hiramatsu]

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
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Masami Hiramatsu]

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
> > >   
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Jiri Olsa]

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
> > >   
> 

Re: [PATCH v3] kprobes: Forbid probing on trampoline and bpf prog

[Chen Zhongjin]

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
>>>>