fix a hyperv W^X violation and remove vmalloc_exec

fix a hyperv W^X violation and remove vmalloc_exec

[Christoph Hellwig]

Hi all,

Dexuan reported a W^X violation due to the fact that the hyper hypercall
page due switching it to be allocated using vmalloc_exec.  The problem
is that PAGE_KERNEL_EXEC as used by vmalloc_exec actually sets writable
permissions in the pte.  This series fixes the issue by switching to the
low-level __vmalloc_node_range interface that allows specifing more
detailed permissions instead.  It then also open codes the other two
callers and removes the somewhat confusing vmalloc_exec interface.

Peter noted that the hyper hypercall page allocation also has another
long standing issue in that it shouldn't use the full vmalloc but just
the module space.  This issue is so far theoretical as the allocation is
done early in the boot process.  I plan to fix it with another bigger
series for 5.9.

[PATCH 1/3] x86/hyperv: allocate the hypercall page with only read and execute bits

[Christoph Hellwig]

Avoid a W^X violation cause by the fact that PAGE_KERNEL_EXEC includes the
writable bit.

For this resurrect the removed PAGE_KERNEL_RX definitіon, but as
PAGE_KERNEL_ROX to match arm64 and powerpc.

Fixes: 78bb17f76edc ("x86/hyperv: use vmalloc_exec for the hypercall page")
Reported-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/hyperv/hv_init.c            | 4 +++-
 arch/x86/include/asm/pgtable_types.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index a54c6a401581dd..2bdc72e6890eca 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -375,7 +375,9 @@ void __init hyperv_init(void)
 	guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
 	wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
 
