2024-04-10 07:31:33

by Boy Wu (吳勃誼)

[permalink] [raw]
Subject: [PATCH v2] arm: kasan: clear stale stack poison

From: Boy Wu <[email protected]>

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 <[email protected]>
Reviewed-by: Mark Rutland <[email protected]>
Acked-by: Andrey Ryabinin <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
---
v2 - Add commit message that reviewer mention
---
arch/arm/kernel/sleep.S | 4 ++++
1 file changed, 4 insertions(+)

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)
--
2.18.0



2024-04-12 08:49:35

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2] arm: kasan: clear stale stack poison

On Wed, Apr 10, 2024 at 9:31 AM boy.wu <[email protected]> wrote:

> From: Boy Wu <[email protected]>
>
> 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

2024-04-28 14:45:58

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2] arm: kasan: clear stale stack poison

On Fri, Apr 12, 2024 at 10:37:06AM +0200, Linus Walleij wrote:
> On Wed, Apr 10, 2024 at 9:31 AM boy.wu <[email protected]> wrote:
>
> > From: Boy Wu <[email protected]>
> >
> > 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...

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-04-29 07:52:35

by Boy Wu (吳勃誼)

[permalink] [raw]
Subject: Re: [PATCH v2] arm: kasan: clear stale stack poison

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 <[email protected]> wrote:
> >
> > > From: Boy Wu <[email protected]>
> > >
> > > 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")

2024-04-29 12:43:39

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2] arm: kasan: clear stale stack poison

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 <[email protected]> wrote:
> > >
> > > > From: Boy Wu <[email protected]>
> > > >
> > > > 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.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-04-29 13:01:51

by Andrey Ryabinin

[permalink] [raw]
Subject: Re: [PATCH v2] arm: kasan: clear stale stack poison



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 <[email protected]> wrote:
>>>>
>>>>> From: Boy Wu <[email protected]>
>>>>>
>>>>> 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")

2024-04-29 13:59:03

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2] arm: kasan: clear stale stack poison

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 <[email protected]> wrote:
> >>>>
> >>>>> From: Boy Wu <[email protected]>
> >>>>>
> >>>>> 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.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!