diff mbox series

mm: kmemleak: take a full lowmem check in kmemleak_*_phys()

Message ID 20220413122925.33856-1-patrick.wang.shcn@gmail.com (mailing list archive)
State New
Headers show
Series mm: kmemleak: take a full lowmem check in kmemleak_*_phys() | expand

Commit Message

patrick wang April 13, 2022, 12:29 p.m. UTC 
The kmemleak_*_phys() apis do not check the address for lowmem's min
boundary, while the caller may pass an address below lowmem, which
will trigger an oops:

# echo scan > /sys/kernel/debug/kmemleak
[   54.888353] Unable to handle kernel paging request at virtual address ff5fffffffe00000
[   54.888932] Oops [#1]
[   54.889102] Modules linked in:
[   54.889326] CPU: 2 PID: 134 Comm: bash Not tainted 5.18.0-rc1-next-20220407 #33
[   54.889620] Hardware name: riscv-virtio,qemu (DT)
[   54.889901] epc : scan_block+0x74/0x15c
[   54.890215]  ra : scan_block+0x72/0x15c
[   54.890390] epc : ffffffff801e5806 ra : ffffffff801e5804 sp : ff200000104abc30
[   54.890607]  gp : ffffffff815cd4e8 tp : ff60000004cfa340 t0 : 0000000000000200
[   54.890835]  t1 : 00aaaaaac23954cc t2 : 00000000000003ff s0 : ff200000104abc90
[   54.891024]  s1 : ffffffff81b0ff28 a0 : 0000000000000000 a1 : ff5fffffffe01000
[   54.891201]  a2 : ffffffff81b0ff28 a3 : 0000000000000002 a4 : 0000000000000001
[   54.891377]  a5 : 0000000000000000 a6 : ff200000104abd7c a7 : 0000000000000005
[   54.891552]  s2 : ff5fffffffe00ff9 s3 : ffffffff815cd998 s4 : ffffffff815d0e90
[   54.891727]  s5 : ffffffff81b0ff28 s6 : 0000000000000020 s7 : ffffffff815d0eb0
[   54.891903]  s8 : ffffffffffffffff s9 : ff5fffffffe00000 s10: ff5fffffffe01000
[   54.892078]  s11: 0000000000000022 t3 : 00ffffffaa17db4c t4 : 000000000000000f
[   54.892271]  t5 : 0000000000000001 t6 : 0000000000000000
[   54.892408] status: 0000000000000100 badaddr: ff5fffffffe00000 cause: 000000000000000d
[   54.892643] [<ffffffff801e5a1c>] scan_gray_list+0x12e/0x1a6
[   54.892824] [<ffffffff801e5d3e>] kmemleak_scan+0x2aa/0x57e
[   54.892961] [<ffffffff801e633c>] kmemleak_write+0x32a/0x40c
[   54.893096] [<ffffffff803915ac>] full_proxy_write+0x56/0x82
[   54.893235] [<ffffffff801ef456>] vfs_write+0xa6/0x2a6
[   54.893362] [<ffffffff801ef880>] ksys_write+0x6c/0xe2
[   54.893487] [<ffffffff801ef918>] sys_write+0x22/0x2a
[   54.893609] [<ffffffff8000397c>] ret_from_syscall+0x0/0x2
[   54.894183] ---[ end trace 0000000000000000 ]---

So check the address for lowmem's min boundary.

Signed-off-by: Patrick Wang <patrick.wang.shcn@gmail.com>
---
 mm/kmemleak.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Andrew Morton April 13, 2022, 7:28 p.m. UTC | #1 
On Wed, 13 Apr 2022 20:29:25 +0800 Patrick Wang <patrick.wang.shcn@gmail.com> wrote:

> The kmemleak_*_phys() apis do not check the address for lowmem's min
> boundary, while the caller may pass an address below lowmem, which
> will trigger an oops:

Do you believe this fix should be backported into -stable kernels?
patrick wang April 14, 2022, 2:40 a.m. UTC | #2 
On Thu, Apr 14, 2022 at 3:28 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 13 Apr 2022 20:29:25 +0800 Patrick Wang <patrick.wang.shcn@gmail.com> wrote:
>
> > The kmemleak_*_phys() apis do not check the address for lowmem's min
> > boundary, while the caller may pass an address below lowmem, which
> > will trigger an oops:
>
> Do you believe this fix should be backported into -stable kernels?

Yes, I think so. The callers may not quite know the actual address they
pass(e.g. from devicetree). So the kmemleak_*_phys() apis should guarantee
the address they finally use is in lowmem range.
Catalin Marinas April 14, 2022, 5:26 p.m. UTC | #3 
On Wed, Apr 13, 2022 at 08:29:25PM +0800, Patrick Wang wrote:
> The kmemleak_*_phys() apis do not check the address for lowmem's min
> boundary, while the caller may pass an address below lowmem, which
> will trigger an oops:
> 
> # echo scan > /sys/kernel/debug/kmemleak
> [   54.888353] Unable to handle kernel paging request at virtual address ff5fffffffe00000
> [   54.888932] Oops [#1]
> [   54.889102] Modules linked in:
> [   54.889326] CPU: 2 PID: 134 Comm: bash Not tainted 5.18.0-rc1-next-20220407 #33
> [   54.889620] Hardware name: riscv-virtio,qemu (DT)
> [   54.889901] epc : scan_block+0x74/0x15c
> [   54.890215]  ra : scan_block+0x72/0x15c
> [   54.890390] epc : ffffffff801e5806 ra : ffffffff801e5804 sp : ff200000104abc30
> [   54.890607]  gp : ffffffff815cd4e8 tp : ff60000004cfa340 t0 : 0000000000000200
> [   54.890835]  t1 : 00aaaaaac23954cc t2 : 00000000000003ff s0 : ff200000104abc90
> [   54.891024]  s1 : ffffffff81b0ff28 a0 : 0000000000000000 a1 : ff5fffffffe01000
> [   54.891201]  a2 : ffffffff81b0ff28 a3 : 0000000000000002 a4 : 0000000000000001
> [   54.891377]  a5 : 0000000000000000 a6 : ff200000104abd7c a7 : 0000000000000005
> [   54.891552]  s2 : ff5fffffffe00ff9 s3 : ffffffff815cd998 s4 : ffffffff815d0e90
> [   54.891727]  s5 : ffffffff81b0ff28 s6 : 0000000000000020 s7 : ffffffff815d0eb0
> [   54.891903]  s8 : ffffffffffffffff s9 : ff5fffffffe00000 s10: ff5fffffffe01000
> [   54.892078]  s11: 0000000000000022 t3 : 00ffffffaa17db4c t4 : 000000000000000f
> [   54.892271]  t5 : 0000000000000001 t6 : 0000000000000000
> [   54.892408] status: 0000000000000100 badaddr: ff5fffffffe00000 cause: 000000000000000d
> [   54.892643] [<ffffffff801e5a1c>] scan_gray_list+0x12e/0x1a6
> [   54.892824] [<ffffffff801e5d3e>] kmemleak_scan+0x2aa/0x57e
> [   54.892961] [<ffffffff801e633c>] kmemleak_write+0x32a/0x40c
> [   54.893096] [<ffffffff803915ac>] full_proxy_write+0x56/0x82
> [   54.893235] [<ffffffff801ef456>] vfs_write+0xa6/0x2a6
> [   54.893362] [<ffffffff801ef880>] ksys_write+0x6c/0xe2
> [   54.893487] [<ffffffff801ef918>] sys_write+0x22/0x2a
> [   54.893609] [<ffffffff8000397c>] ret_from_syscall+0x0/0x2

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Yee Lee (李建誼) May 27, 2022, 4 a.m. UTC | #4 
In arm64, some false leaks happen since memblock allocates memory 
eariler than that the pfn boundary (max_low_pfn/min_low_pfn) is
initialized. I would relax the checking condidtion when the boundary
is not ready. 

Please refer to the patch: https://lkml.org/lkml/2022/5/26/971
   
Not sure this work for the other archs like RISCV and for your
situation. Please help to review it.  

On Wed, 2022-04-13 at 20:29 +0800, Patrick Wang wrote:
> The kmemleak_*_phys() apis do not check the address for lowmem's min
> boundary, while the caller may pass an address below lowmem, which
> will trigger an oops:
> 
> # echo scan > /sys/kernel/debug/kmemleak
> [   54.888353] Unable to handle kernel paging request at virtual
> address ff5fffffffe00000
> [   54.888932] Oops [#1]
> [   54.889102] Modules linked in:
> [   54.889326] CPU: 2 PID: 134 Comm: bash Not tainted 5.18.0-rc1-
> next-20220407 #33
> [   54.889620] Hardware name: riscv-virtio,qemu (DT)
> [   54.889901] epc : scan_block+0x74/0x15c
> [   54.890215]  ra : scan_block+0x72/0x15c
> [   54.890390] epc : ffffffff801e5806 ra : ffffffff801e5804 sp :
> ff200000104abc30
> [   54.890607]  gp : ffffffff815cd4e8 tp : ff60000004cfa340 t0 :
> 0000000000000200
> [   54.890835]  t1 : 00aaaaaac23954cc t2 : 00000000000003ff s0 :
> ff200000104abc90
> [   54.891024]  s1 : ffffffff81b0ff28 a0 : 0000000000000000 a1 :
> ff5fffffffe01000
> [   54.891201]  a2 : ffffffff81b0ff28 a3 : 0000000000000002 a4 :
> 0000000000000001
> [   54.891377]  a5 : 0000000000000000 a6 : ff200000104abd7c a7 :
> 0000000000000005
> [   54.891552]  s2 : ff5fffffffe00ff9 s3 : ffffffff815cd998 s4 :
> ffffffff815d0e90
> [   54.891727]  s5 : ffffffff81b0ff28 s6 : 0000000000000020 s7 :
> ffffffff815d0eb0
> [   54.891903]  s8 : ffffffffffffffff s9 : ff5fffffffe00000 s10:
> ff5fffffffe01000
> [   54.892078]  s11: 0000000000000022 t3 : 00ffffffaa17db4c t4 :
> 000000000000000f
> [   54.892271]  t5 : 0000000000000001 t6 : 0000000000000000
> [   54.892408] status: 0000000000000100 badaddr: ff5fffffffe00000
> cause: 000000000000000d
> [   54.892643] [<ffffffff801e5a1c>] scan_gray_list+0x12e/0x1a6
> [   54.892824] [<ffffffff801e5d3e>] kmemleak_scan+0x2aa/0x57e
> [   54.892961] [<ffffffff801e633c>] kmemleak_write+0x32a/0x40c
> [   54.893096] [<ffffffff803915ac>] full_proxy_write+0x56/0x82
> [   54.893235] [<ffffffff801ef456>] vfs_write+0xa6/0x2a6
> [   54.893362] [<ffffffff801ef880>] ksys_write+0x6c/0xe2
> [   54.893487] [<ffffffff801ef918>] sys_write+0x22/0x2a
> [   54.893609] [<ffffffff8000397c>] ret_from_syscall+0x0/0x2
> [   54.894183] ---[ end trace 0000000000000000 ]---
> 
> So check the address for lowmem's min boundary.
> 
> Signed-off-by: Patrick Wang <patrick.wang.shcn@gmail.com>
> ---
>  mm/kmemleak.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index acd7cbb82..a182f5dda 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -1132,7 +1132,7 @@ EXPORT_SYMBOL(kmemleak_no_scan);
>  void __ref kmemleak_alloc_phys(phys_addr_t phys, size_t size, int
> min_count,
>  			       gfp_t gfp)
>  {
> -	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) <
> max_low_pfn)
> +	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) <
> max_low_pfn)
>  		kmemleak_alloc(__va(phys), size, min_count, gfp);
>  }
>  EXPORT_SYMBOL(kmemleak_alloc_phys);
> @@ -1146,7 +1146,7 @@ EXPORT_SYMBOL(kmemleak_alloc_phys);
>   */
>  void __ref kmemleak_free_part_phys(phys_addr_t phys, size_t size)
>  {
> -	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) <
> max_low_pfn)
> +	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) <
> max_low_pfn)
>  		kmemleak_free_part(__va(phys), size);
>  }
>  EXPORT_SYMBOL(kmemleak_free_part_phys);
> @@ -1158,7 +1158,7 @@ EXPORT_SYMBOL(kmemleak_free_part_phys);
>   */
>  void __ref kmemleak_not_leak_phys(phys_addr_t phys)
>  {
> -	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) <
> max_low_pfn)
> +	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) <
> max_low_pfn)
>  		kmemleak_not_leak(__va(phys));
>  }
>  EXPORT_SYMBOL(kmemleak_not_leak_phys);
> @@ -1170,7 +1170,7 @@ EXPORT_SYMBOL(kmemleak_not_leak_phys);
>   */
>  void __ref kmemleak_ignore_phys(phys_addr_t phys)
>  {
> -	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) <
> max_low_pfn)
> +	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) <
> max_low_pfn)
>  		kmemleak_ignore(__va(phys));
>  }
>  EXPORT_SYMBOL(kmemleak_ignore_phys);
diff mbox series

