diff mbox series

[v2] arm: kasan: clear stale stack poison

Message ID 20240410073044.23294-1-boy.wu@mediatek.com (mailing list archive)
State New
Headers show
Series [v2] arm: kasan: clear stale stack poison | expand

Commit Message

Boy Wu (吳勃誼) April 10, 2024, 7:30 a.m. UTC
From: Boy Wu <boy.wu@mediatek.com>

We found below OOB crash:

[   33.452494] ==================================================================
[   33.453513] BUG: KASAN: stack-out-of-bounds in refresh_cpu_vm_stats.constprop.0+0xcc/0x2ec
[   33.454660] Write of size 164 at addr c1d03d30 by task swapper/0/0
[   33.455515]
[   33.455767] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O       6.1.25-mainline #1
[   33.456880] Hardware name: Generic DT based system
[   33.457555]  unwind_backtrace from show_stack+0x18/0x1c
[   33.458326]  show_stack from dump_stack_lvl+0x40/0x4c
[   33.459072]  dump_stack_lvl from print_report+0x158/0x4a4
[   33.459863]  print_report from kasan_report+0x9c/0x148
[   33.460616]  kasan_report from kasan_check_range+0x94/0x1a0
[   33.461424]  kasan_check_range from memset+0x20/0x3c
[   33.462157]  memset from refresh_cpu_vm_stats.constprop.0+0xcc/0x2ec
[   33.463064]  refresh_cpu_vm_stats.constprop.0 from tick_nohz_idle_stop_tick+0x180/0x53c
[   33.464181]  tick_nohz_idle_stop_tick from do_idle+0x264/0x354
[   33.465029]  do_idle from cpu_startup_entry+0x20/0x24
[   33.465769]  cpu_startup_entry from rest_init+0xf0/0xf4
[   33.466528]  rest_init from arch_post_acpi_subsys_init+0x0/0x18
[   33.467397]
[   33.467644] The buggy address belongs to stack of task swapper/0/0
[   33.468493]  and is located at offset 112 in frame:
[   33.469172]  refresh_cpu_vm_stats.constprop.0+0x0/0x2ec
[   33.469917]
[   33.470165] This frame has 2 objects:
[   33.470696]  [32, 76) 'global_zone_diff'
[   33.470729]  [112, 276) 'global_node_diff'
[   33.471294]
[   33.472095] The buggy address belongs to the physical page:
[   33.472862] page:3cd72da8 refcount:1 mapcount:0 mapping:00000000 index:0x0 pfn:0x41d03
[   33.473944] flags: 0x1000(reserved|zone=0)
[   33.474565] raw: 00001000 ed741470 ed741470 00000000 00000000 00000000 ffffffff 00000001
[   33.475656] raw: 00000000
[   33.476050] page dumped because: kasan: bad access detected
[   33.476816]
[   33.477061] Memory state around the buggy address:
[   33.477732]  c1d03c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   33.478630]  c1d03c80: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00
[   33.479526] >c1d03d00: 00 04 f2 f2 f2 f2 00 00 00 00 00 00 f1 f1 f1 f1
[   33.480415]                                                ^
[   33.481195]  c1d03d80: 00 00 00 00 00 00 00 00 00 00 04 f3 f3 f3 f3 f3
[   33.482088]  c1d03e00: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
[   33.482978] ==================================================================

We find the root cause of this OOB is that arm does not clear stale stack
poison in the case of cpuidle.

This patch refer to arch/arm64/kernel/sleep.S to resolve this issue.

From cited commit [1] that explain the problem

Functions which the compiler has instrumented for KASAN place poison on
the stack shadow upon entry and remove this poison prior to returning.

In the case of cpuidle, CPUs exit the kernel a number of levels deep in
C code.  Any instrumented functions on this critical path will leave
portions of the stack shadow poisoned.

If CPUs lose context and return to the kernel via a cold path, we
restore a prior context saved in __cpu_suspend_enter are forgotten, and
we never remove the poison they placed in the stack shadow area by
functions calls between this and the actual exit of the kernel.

Thus, (depending on stackframe layout) subsequent calls to instrumented
functions may hit this stale poison, resulting in (spurious) KASAN
splats to the console.

To avoid this, clear any stale poison from the idle thread for a CPU
prior to bringing a CPU online.

From cited commit [2]

Extend to check for CONFIG_KASAN_STACK

[1] commit 0d97e6d8024c ("arm64: kasan: clear stale stack poison")
[2] commit d56a9ef84bd0 ("kasan, arm64: unpoison stack only with CONFIG_KASAN_STACK")

Signed-off-by: Boy Wu <boy.wu@mediatek.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
v2 - Add commit message that reviewer mention
---
 arch/arm/kernel/sleep.S | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Linus Walleij April 12, 2024, 8:37 a.m. UTC | #1
