LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v4] mm: Defer kmemleak object creation of module_alloc()
@ 2021-11-25  8:03 Kefeng Wang
  2021-11-25 16:07 ` Andrey Konovalov
  0 siblings, 1 reply; 2+ messages in thread
From: Kefeng Wang @ 2021-11-25  8:03 UTC (permalink / raw)
  To: Andrey Ryabinin, Dmitry Vyukov, Andrew Morton, linux-arm-kernel,
	linux-kernel, linux-s390, kasan-dev, linux-mm
  Cc: Catalin Marinas, Will Deacon, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger, Alexander Gordeev, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, Alexander Potapenko,
	Kefeng Wang, Yongqiang Liu

Yongqiang reports a kmemleak panic when module insmod/rmmod
with KASAN enabled(without KASAN_VMALLOC) on x86[1].

When the module area allocates memory, it's kmemleak_object
is created successfully, but the KASAN shadow memory of module
allocation is not ready, so when kmemleak scan the module's
pointer, it will panic due to no shadow memory with KASAN check.

module_alloc
  __vmalloc_node_range
    kmemleak_vmalloc
				kmemleak_scan
				  update_checksum
  kasan_module_alloc
    kmemleak_ignore

Note, there is no problem if KASAN_VMALLOC enabled, the modules
area entire shadow memory is preallocated. Thus, the bug only
exits on ARCH which supports dynamic allocation of module area
per module load, for now, only x86/arm64/s390 are involved.


Add a VM_DEFER_KMEMLEAK flags, defer vmalloc'ed object register
of kmemleak in module_alloc() to fix this issue.

[1] https://lore.kernel.org/all/6d41e2b9-4692-5ec4-b1cd-cbe29ae89739@huawei.com/

Fixes: 793213a82de4 ("s390/kasan: dynamic shadow mem allocation for modules")
Fixes: 39d114ddc682 ("arm64: add KASAN support")
Fixes: bebf56a1b176 ("kasan: enable instrumentation of global variables")
Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
V4:
- add fix tag
- fix missing change about VM_DELAY_KMEMLEAK
v3:
- update changelog to add more explanation
- use DEFER instead of DELAY sugguested by Catalin.
v2:
- fix type error on changelog and kasan_module_alloc()

 arch/arm64/kernel/module.c | 4 ++--
 arch/s390/kernel/module.c  | 5 +++--
 arch/x86/kernel/module.c   | 7 ++++---
 include/linux/kasan.h      | 4 ++--
 include/linux/vmalloc.h    | 7 +++++++
 mm/kasan/shadow.c          | 9 +++++++--
 mm/vmalloc.c               | 3 ++-
 7 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index b5ec010c481f..309a27553c87 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -36,7 +36,7 @@ void *module_alloc(unsigned long size)
 		module_alloc_end = MODULES_END;
 
 	p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
-				module_alloc_end, gfp_mask, PAGE_KERNEL, 0,
+				module_alloc_end, gfp_mask, PAGE_KERNEL, VM_DEFER_KMEMLEAK,
 				NUMA_NO_NODE, __builtin_return_address(0));
 
 	if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
@@ -58,7 +58,7 @@ void *module_alloc(unsigned long size)
 				PAGE_KERNEL, 0, NUMA_NO_NODE,
 				__builtin_return_address(0));
 
-	if (p && (kasan_module_alloc(p, size) < 0)) {
+	if (p && (kasan_module_alloc(p, size, gfp_mask) < 0)) {
 		vfree(p);
 		return NULL;
 	}
diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
index b01ba460b7ca..d52d85367bf7 100644
--- a/arch/s390/kernel/module.c
+++ b/arch/s390/kernel/module.c
@@ -37,14 +37,15 @@
 
 void *module_alloc(unsigned long size)
 {
+	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
 
 	if (PAGE_ALIGN(size) > MODULES_LEN)
 		return NULL;
 	p = __vmalloc_node_range(size, MODULE_ALIGN, MODULES_VADDR, MODULES_END,
-				 GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
+				 gfp_mask, PAGE_KERNEL_EXEC, VM_DEFER_KMEMLEAK, NUMA_NO_NODE,
 				 __builtin_return_address(0));
-	if (p && (kasan_module_alloc(p, size) < 0)) {
+	if (p && (kasan_module_alloc(p, size, gfp_mask) < 0)) {
 		vfree(p);
 		return NULL;
 	}
diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 169fb6f4cd2e..95fa745e310a 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -67,6 +67,7 @@ static unsigned long int get_module_load_offset(void)
 
 void *module_alloc(unsigned long size)
 {
+	gfp_t gfp_mask = GFP_KERNEL;
 	void *p;
 
 	if (PAGE_ALIGN(size) > MODULES_LEN)
@@ -74,10 +75,10 @@ void *module_alloc(unsigned long size)
 
 	p = __vmalloc_node_range(size, MODULE_ALIGN,
 				    MODULES_VADDR + get_module_load_offset(),
-				    MODULES_END, GFP_KERNEL,
-				    PAGE_KERNEL, 0, NUMA_NO_NODE,
+				    MODULES_END, gfp_mask,
+				    PAGE_KERNEL, VM_DEFER_KMEMLEAK, NUMA_NO_NODE,
 				    __builtin_return_address(0));
-	if (p && (kasan_module_alloc(p, size) < 0)) {
+	if (p && (kasan_module_alloc(p, size, gfp_mask) < 0)) {
 		vfree(p);
 		return NULL;
 	}
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index d8783b682669..89c99e5e67de 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -474,12 +474,12 @@ static inline void kasan_populate_early_vm_area_shadow(void *start,
  * allocations with real shadow memory. With KASAN vmalloc, the special
  * case is unnecessary, as the work is handled in the generic case.
  */
-int kasan_module_alloc(void *addr, size_t size);
+int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask);
 void kasan_free_shadow(const struct vm_struct *vm);
 
 #else /* (CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS) && !CONFIG_KASAN_VMALLOC */
 
-static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
+static inline int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask) { return 0; }
 static inline void kasan_free_shadow(const struct vm_struct *vm) {}
 
 #endif /* (CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS) && !CONFIG_KASAN_VMALLOC */
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 6e022cc712e6..506fc6e6a126 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -28,6 +28,13 @@ struct notifier_block;		/* in notifier.h */
 #define VM_MAP_PUT_PAGES	0x00000200	/* put pages and free array in vfree */
 #define VM_NO_HUGE_VMAP		0x00000400	/* force PAGE_SIZE pte mapping */
 
+#if defined(CONFIG_KASAN) && (defined(CONFIG_KASAN_GENERIC) || \
+	defined(CONFIG_KASAN_SW_TAGS)) && !defined(CONFIG_KASAN_VMALLOC)
+#define VM_DEFER_KMEMLEAK	0x00000800	/* defer kmemleak object creation */
+#else
+#define VM_DEFER_KMEMLEAK	0
+#endif
+
 /*
  * VM_KASAN is used slightly differently depending on CONFIG_KASAN_VMALLOC.
  *
diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
index 4a4929b29a23..2ade2f484562 100644
--- a/mm/kasan/shadow.c
+++ b/mm/kasan/shadow.c
@@ -498,7 +498,7 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
 
 #else /* CONFIG_KASAN_VMALLOC */
 
-int kasan_module_alloc(void *addr, size_t size)
+int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask)
 {
 	void *ret;
 	size_t scaled_size;
@@ -520,9 +520,14 @@ int kasan_module_alloc(void *addr, size_t size)
 			__builtin_return_address(0));
 
 	if (ret) {
+		struct vm_struct *vm = find_vm_area(addr);
 		__memset(ret, KASAN_SHADOW_INIT, shadow_size);
-		find_vm_area(addr)->flags |= VM_KASAN;
+		vm->flags |= VM_KASAN;
 		kmemleak_ignore(ret);
+
+		if (vm->flags & VM_DEFER_KMEMLEAK)
+			kmemleak_vmalloc(vm, size, gfp_mask);
+
 		return 0;
 	}
 
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d2a00ad4e1dd..bf3c2fe8f528 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3074,7 +3074,8 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
 	clear_vm_uninitialized_flag(area);
 
 	size = PAGE_ALIGN(size);
-	kmemleak_vmalloc(area, size, gfp_mask);
+	if (!(vm_flags & VM_DEFER_KMEMLEAK))
+		kmemleak_vmalloc(area, size, gfp_mask);
 
 	return addr;
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v4] mm: Defer kmemleak object creation of module_alloc()
  2021-11-25  8:03 [PATCH v4] mm: Defer kmemleak object creation of module_alloc() Kefeng Wang
@ 2021-11-25 16:07 ` Andrey Konovalov
  0 siblings, 0 replies; 2+ messages in thread