-	hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
+	hv_hypercall_pg = __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START,
+			VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_ROX,
+			VM_FLUSH_RESET_PERMS, NUMA_NO_NODE, __func__);
 	if (hv_hypercall_pg == NULL) {
 		wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
 		goto remove_cpuhp_state;
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 2da1f95b88d761..816b31c685505f 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -194,6 +194,7 @@ enum page_cache_mode {
 #define _PAGE_TABLE_NOENC	 (__PP|__RW|_USR|___A|   0|___D|   0|   0)
 #define _PAGE_TABLE		 (__PP|__RW|_USR|___A|   0|___D|   0|   0| _ENC)
 #define __PAGE_KERNEL_RO	 (__PP|   0|   0|___A|__NX|___D|   0|___G)
+#define __PAGE_KERNEL_ROX	 (__PP|   0|   0|___A|   0|___D|   0|___G)
 #define __PAGE_KERNEL_NOCACHE	 (__PP|__RW|   0|___A|__NX|___D|   0|___G| __NC)
 #define __PAGE_KERNEL_VVAR	 (__PP|   0|_USR|___A|__NX|___D|   0|___G)
 #define __PAGE_KERNEL_LARGE	 (__PP|__RW|   0|___A|__NX|___D|_PSE|___G)
@@ -219,6 +220,7 @@ enum page_cache_mode {
 #define PAGE_KERNEL_RO		__pgprot_mask(__PAGE_KERNEL_RO         | _ENC)
 #define PAGE_KERNEL_EXEC	__pgprot_mask(__PAGE_KERNEL_EXEC       | _ENC)
 #define PAGE_KERNEL_EXEC_NOENC	__pgprot_mask(__PAGE_KERNEL_EXEC       |    0)
+#define PAGE_KERNEL_ROX		__pgprot_mask(__PAGE_KERNEL_ROX        | _ENC)
 #define PAGE_KERNEL_NOCACHE	__pgprot_mask(__PAGE_KERNEL_NOCACHE    | _ENC)
 #define PAGE_KERNEL_LARGE	__pgprot_mask(__PAGE_KERNEL_LARGE      | _ENC)
 #define PAGE_KERNEL_LARGE_EXEC	__pgprot_mask(__PAGE_KERNEL_LARGE_EXEC | _ENC)
-- 
2.26.2

[PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Christoph Hellwig]

Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
page read-only just after the allocation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm64/kernel/probes/kprobes.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index d1c95dcf1d7833..cbe49cd117cfec 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 
 void *alloc_insn_page(void)
 {
-	void *page;
-
-	page = vmalloc_exec(PAGE_SIZE);
-	if (page) {
-		set_memory_ro((unsigned long)page, 1);
-		set_vm_flush_reset_perms(page);
-	}
-
-	return page;
+	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
+			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
+			NUMA_NO_NODE, __func__);
 }
 
 /* arm kprobe: install breakpoint in text */
-- 
2.26.2

[PATCH 3/3] mm: remove vmalloc_exec

[Christoph Hellwig]

Merge vmalloc_exec into its only caller.  Note that for !CONFIG_MMU
__vmalloc_node_range maps to __vmalloc, which directly clears the
__GFP_HIGHMEM added by the vmalloc_exec stub anyway.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/vmalloc.h |  1 -
 kernel/module.c         |  4 +++-
 mm/nommu.c              | 17 -----------------
 mm/vmalloc.c            | 20 --------------------
 4 files changed, 3 insertions(+), 39 deletions(-)

diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 48bb681e6c2aed..0221f852a7e1a3 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -106,7 +106,6 @@ extern void *vzalloc(unsigned long size);
 extern void *vmalloc_user(unsigned long size);
 extern void *vmalloc_node(unsigned long size, int node);
 extern void *vzalloc_node(unsigned long size, int node);
-extern void *vmalloc_exec(unsigned long size);
 extern void *vmalloc_32(unsigned long size);
 extern void *vmalloc_32_user(unsigned long size);
 extern void *__vmalloc(unsigned long size, gfp_t gfp_mask);
diff --git a/kernel/module.c b/kernel/module.c
index e8a198588f26ee..0c6573b98c3662 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2783,7 +2783,9 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
 
 void * __weak module_alloc(unsigned long size)
 {
-	return vmalloc_exec(size);
+	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
+			GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
+			NUMA_NO_NODE, __func__);
 }
 
 bool __weak module_init_section(const char *name)
diff --git a/mm/nommu.c b/mm/nommu.c
index cdcad5d61dd194..f32a69095d509e 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -290,23 +290,6 @@ void *vzalloc_node(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node);
 
-/**
- *	vmalloc_exec  -  allocate virtually contiguous, executable memory
- *	@size:		allocation size
- *
- *	Kernel-internal function to allocate enough pages to cover @size
- *	the page level allocator and map them into contiguous and
- *	executable kernel virtual space.
- *
- *	For tight control over page level allocator and protection flags
- *	use __vmalloc() instead.
- */
-
-void *vmalloc_exec(unsigned long size)
-{
-	return __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM);
-}
-
 /**
  * vmalloc_32  -  allocate virtually contiguous memory (32bit addressable)
  *	@size:		allocation size
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 3091c2ca60dfd1..f60948aac0d0e4 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2696,26 +2696,6 @@ void *vzalloc_node(unsigned long size, int node)
 }
 EXPORT_SYMBOL(vzalloc_node);
 
-/**
- * vmalloc_exec - allocate virtually contiguous, executable memory
- * @size:	  allocation size
- *
- * Kernel-internal function to allocate enough pages to cover @size
- * the page level allocator and map them into contiguous and
- * executable kernel virtual space.
- *
- * For tight control over page level allocator and protection flags
- * use __vmalloc() instead.
- *
- * Return: pointer to the allocated memory or %NULL on error
- */
-void *vmalloc_exec(unsigned long size)
-{
-	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
-			GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
-			NUMA_NO_NODE, __builtin_return_address(0));
-}
-
 #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
 #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)
-- 
2.26.2

Re: [PATCH 3/3] mm: remove vmalloc_exec

[David Hildenbrand]