On Wed, Apr 10, 2024 at 9:31 AM boy.wu <boy.wu@mediatek.com> wrote:

> From: Boy Wu <boy.wu@mediatek.com>
>
> We found below OOB crash:

Thanks for digging in!

Pleas put this patch into Russell's patch tracker so he can apply it:
https://www.armlinux.org.uk/developer/patches/

Yours,
Linus Walleij
Russell King (Oracle) April 28, 2024, 2:45 p.m. UTC | #2
On Fri, Apr 12, 2024 at 10:37:06AM +0200, Linus Walleij wrote:
> On Wed, Apr 10, 2024 at 9:31 AM boy.wu <boy.wu@mediatek.com> wrote:
> 
> > From: Boy Wu <boy.wu@mediatek.com>
> >
> > We found below OOB crash:
> 
> Thanks for digging in!
> 
> Pleas put this patch into Russell's patch tracker so he can apply it:
> https://www.armlinux.org.uk/developer/patches/

Is this a bug fix? If so, having a Fixes: tag would be nice...
Boy Wu (吳勃誼) April 29, 2024, 7:51 a.m. UTC | #3
On Sun, 2024-04-28 at 15:45 +0100, Russell King (Oracle) wrote:
>  On Fri, Apr 12, 2024 at 10:37:06AM +0200, Linus Walleij wrote:
> > On Wed, Apr 10, 2024 at 9:31 AM boy.wu <boy.wu@mediatek.com> wrote:
> > 
> > > From: Boy Wu <boy.wu@mediatek.com>
> > >
> > > We found below OOB crash:
> > 
> > Thanks for digging in!
> > 
> > Pleas put this patch into Russell's patch tracker so he can apply
> it:
> > https://www.armlinux.org.uk/developer/patches/
> 
> Is this a bug fix? If so, having a Fixes: tag would be nice...
> 

This is a patch for cpuidle flow when KASAN enable, that is in ARM64
but not in ARM, so add to ARM.