From: Andrey Konovalov @ 2021-11-25 16:07 UTC (permalink / raw)
  To: Kefeng Wang
  Cc: Andrey Ryabinin, Dmitry Vyukov, Andrew Morton, Linux ARM, LKML,
	linux-s390, kasan-dev, Linux Memory Management List,
	Catalin Marinas, Will Deacon, Heiko Carstens, Vasily Gorbik,
	Christian Borntraeger, Alexander Gordeev, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Dave Hansen, Alexander Potapenko,
	Yongqiang Liu

On Thu, Nov 25, 2021 at 8:52 AM 'Kefeng Wang' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> Yongqiang reports a kmemleak panic when module insmod/rmmod
> with KASAN enabled(without KASAN_VMALLOC) on x86[1].
>
> When the module area allocates memory, it's kmemleak_object
> is created successfully, but the KASAN shadow memory of module
> allocation is not ready, so when kmemleak scan the module's
> pointer, it will panic due to no shadow memory with KASAN check.
>
> module_alloc
>   __vmalloc_node_range
>     kmemleak_vmalloc
>                                 kmemleak_scan
>                                   update_checksum
>   kasan_module_alloc
>     kmemleak_ignore
>
> Note, there is no problem if KASAN_VMALLOC enabled, the modules
> area entire shadow memory is preallocated. Thus, the bug only
> exits on ARCH which supports dynamic allocation of module area
> per module load, for now, only x86/arm64/s390 are involved.
>
>
> Add a VM_DEFER_KMEMLEAK flags, defer vmalloc'ed object register
> of kmemleak in module_alloc() to fix this issue.
>
> [1] https://lore.kernel.org/all/6d41e2b9-4692-5ec4-b1cd-cbe29ae89739@huawei.com/
>
> Fixes: 793213a82de4 ("s390/kasan: dynamic shadow mem allocation for modules")
> Fixes: 39d114ddc682 ("arm64: add KASAN support")
> Fixes: bebf56a1b176 ("kasan: enable instrumentation of global variables")
> Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> V4:
> - add fix tag
> - fix missing change about VM_DELAY_KMEMLEAK
> v3:
> - update changelog to add more explanation
> - use DEFER instead of DELAY sugguested by Catalin.
> v2:
> - fix type error on changelog and kasan_module_alloc()
>
>  arch/arm64/kernel/module.c | 4 ++--
>  arch/s390/kernel/module.c  | 5 +++--
>  arch/x86/kernel/module.c   | 7 ++++---
>  include/linux/kasan.h      | 4 ++--
>  include/linux/vmalloc.h    | 7 +++++++
>  mm/kasan/shadow.c          | 9 +++++++--
>  mm/vmalloc.c               | 3 ++-
>  7 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index b5ec010c481f..309a27553c87 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -36,7 +36,7 @@ void *module_alloc(unsigned long size)
>                 module_alloc_end = MODULES_END;
>
>         p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> -                               module_alloc_end, gfp_mask, PAGE_KERNEL, 0,
> +                               module_alloc_end, gfp_mask, PAGE_KERNEL, VM_DEFER_KMEMLEAK,
>                                 NUMA_NO_NODE, __builtin_return_address(0));
>
>         if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
> @@ -58,7 +58,7 @@ void *module_alloc(unsigned long size)
>                                 PAGE_KERNEL, 0, NUMA_NO_NODE,
>                                 __builtin_return_address(0));
>
> -       if (p && (kasan_module_alloc(p, size) < 0)) {
> +       if (p && (kasan_module_alloc(p, size, gfp_mask) < 0)) {
>                 vfree(p);
>                 return NULL;
>         }
> diff --git a/arch/s390/kernel/module.c b/arch/s390/kernel/module.c
> index b01ba460b7ca..d52d85367bf7 100644
> --- a/arch/s390/kernel/module.c
> +++ b/arch/s390/kernel/module.c
> @@ -37,14 +37,15 @@
>
>  void *module_alloc(unsigned long size)
>  {
> +       gfp_t gfp_mask = GFP_KERNEL;
>         void *p;
>
>         if (PAGE_ALIGN(size) > MODULES_LEN)
>                 return NULL;
>         p = __vmalloc_node_range(size, MODULE_ALIGN, MODULES_VADDR, MODULES_END,
> -                                GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
> +                                gfp_mask, PAGE_KERNEL_EXEC, VM_DEFER_KMEMLEAK, NUMA_NO_NODE,
>                                  __builtin_return_address(0));
> -       if (p && (kasan_module_alloc(p, size) < 0)) {
> +       if (p && (kasan_module_alloc(p, size, gfp_mask) < 0)) {
>                 vfree(p);
>                 return NULL;
>         }
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index 169fb6f4cd2e..95fa745e310a 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -67,6 +67,7 @@ static unsigned long int get_module_load_offset(void)
>
>  void *module_alloc(unsigned long size)
>  {
> +       gfp_t gfp_mask = GFP_KERNEL;
>         void *p;
>
>         if (PAGE_ALIGN(size) > MODULES_LEN)
> @@ -74,10 +75,10 @@ void *module_alloc(unsigned long size)
>
>         p = __vmalloc_node_range(size, MODULE_ALIGN,
>                                     MODULES_VADDR + get_module_load_offset(),
> -                                   MODULES_END, GFP_KERNEL,
> -                                   PAGE_KERNEL, 0, NUMA_NO_NODE,
> +                                   MODULES_END, gfp_mask,
> +                                   PAGE_KERNEL, VM_DEFER_KMEMLEAK, NUMA_NO_NODE,
>                                     __builtin_return_address(0));
> -       if (p && (kasan_module_alloc(p, size) < 0)) {
> +       if (p && (kasan_module_alloc(p, size, gfp_mask) < 0)) {
>                 vfree(p);
>                 return NULL;
>         }
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index d8783b682669..89c99e5e67de 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -474,12 +474,12 @@ static inline void kasan_populate_early_vm_area_shadow(void *start,
>   * allocations with real shadow memory. With KASAN vmalloc, the special
>   * case is unnecessary, as the work is handled in the generic case.
>   */
> -int kasan_module_alloc(void *addr, size_t size);
> +int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask);
>  void kasan_free_shadow(const struct vm_struct *vm);
>
>  #else /* (CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS) && !CONFIG_KASAN_VMALLOC */
>
> -static inline int kasan_module_alloc(void *addr, size_t size) { return 0; }
> +static inline int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask) { return 0; }
>  static inline void kasan_free_shadow(const struct vm_struct *vm) {}
>
>  #endif /* (CONFIG_KASAN_GENERIC || CONFIG_KASAN_SW_TAGS) && !CONFIG_KASAN_VMALLOC */
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 6e022cc712e6..506fc6e6a126 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -28,6 +28,13 @@ struct notifier_block;               /* in notifier.h */
>  #define VM_MAP_PUT_PAGES       0x00000200      /* put pages and free array in vfree */
>  #define VM_NO_HUGE_VMAP                0x00000400      /* force PAGE_SIZE pte mapping */
>
> +#if defined(CONFIG_KASAN) && (defined(CONFIG_KASAN_GENERIC) || \
> +       defined(CONFIG_KASAN_SW_TAGS)) && !defined(CONFIG_KASAN_VMALLOC)
> +#define VM_DEFER_KMEMLEAK      0x00000800      /* defer kmemleak object creation */
> +#else
> +#define VM_DEFER_KMEMLEAK      0
> +#endif

No need for CONFIG_KASAN check: CONFIG_KASAN_GENERIC ||
CONFIG_KASAN_SW_TAGS implies CONFIG_KASAN.



> +
>  /*
>   * VM_KASAN is used slightly differently depending on CONFIG_KASAN_VMALLOC.
>   *
> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
> index 4a4929b29a23..2ade2f484562 100644
> --- a/mm/kasan/shadow.c
> +++ b/mm/kasan/shadow.c
> @@ -498,7 +498,7 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
>
>  #else /* CONFIG_KASAN_VMALLOC */
>
> -int kasan_module_alloc(void *addr, size_t size)
> +int kasan_module_alloc(void *addr, size_t size, gfp_t gfp_mask)
>  {
>         void *ret;
>         size_t scaled_size;
> @@ -520,9 +520,14 @@ int kasan_module_alloc(void *addr, size_t size)
>                         __builtin_return_address(0));
>
>         if (ret) {
> +               struct vm_struct *vm = find_vm_area(addr);
>                 __memset(ret, KASAN_SHADOW_INIT, shadow_size);
> -               find_vm_area(addr)->flags |= VM_KASAN;
> +               vm->flags |= VM_KASAN;
>                 kmemleak_ignore(ret);
> +
> +               if (vm->flags & VM_DEFER_KMEMLEAK)
> +                       kmemleak_vmalloc(vm, size, gfp_mask);
> +
>                 return 0;
>         }
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d2a00ad4e1dd..bf3c2fe8f528 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3074,7 +3074,8 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
>         clear_vm_uninitialized_flag(area);
>
>         size = PAGE_ALIGN(size);
> -       kmemleak_vmalloc(area, size, gfp_mask);
> +       if (!(vm_flags & VM_DEFER_KMEMLEAK))
> +               kmemleak_vmalloc(area, size, gfp_mask);
>
>         return addr;
>
> --
> 2.26.2
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20211125080307.27225-1-wangkefeng.wang%40huawei.com.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-11-25 16:19 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-25  8:03 [PATCH v4] mm: Defer kmemleak object creation of module_alloc() Kefeng Wang
2021-11-25 16:07 ` Andrey Konovalov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).