[PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jisheng Zhang]

Use the __vmalloc_node_range() to simplify x86's alloc_insn_page()
implementation.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 arch/x86/kernel/kprobes/core.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index df776cdca327..75081f3dbe44 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -383,26 +383,10 @@ static int prepare_boost(kprobe_opcode_t *buf, struct kprobe *p,
 /* Make page to RO mode when allocate it */
 void *alloc_insn_page(void)
 {
-	void *page;
-
-	page = module_alloc(PAGE_SIZE);
-	if (!page)
-		return NULL;
-
-	set_vm_flush_reset_perms(page);
-	/*
-	 * First make the page read-only, and only then make it executable to
-	 * prevent it from being W+X in between.
-	 */
-	set_memory_ro((unsigned long)page, 1);
-
-	/*
-	 * TODO: Once additional kernel code protection mechanisms are set, ensure
-	 * that the page was not maliciously altered and it is still zeroed.
-	 */
-	set_memory_x((unsigned long)page, 1);
-
-	return page;
+	return __vmalloc_node_range(PAGE_SIZE, PAGE_SIZE, VMALLOC_START,
+			VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_ROX,
+			VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
+			__builtin_return_address(0));
 }
 
 /* Recover page to RW mode before releasing it */
-- 
2.31.0

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Masami Hiramatsu]

Hi,

On Tue, 13 Apr 2021 18:03:24 +0800
Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:

> Use the __vmalloc_node_range() to simplify x86's alloc_insn_page()
> implementation.

Have you checked this is equivarent to the original code on
all architecture? IIRC, some arch has a special module_alloc(),
thus I NACKed similar patch previously.

Thank you,

> 
> Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> ---
>  arch/x86/kernel/kprobes/core.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index df776cdca327..75081f3dbe44 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -383,26 +383,10 @@ static int prepare_boost(kprobe_opcode_t *buf, struct kprobe *p,
>  /* Make page to RO mode when allocate it */
>  void *alloc_insn_page(void)
>  {
> -	void *page;
> -
> -	page = module_alloc(PAGE_SIZE);
> -	if (!page)
> -		return NULL;
> -
> -	set_vm_flush_reset_perms(page);
> -	/*
> -	 * First make the page read-only, and only then make it executable to
> -	 * prevent it from being W+X in between.
> -	 */
> -	set_memory_ro((unsigned long)page, 1);
> -
> -	/*
> -	 * TODO: Once additional kernel code protection mechanisms are set, ensure
> -	 * that the page was not maliciously altered and it is still zeroed.
> -	 */
> -	set_memory_x((unsigned long)page, 1);
> -
> -	return page;
> +	return __vmalloc_node_range(PAGE_SIZE, PAGE_SIZE, VMALLOC_START,
> +			VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_ROX,
> +			VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
> +			__builtin_return_address(0));
>  }
>  
>  /* Recover page to RW mode before releasing it */
> -- 
> 2.31.0
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jisheng Zhang]

On Tue, 13 Apr 2021 22:00:30 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:


> 
> 
> Hi,

Hi

> 
> On Tue, 13 Apr 2021 18:03:24 +0800
> Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> 
> > Use the __vmalloc_node_range() to simplify x86's alloc_insn_page()
> > implementation.  
> 
> Have you checked this is equivarent to the original code on
> all architecture? IIRC, some arch has a special module_alloc(),

Indeed, this isn't equivarent to the original code. FWICT, the differences
on x86 are:

1) module_alloc() allocates a special vmalloc range
2) module_alloc() randomizes the return address via. module_load_offset()
3) module_alloc() also supports kasan instrumentation by kasan_module_alloc()

But I'm not sure whether the above differences are useful for kprobes ss insn
slot page or not. Take 1) for example, special range in module_alloc is
due to relative jump limitation, modules need to call kernel .text. does
kprobes ss ins slot needs this limitation too?

Thanks