On 18.06.20 08:43, Christoph Hellwig wrote:
> Merge vmalloc_exec into its only caller.  Note that for !CONFIG_MMU
> __vmalloc_node_range maps to __vmalloc, which directly clears the
> __GFP_HIGHMEM added by the vmalloc_exec stub anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/vmalloc.h |  1 -
>  kernel/module.c         |  4 +++-
>  mm/nommu.c              | 17 -----------------
>  mm/vmalloc.c            | 20 --------------------
>  4 files changed, 3 insertions(+), 39 deletions(-)
> 
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 48bb681e6c2aed..0221f852a7e1a3 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -106,7 +106,6 @@ extern void *vzalloc(unsigned long size);
>  extern void *vmalloc_user(unsigned long size);
>  extern void *vmalloc_node(unsigned long size, int node);
>  extern void *vzalloc_node(unsigned long size, int node);
> -extern void *vmalloc_exec(unsigned long size);
>  extern void *vmalloc_32(unsigned long size);
>  extern void *vmalloc_32_user(unsigned long size);
>  extern void *__vmalloc(unsigned long size, gfp_t gfp_mask);
> diff --git a/kernel/module.c b/kernel/module.c
> index e8a198588f26ee..0c6573b98c3662 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2783,7 +2783,9 @@ static void dynamic_debug_remove(struct module *mod, struct _ddebug *debug)
>  
>  void * __weak module_alloc(unsigned long size)
>  {
> -	return vmalloc_exec(size);
> +	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> +			GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
> +			NUMA_NO_NODE, __func__);
>  }
>  
>  bool __weak module_init_section(const char *name)
> diff --git a/mm/nommu.c b/mm/nommu.c
> index cdcad5d61dd194..f32a69095d509e 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -290,23 +290,6 @@ void *vzalloc_node(unsigned long size, int node)
>  }
>  EXPORT_SYMBOL(vzalloc_node);
>  
> -/**
> - *	vmalloc_exec  -  allocate virtually contiguous, executable memory
> - *	@size:		allocation size
> - *
> - *	Kernel-internal function to allocate enough pages to cover @size
> - *	the page level allocator and map them into contiguous and
> - *	executable kernel virtual space.
> - *
> - *	For tight control over page level allocator and protection flags
> - *	use __vmalloc() instead.
> - */
> -
> -void *vmalloc_exec(unsigned long size)
> -{
> -	return __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM);
> -}
> -
>  /**
>   * vmalloc_32  -  allocate virtually contiguous memory (32bit addressable)
>   *	@size:		allocation size
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 3091c2ca60dfd1..f60948aac0d0e4 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2696,26 +2696,6 @@ void *vzalloc_node(unsigned long size, int node)
>  }
>  EXPORT_SYMBOL(vzalloc_node);
>  
> -/**
> - * vmalloc_exec - allocate virtually contiguous, executable memory
> - * @size:	  allocation size
> - *
> - * Kernel-internal function to allocate enough pages to cover @size
> - * the page level allocator and map them into contiguous and
> - * executable kernel virtual space.
> - *
> - * For tight control over page level allocator and protection flags
> - * use __vmalloc() instead.
> - *
> - * Return: pointer to the allocated memory or %NULL on error
> - */
> -void *vmalloc_exec(unsigned long size)
> -{
> -	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> -			GFP_KERNEL, PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS,
> -			NUMA_NO_NODE, __builtin_return_address(0));
> -}
> -
>  #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
>  #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
>  #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)
> 

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[David Hildenbrand]

On 18.06.20 08:43, Christoph Hellwig wrote:
> Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> page read-only just after the allocation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm64/kernel/probes/kprobes.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d1c95dcf1d7833..cbe49cd117cfec 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  
>  void *alloc_insn_page(void)
>  {
> -	void *page;
> -
> -	page = vmalloc_exec(PAGE_SIZE);
> -	if (page) {
> -		set_memory_ro((unsigned long)page, 1);
> -		set_vm_flush_reset_perms(page);
> -	}
> -
> -	return page;
> +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> +			NUMA_NO_NODE, __func__);