Patch

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index acd7cbb82..a182f5dda 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1132,7 +1132,7 @@  EXPORT_SYMBOL(kmemleak_no_scan);
 void __ref kmemleak_alloc_phys(phys_addr_t phys, size_t size, int min_count,
 			       gfp_t gfp)
 {
-	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) < max_low_pfn)
+	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) < max_low_pfn)
 		kmemleak_alloc(__va(phys), size, min_count, gfp);
 }
 EXPORT_SYMBOL(kmemleak_alloc_phys);
@@ -1146,7 +1146,7 @@  EXPORT_SYMBOL(kmemleak_alloc_phys);
  */
 void __ref kmemleak_free_part_phys(phys_addr_t phys, size_t size)
 {
-	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) < max_low_pfn)
+	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) < max_low_pfn)
 		kmemleak_free_part(__va(phys), size);
 }
 EXPORT_SYMBOL(kmemleak_free_part_phys);
@@ -1158,7 +1158,7 @@  EXPORT_SYMBOL(kmemleak_free_part_phys);
  */
 void __ref kmemleak_not_leak_phys(phys_addr_t phys)
 {
-	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) < max_low_pfn)
+	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) < max_low_pfn)
 		kmemleak_not_leak(__va(phys));
 }
 EXPORT_SYMBOL(kmemleak_not_leak_phys);
@@ -1170,7 +1170,7 @@  EXPORT_SYMBOL(kmemleak_not_leak_phys);
  */
 void __ref kmemleak_ignore_phys(phys_addr_t phys)
 {
-	if (!IS_ENABLED(CONFIG_HIGHMEM) || PHYS_PFN(phys) < max_low_pfn)
+	if (PHYS_PFN(phys) >= min_low_pfn && PHYS_PFN(phys) < max_low_pfn)
 		kmemleak_ignore(__va(phys));
 }
 EXPORT_SYMBOL(kmemleak_ignore_phys);