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

[Jisheng Zhang <Jisheng.Zhang@synaptics.com>]

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>