I do wonder if something like vmalloc_prot(size, prot) would make this
(and the other two users) easier to read.

So instead of ripping out vmalloc_exec(), converting it into
vmalloc_prot() instead.

Did you consider that?

Apart from that LGTM

>  }
>  
>  /* arm kprobe: install breakpoint in text */
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 08:43:06AM +0200, Christoph Hellwig wrote:
> Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> page read-only just after the allocation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm64/kernel/probes/kprobes.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d1c95dcf1d7833..cbe49cd117cfec 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  
>  void *alloc_insn_page(void)
>  {
> -	void *page;
> -
> -	page = vmalloc_exec(PAGE_SIZE);
> -	if (page) {
> -		set_memory_ro((unsigned long)page, 1);
> -		set_vm_flush_reset_perms(page);
> -	}
> -
> -	return page;
> +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> +			NUMA_NO_NODE, __func__);
>  }

I think this has the exact same range issue as the x86 user. But it
might be less fatal if their PLT magic can cover the full range.

Re: fix a hyperv W^X violation and remove vmalloc_exec

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 08:43:04AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> Dexuan reported a W^X violation due to the fact that the hyper hypercall
> page due switching it to be allocated using vmalloc_exec.  The problem
> is that PAGE_KERNEL_EXEC as used by vmalloc_exec actually sets writable
> permissions in the pte.  This series fixes the issue by switching to the
> low-level __vmalloc_node_range interface that allows specifing more
> detailed permissions instead.  It then also open codes the other two
> callers and removes the somewhat confusing vmalloc_exec interface.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> Peter noted that the hyper hypercall page allocation also has another
> long standing issue in that it shouldn't use the full vmalloc but just
> the module space.  This issue is so far theoretical as the allocation is
> done early in the boot process.  I plan to fix it with another bigger
> series for 5.9.

Thanks!

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Peter Zijlstra]

On Thu, Jun 18, 2020 at 10:55:58AM +0200, David Hildenbrand wrote:
> On 18.06.20 08:43, Christoph Hellwig wrote:
> > Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> > page read-only just after the allocation.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  arch/arm64/kernel/probes/kprobes.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > index d1c95dcf1d7833..cbe49cd117cfec 100644
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> >  
> >  void *alloc_insn_page(void)
> >  {
> > -	void *page;
> > -
> > -	page = vmalloc_exec(PAGE_SIZE);
> > -	if (page) {
> > -		set_memory_ro((unsigned long)page, 1);
> > -		set_vm_flush_reset_perms(page);
> > -	}
> > -
> > -	return page;
> > +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > +			NUMA_NO_NODE, __func__);
> 
> I do wonder if something like vmalloc_prot(size, prot) would make this
> (and the other two users) easier to read.
> 
> So instead of ripping out vmalloc_exec(), converting it into
> vmalloc_prot() instead.
> 
> Did you consider that?

For x86 Christoph did module_alloc_prot(), which is in his more
extensive set of patches addressing this. I suspect that would be the
right thing for ARM64 as well.

Re: [PATCH 1/3] x86/hyperv: allocate the hypercall page with only read and execute bits

[Wei Liu]

On Thu, Jun 18, 2020 at 08:43:05AM +0200, Christoph Hellwig wrote:
> Avoid a W^X violation cause by the fact that PAGE_KERNEL_EXEC includes the
> writable bit.
> 
> For this resurrect the removed PAGE_KERNEL_RX definitіon, but as
> PAGE_KERNEL_ROX to match arm64 and powerpc.
> 
> Fixes: 78bb17f76edc ("x86/hyperv: use vmalloc_exec for the hypercall page")
> Reported-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Wei Liu <wei.liu@kernel.org>

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Christoph Hellwig]