> thus I NACKed similar patch previously.
> 
> Thank you,
> 
> >
> > Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
> > ---
> >  arch/x86/kernel/kprobes/core.c | 24 ++++--------------------
> >  1 file changed, 4 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index df776cdca327..75081f3dbe44 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -383,26 +383,10 @@ static int prepare_boost(kprobe_opcode_t *buf, struct kprobe *p,
> >  /* Make page to RO mode when allocate it */
> >  void *alloc_insn_page(void)
> >  {
> > -     void *page;
> > -
> > -     page = module_alloc(PAGE_SIZE);
> > -     if (!page)
> > -             return NULL;
> > -
> > -     set_vm_flush_reset_perms(page);
> > -     /*
> > -      * First make the page read-only, and only then make it executable to
> > -      * prevent it from being W+X in between.
> > -      */
> > -     set_memory_ro((unsigned long)page, 1);
> > -
> > -     /*
> > -      * TODO: Once additional kernel code protection mechanisms are set, ensure
> > -      * that the page was not maliciously altered and it is still zeroed.
> > -      */
> > -     set_memory_x((unsigned long)page, 1);
> > -
> > -     return page;
> > +     return __vmalloc_node_range(PAGE_SIZE, PAGE_SIZE, VMALLOC_START,
> > +                     VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_ROX,
> > +                     VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
> > +                     __builtin_return_address(0));
> >  }
> >
> >  /* Recover page to RW mode before releasing it */
> > --
> > 2.31.0
> >  
> 
> 
> --
> Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jisheng Zhang]

Jisheng Zhang wrote:

> 
> 
> Hi,  

Hi

> 
> On Tue, 13 Apr 2021 18:03:24 +0800
> Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>   
> > Use the __vmalloc_node_range() to simplify x86's alloc_insn_page() 
> > implementation.  
> 
> Have you checked this is equivarent to the original code on all 
> architecture? IIRC, some arch has a special module_alloc(),  

> Indeed, this isn't equivarent to the original code. FWICT, the differences on x86 are:

> 1) module_alloc() allocates a special vmalloc range
> 2) module_alloc() randomizes the return address via. module_load_offset()
> 3) module_alloc() also supports kasan instrumentation by kasan_module_alloc()

> But I'm not sure whether the above differences are useful for kprobes ss
> insn slot page or not. Take 1) for example, special range in module_alloc
> is due to relative jump limitation, modules need to call kernel .text. does
> kprobes ss ins slot needs this limitation too?

Oops, I found this wonderful thread:
https://www.lkml.org/lkml/2020/7/28/1413

So kprobes ss ins slot page "must be in the range of relative branching only
for x86 and arm"

And Jarkko's "arch/x86: kprobes: Remove MODULES dependency" series look
much better. The last version is v5, I'm not sure whether Jarkko will
send new version to mainline the series.

thanks

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Masami Hiramatsu]

Hi Jisheng,

On Wed, 14 Apr 2021 15:27:28 +0800
Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:

\
> > 
> > On Tue, 13 Apr 2021 18:03:24 +0800
> > Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> >   
> > > Use the __vmalloc_node_range() to simplify x86's alloc_insn_page() 
> > > implementation.  
> > 
> > Have you checked this is equivarent to the original code on all 
> > architecture? IIRC, some arch has a special module_alloc(),  
> 
> > Indeed, this isn't equivarent to the original code. FWICT, the differences on x86 are:
> 
> > 1) module_alloc() allocates a special vmalloc range
> > 2) module_alloc() randomizes the return address via. module_load_offset()
> > 3) module_alloc() also supports kasan instrumentation by kasan_module_alloc()
> 
> > But I'm not sure whether the above differences are useful for kprobes ss
> > insn slot page or not. Take 1) for example, special range in module_alloc
> > is due to relative jump limitation, modules need to call kernel .text. does
> > kprobes ss ins slot needs this limitation too?
> 
> Oops, I found this wonderful thread:
> https://www.lkml.org/lkml/2020/7/28/1413
> 
> So kprobes ss ins slot page "must be in the range of relative branching only
> for x86 and arm"

Yes, at this moment. (Not sure we can introduce similar feature on other arch too)

> 
> And Jarkko's "arch/x86: kprobes: Remove MODULES dependency" series look
> much better. The last version is v5, I'm not sure whether Jarkko will
> send new version to mainline the series.

I hope so. If module_alloc() itself is implemented on the generic text_alloc(),
I can replace the module_alloc() with text_alloc(). 

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jarkko Sakkinen]

On Wed, Apr 14, 2021 at 03:27:28PM +0800, Jisheng Zhang wrote:
> Jisheng Zhang wrote:
> 
> > 
> > 
> > Hi,  
> 
> Hi
> 
> > 
> > On Tue, 13 Apr 2021 18:03:24 +0800
> > Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> >   
> > > Use the __vmalloc_node_range() to simplify x86's alloc_insn_page() 
> > > implementation.  
> > 
> > Have you checked this is equivarent to the original code on all 
> > architecture? IIRC, some arch has a special module_alloc(),  
> 
> > Indeed, this isn't equivarent to the original code. FWICT, the differences on x86 are:
> 
> > 1) module_alloc() allocates a special vmalloc range
> > 2) module_alloc() randomizes the return address via. module_load_offset()
> > 3) module_alloc() also supports kasan instrumentation by kasan_module_alloc()
> 
> > But I'm not sure whether the above differences are useful for kprobes ss
> > insn slot page or not. Take 1) for example, special range in module_alloc
> > is due to relative jump limitation, modules need to call kernel .text. does
> > kprobes ss ins slot needs this limitation too?
> 
> Oops, I found this wonderful thread:
> https://www.lkml.org/lkml/2020/7/28/1413
> 
> So kprobes ss ins slot page "must be in the range of relative branching only
> for x86 and arm"
> 
> And Jarkko's "arch/x86: kprobes: Remove MODULES dependency" series look
> much better. The last version is v5, I'm not sure whether Jarkko will
> send new version to mainline the series.