The reference commits did not mention fix any commits.
[1] commit 0d97e6d8024c ("arm64: kasan: clear stale stack poison")
[2] commit d56a9ef84bd0 ("kasan, arm64: unpoison stack only with
CONFIG_KASAN_STACK")
Russell King (Oracle) April 29, 2024, 12:42 p.m. UTC | #4
On Mon, Apr 29, 2024 at 07:51:49AM +0000, Boy Wu (吳勃誼) wrote:
> On Sun, 2024-04-28 at 15:45 +0100, Russell King (Oracle) wrote:
> >  On Fri, Apr 12, 2024 at 10:37:06AM +0200, Linus Walleij wrote:
> > > On Wed, Apr 10, 2024 at 9:31 AM boy.wu <boy.wu@mediatek.com> wrote:
> > > 
> > > > From: Boy Wu <boy.wu@mediatek.com>
> > > >
> > > > We found below OOB crash:
> > > 
> > > Thanks for digging in!
> > > 
> > > Pleas put this patch into Russell's patch tracker so he can apply
> > it:
> > > https://www.armlinux.org.uk/developer/patches/
> > 
> > Is this a bug fix? If so, having a Fixes: tag would be nice...
> > 
> 
> This is a patch for cpuidle flow when KASAN enable, that is in ARM64
> but not in ARM, so add to ARM.
> 
> The reference commits did not mention fix any commits.
> [1] commit 0d97e6d8024c ("arm64: kasan: clear stale stack poison")
> [2] commit d56a9ef84bd0 ("kasan, arm64: unpoison stack only with
> CONFIG_KASAN_STACK")

These are not suitable for use as a Fixes: tag because these commits
refer to code in another part of the tree that has nothing to do with
the BUG() dump that is contained within your commit message.

I ask again... Is this a bug fix?

Is it a regression?

Is it something that used to work that no longer works?

When did it break?

Has it always been broken?

Has it been broken since KASAN was introduced on 32-bit ARM?

I'm not applying this commit until I get *proper* answers to these
questions so that I can work out whether this needs to go in -rc
or whether it waits until the next merge window... and whether it
needs a Fixes: tag or not.
Andrey Ryabinin April 29, 2024, 12:57 p.m. UTC | #5
On 4/29/24 14:42, Russell King (Oracle) wrote:
> On Mon, Apr 29, 2024 at 07:51:49AM +0000, Boy Wu (吳勃誼) wrote:
>> On Sun, 2024-04-28 at 15:45 +0100, Russell King (Oracle) wrote:
>>>  On Fri, Apr 12, 2024 at 10:37:06AM +0200, Linus Walleij wrote:
>>>> On Wed, Apr 10, 2024 at 9:31 AM boy.wu <boy.wu@mediatek.com> wrote:
>>>>
>>>>> From: Boy Wu <boy.wu@mediatek.com>
>>>>>
>>>>> We found below OOB crash:
>>>>
>>>> Thanks for digging in!
>>>>
>>>> Pleas put this patch into Russell's patch tracker so he can apply
>>> it:
>>>> https://www.armlinux.org.uk/developer/patches/
>>>
>>> Is this a bug fix? If so, having a Fixes: tag would be nice...
>>>
>>
>> This is a patch for cpuidle flow when KASAN enable, that is in ARM64
>> but not in ARM, so add to ARM.
>>
>> The reference commits did not mention fix any commits.
>> [1] commit 0d97e6d8024c ("arm64: kasan: clear stale stack poison")
>> [2] commit d56a9ef84bd0 ("kasan, arm64: unpoison stack only with
>> CONFIG_KASAN_STACK")
> 
> These are not suitable for use as a Fixes: tag because these commits
> refer to code in another part of the tree that has nothing to do with
> the BUG() dump that is contained within your commit message.
> 
> I ask again... Is this a bug fix?
> 
> Is it a regression?
> 
> Is it something that used to work that no longer works?
> 
> When did it break?
> 
> Has it always been broken?
> 
> Has it been broken since KASAN was introduced on 32-bit ARM?
> 

Yes, this is a bug fix and it has been broken since KASAN was introduced on 32-bit ARM.
So, I think this should be
	Fixes: 5615f69bc209 ("ARM: 9016/2: Initialize the mapping of KASan shadow memory")
Russell King (Oracle) April 29, 2024, 1:11 p.m. UTC | #6
On Mon, Apr 29, 2024 at 02:57:35PM +0200, Andrey Ryabinin wrote:
> On 4/29/24 14:42, Russell King (Oracle) wrote:
> > On Mon, Apr 29, 2024 at 07:51:49AM +0000, Boy Wu (吳勃誼) wrote:
> >> On Sun, 2024-04-28 at 15:45 +0100, Russell King (Oracle) wrote:
> >>>  On Fri, Apr 12, 2024 at 10:37:06AM +0200, Linus Walleij wrote:
> >>>> On Wed, Apr 10, 2024 at 9:31 AM boy.wu <boy.wu@mediatek.com> wrote:
> >>>>
> >>>>> From: Boy Wu <boy.wu@mediatek.com>
> >>>>>
> >>>>> We found below OOB crash:
> >>>>
> >>>> Thanks for digging in!
> >>>>
> >>>> Pleas put this patch into Russell's patch tracker so he can apply
> >>> it:
> >>>> https://www.armlinux.org.uk/developer/patches/
> >>>
> >>> Is this a bug fix? If so, having a Fixes: tag would be nice...
> >>>
> >>
> >> This is a patch for cpuidle flow when KASAN enable, that is in ARM64
> >> but not in ARM, so add to ARM.
> >>
> >> The reference commits did not mention fix any commits.
> >> [1] commit 0d97e6d8024c ("arm64: kasan: clear stale stack poison")
> >> [2] commit d56a9ef84bd0 ("kasan, arm64: unpoison stack only with
> >> CONFIG_KASAN_STACK")
> > 
> > These are not suitable for use as a Fixes: tag because these commits
> > refer to code in another part of the tree that has nothing to do with
> > the BUG() dump that is contained within your commit message.
> > 
> > I ask again... Is this a bug fix?
> > 
> > Is it a regression?
> > 
> > Is it something that used to work that no longer works?
> > 
> > When did it break?
> > 
> > Has it always been broken?
> > 
> > Has it been broken since KASAN was introduced on 32-bit ARM?
> > 
> 
> Yes, this is a bug fix and it has been broken since KASAN was introduced on 32-bit ARM.
> So, I think this should be
> 	Fixes: 5615f69bc209 ("ARM: 9016/2: Initialize the mapping of KASan shadow memory")

Brilliant, thanks! Now merged.
diff mbox series

Patch

diff --git a/arch/arm/kernel/sleep.S b/arch/arm/kernel/sleep.S
index a86a1d4f3461..93afd1005b43 100644
--- a/arch/arm/kernel/sleep.S
+++ b/arch/arm/kernel/sleep.S
@@ -127,6 +127,10 @@  cpu_resume_after_mmu:
 	instr_sync
 #endif
 	bl	cpu_init		@ restore the und/abt/irq banked regs
+#if defined(CONFIG_KASAN) && defined(CONFIG_KASAN_STACK)
+	mov	r0, sp
+	bl	kasan_unpoison_task_stack_below
+#endif
 	mov	r0, #0			@ return zero on success
 	ldmfd	sp!, {r4 - r11, pc}
 ENDPROC(cpu_resume_after_mmu)