On Thu, Jun 18, 2020 at 12:35:06PM +0200, Peter Zijlstra wrote:
> On Thu, Jun 18, 2020 at 10:55:58AM +0200, David Hildenbrand wrote:
> > On 18.06.20 08:43, Christoph Hellwig wrote:
> > > Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> > > page read-only just after the allocation.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  arch/arm64/kernel/probes/kprobes.c | 12 +++---------
> > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > > index d1c95dcf1d7833..cbe49cd117cfec 100644
> > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > >  
> > >  void *alloc_insn_page(void)
> > >  {
> > > -	void *page;
> > > -
> > > -	page = vmalloc_exec(PAGE_SIZE);
> > > -	if (page) {
> > > -		set_memory_ro((unsigned long)page, 1);
> > > -		set_vm_flush_reset_perms(page);
> > > -	}
> > > -
> > > -	return page;
> > > +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > > +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > > +			NUMA_NO_NODE, __func__);
> > 
> > I do wonder if something like vmalloc_prot(size, prot) would make this
> > (and the other two users) easier to read.
> > 
> > So instead of ripping out vmalloc_exec(), converting it into
> > vmalloc_prot() instead.
> > 
> > Did you consider that?
> 
> For x86 Christoph did module_alloc_prot(), which is in his more
> extensive set of patches addressing this. I suspect that would be the
> right thing for ARM64 as well.

Yes.  The somewhat hacky way I added it cause problems for UML, so I
instead plan to do a series converting all architectures over to
module_alloc_prot, plus lots of other cleanups in the area that I
noticed.

I don't think vmalloc_prot is a good idea per se, as there only few
potential users, and I don't want too many vmalloc APIs.

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Andrew Morton]

On Thu, 18 Jun 2020 08:43:06 +0200 Christoph Hellwig <hch@lst.de> wrote:

> Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> page read-only just after the allocation.
> 
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  
>  void *alloc_insn_page(void)
>  {
> -	void *page;
> -
> -	page = vmalloc_exec(PAGE_SIZE);
> -	if (page) {
> -		set_memory_ro((unsigned long)page, 1);
> -		set_vm_flush_reset_perms(page);
> -	}
> -
> -	return page;
> +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> +			NUMA_NO_NODE, __func__);
>  }
>  
>  /* arm kprobe: install breakpoint in text */

But why.  I think this is just a cleanup, doesn't address any runtime issue?

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Christoph Hellwig]

On Sat, Jun 20, 2020 at 07:16:16PM -0700, Andrew Morton wrote:
> On Thu, 18 Jun 2020 08:43:06 +0200 Christoph Hellwig <hch@lst.de> wrote:
> 
> > Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> > page read-only just after the allocation.
> > 
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> >  
> >  void *alloc_insn_page(void)
> >  {
> > -	void *page;
> > -
> > -	page = vmalloc_exec(PAGE_SIZE);
> > -	if (page) {
> > -		set_memory_ro((unsigned long)page, 1);
> > -		set_vm_flush_reset_perms(page);
> > -	}
> > -
> > -	return page;
> > +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > +			NUMA_NO_NODE, __func__);
> >  }
> >  
> >  /* arm kprobe: install breakpoint in text */
> 
> But why.  I think this is just a cleanup, doesn't address any runtime issue?

It doesn't "fix" an issue - it just simplifies and speeds up the code.

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Will Deacon]

On Tue, Jun 23, 2020 at 11:05:05AM +0200, Christoph Hellwig wrote:
> On Sat, Jun 20, 2020 at 07:16:16PM -0700, Andrew Morton wrote:
> > On Thu, 18 Jun 2020 08:43:06 +0200 Christoph Hellwig <hch@lst.de> wrote:
> > 
> > > Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> > > page read-only just after the allocation.
> > > 
> > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > >  
> > >  void *alloc_insn_page(void)
> > >  {
> > > -	void *page;
> > > -
> > > -	page = vmalloc_exec(PAGE_SIZE);
> > > -	if (page) {
> > > -		set_memory_ro((unsigned long)page, 1);
> > > -		set_vm_flush_reset_perms(page);
> > > -	}
> > > -
> > > -	return page;
> > > +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > > +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > > +			NUMA_NO_NODE, __func__);
> > >  }
> > >  
> > >  /* arm kprobe: install breakpoint in text */
> > 
> > But why.  I think this is just a cleanup, doesn't address any runtime issue?
> 
> It doesn't "fix" an issue - it just simplifies and speeds up the code.