Ya, I got really busy with upstreaming SGX. That's why this was left out
(but luckily SGX got finally into upstream).

Thanks for reminding. Any motivation to pick it up and continue where I
left of?

/Jarkko

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jarkko Sakkinen]

On Wed, Apr 14, 2021 at 05:22:58PM +0900, Masami Hiramatsu wrote:
> Hi Jisheng,
> 
> On Wed, 14 Apr 2021 15:27:28 +0800
> Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> 
> \
> > > 
> > > On Tue, 13 Apr 2021 18:03:24 +0800
> > > Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
> > >   
> > > > Use the __vmalloc_node_range() to simplify x86's alloc_insn_page() 
> > > > implementation.  
> > > 
> > > Have you checked this is equivarent to the original code on all 
> > > architecture? IIRC, some arch has a special module_alloc(),  
> > 
> > > Indeed, this isn't equivarent to the original code. FWICT, the differences on x86 are:
> > 
> > > 1) module_alloc() allocates a special vmalloc range
> > > 2) module_alloc() randomizes the return address via. module_load_offset()
> > > 3) module_alloc() also supports kasan instrumentation by kasan_module_alloc()
> > 
> > > But I'm not sure whether the above differences are useful for kprobes ss
> > > insn slot page or not. Take 1) for example, special range in module_alloc
> > > is due to relative jump limitation, modules need to call kernel .text. does
> > > kprobes ss ins slot needs this limitation too?
> > 
> > Oops, I found this wonderful thread:
> > https://www.lkml.org/lkml/2020/7/28/1413
> > 
> > So kprobes ss ins slot page "must be in the range of relative branching only
> > for x86 and arm"
> 
> Yes, at this moment. (Not sure we can introduce similar feature on other arch too)
> 
> > 
> > And Jarkko's "arch/x86: kprobes: Remove MODULES dependency" series look
> > much better. The last version is v5, I'm not sure whether Jarkko will
> > send new version to mainline the series.
> 
> I hope so. If module_alloc() itself is implemented on the generic text_alloc(),
> I can replace the module_alloc() with text_alloc(). 

I can of course look into this too. Right now in two vacation coming back
end of this month.

/Jarkko

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jisheng Zhang]

On Wed, 14 Apr 2021 16:12:23 +0300 Jarkko Sakkinen <jarkko@kernel.org> wrote:


> > So kprobes ss ins slot page "must be in the range of relative branching only
> > for x86 and arm"
> >
> > And Jarkko's "arch/x86: kprobes: Remove MODULES dependency" series look
> > much better. The last version is v5, I'm not sure whether Jarkko will
> > send new version to mainline the series.  
> 
> Ya, I got really busy with upstreaming SGX. That's why this was left out
> (but luckily SGX got finally into upstream).
> 
> Thanks for reminding. Any motivation to pick it up and continue where I
> left of?
> 

I can try, I will try to send patches once next rc1 is released.

thanks

Re: [PATCH] x86/kprobes: Simplify alloc_insn_page() with __vmalloc_node_range

[Jarkko Sakkinen]

On Fri, Apr 16, 2021 at 03:06:16PM +0800, Jisheng Zhang wrote:
> On Wed, 14 Apr 2021 16:12:23 +0300 Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> 
> > > So kprobes ss ins slot page "must be in the range of relative branching only
> > > for x86 and arm"
> > >
> > > And Jarkko's "arch/x86: kprobes: Remove MODULES dependency" series look
> > > much better. The last version is v5, I'm not sure whether Jarkko will
> > > send new version to mainline the series.  
> > 
> > Ya, I got really busy with upstreaming SGX. That's why this was left out
> > (but luckily SGX got finally into upstream).
> > 
> > Thanks for reminding. Any motivation to pick it up and continue where I
> > left of?
> > 
> 
> I can try, I will try to send patches once next rc1 is released.
> 
> thanks

Alright, thanks. Be sure to CC me, I'm happy to test them in my
environment.

/Jarkko