Ok, but I don't understand the PLT comment from Peter in
20200618092754.GF576905@hirez.programming.kicks-ass.net:

  | I think this has the exact same range issue as the x86 user. But it
  | might be less fatal if their PLT magic can cover the full range.

Peter, please could you elaborate on your concern? I feel like I'm missing
some context.

Cheers,

Will

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Peter Zijlstra]

On Tue, Jun 23, 2020 at 10:07:58AM +0100, Will Deacon wrote:
> On Tue, Jun 23, 2020 at 11:05:05AM +0200, Christoph Hellwig wrote:
> > On Sat, Jun 20, 2020 at 07:16:16PM -0700, Andrew Morton wrote:
> > > On Thu, 18 Jun 2020 08:43:06 +0200 Christoph Hellwig <hch@lst.de> wrote:
> > > 
> > > > Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> > > > page read-only just after the allocation.
> > > > 
> > > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > > @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > > >  
> > > >  void *alloc_insn_page(void)
> > > >  {
> > > > -	void *page;
> > > > -
> > > > -	page = vmalloc_exec(PAGE_SIZE);
> > > > -	if (page) {
> > > > -		set_memory_ro((unsigned long)page, 1);
> > > > -		set_vm_flush_reset_perms(page);
> > > > -	}
> > > > -
> > > > -	return page;
> > > > +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > > > +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > > > +			NUMA_NO_NODE, __func__);
> > > >  }
> > > >  
> > > >  /* arm kprobe: install breakpoint in text */
> > > 
> > > But why.  I think this is just a cleanup, doesn't address any runtime issue?
> > 
> > It doesn't "fix" an issue - it just simplifies and speeds up the code.
> 
> Ok, but I don't understand the PLT comment from Peter in
> 20200618092754.GF576905@hirez.programming.kicks-ass.net:
> 
>   | I think this has the exact same range issue as the x86 user. But it
>   | might be less fatal if their PLT magic can cover the full range.
> 
> Peter, please could you elaborate on your concern? I feel like I'm missing
> some context.

On x86 we can only directly call code in a (signed) 32bit immediate
range (2G) and our kernel text and module range are constrained by that.

IIRC ARM64 has an even smaller immediate range and needs to play fixup
games with trampolines or somesuch (there was an ARM specific name for
it that I've misplaced again). Does that machinery cover the entire
vmalloc space or are you only able to fix up for a smaller range?

Your arch/arm64/kernel/module.c:module_alloc() implementation seems to
have an explicit module range different from the full vmalloc range, I'm
thinking this is for a reason.

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Will Deacon]

On Tue, Jun 23, 2020 at 11:37:14AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 23, 2020 at 10:07:58AM +0100, Will Deacon wrote:
> > On Tue, Jun 23, 2020 at 11:05:05AM +0200, Christoph Hellwig wrote:
> > > On Sat, Jun 20, 2020 at 07:16:16PM -0700, Andrew Morton wrote:
> > > > On Thu, 18 Jun 2020 08:43:06 +0200 Christoph Hellwig <hch@lst.de> wrote:
> > > > > --- a/arch/arm64/kernel/probes/kprobes.c
> > > > > +++ b/arch/arm64/kernel/probes/kprobes.c
> > > > > @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
> > > > >  
> > > > >  void *alloc_insn_page(void)
> > > > >  {
> > > > > -	void *page;
> > > > > -
> > > > > -	page = vmalloc_exec(PAGE_SIZE);
> > > > > -	if (page) {
> > > > > -		set_memory_ro((unsigned long)page, 1);
> > > > > -		set_vm_flush_reset_perms(page);
> > > > > -	}
> > > > > -
> > > > > -	return page;
> > > > > +	return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > > > > +			GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > > > > +			NUMA_NO_NODE, __func__);
> > > > >  }
> > > > >  
> > > > >  /* arm kprobe: install breakpoint in text */
> > > > 
> > > > But why.  I think this is just a cleanup, doesn't address any runtime issue?
> > > 
> > > It doesn't "fix" an issue - it just simplifies and speeds up the code.
> > 
> > Ok, but I don't understand the PLT comment from Peter in
> > 20200618092754.GF576905@hirez.programming.kicks-ass.net:
> > 
> >   | I think this has the exact same range issue as the x86 user. But it
> >   | might be less fatal if their PLT magic can cover the full range.
> > 
> > Peter, please could you elaborate on your concern? I feel like I'm missing
> > some context.
> 
> On x86 we can only directly call code in a (signed) 32bit immediate
> range (2G) and our kernel text and module range are constrained by that.
> 
> IIRC ARM64 has an even smaller immediate range and needs to play fixup
> games with trampolines or somesuch (there was an ARM specific name for
> it that I've misplaced again). Does that machinery cover the entire
> vmalloc space or are you only able to fix up for a smaller range?
> 
> Your arch/arm64/kernel/module.c:module_alloc() implementation seems to
> have an explicit module range different from the full vmalloc range, I'm
> thinking this is for a reason.

Ah, gotcha. In this case, we're talking about the kprobe out-of-line
buffer. We don't directly branch to that; instead we take a BRK exception
and either exception return + singlestep the OOL buffer, or we simulate
the instruction if it's doing anything PC-relative, so I don't see the
need for a PLT.

Will

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Ard Biesheuvel]

On Thu, 18 Jun 2020 at 08:44, Christoph Hellwig <hch@lst.de> wrote:
>
> Use PAGE_KERNEL_ROX directly instead of allocating RWX and setting the
> page read-only just after the allocation.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/arm64/kernel/probes/kprobes.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index d1c95dcf1d7833..cbe49cd117cfec 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -120,15 +120,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>
>  void *alloc_insn_page(void)
>  {
> -       void *page;
> -
> -       page = vmalloc_exec(PAGE_SIZE);
> -       if (page) {
> -               set_memory_ro((unsigned long)page, 1);
> -               set_vm_flush_reset_perms(page);
> -       }
> -
> -       return page;
> +       return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> +                       GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> +                       NUMA_NO_NODE, __func__);

Why is this passing a string for the 'caller' argument, and not the
address of the caller?


>  }
>
>  /* arm kprobe: install breakpoint in text */
> --
> 2.26.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Christoph Hellwig]

On Sat, Jun 27, 2020 at 09:34:42AM +0200, Ard Biesheuvel wrote:
> > +       return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > +                       GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > +                       NUMA_NO_NODE, __func__);
> 
> Why is this passing a string for the 'caller' argument, and not the
> address of the caller?

Dementia.  Fix sent.

Re: [PATCH 2/3] arm64: use PAGE_KERNEL_ROX directly in alloc_insn_page

[Ard Biesheuvel]

On Sat, 27 Jun 2020 at 09:57, Christoph Hellwig <hch@lst.de> wrote:
>
> On Sat, Jun 27, 2020 at 09:34:42AM +0200, Ard Biesheuvel wrote:
> > > +       return __vmalloc_node_range(PAGE_SIZE, 1, VMALLOC_START, VMALLOC_END,
> > > +                       GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS,
> > > +                       NUMA_NO_NODE, __func__);
> >
> > Why is this passing a string for the 'caller' argument, and not the
> > address of the caller?
>
> Dementia.  Fix sent.

:-)