ARM: mm: fix stack corruption when CONFIG_ARM_PV_FIXUP=y

Commit Message

Zhi-zhou Zhang Sept. 7, 2023, 2:33 p.m. UTC

From: Zhizhou Zhang <[e-mail]>

flush_cache_all() save registers to stack at function entry.
If it's called after cache disabled, the data is written to
memory directly. So the following clean cache operation corrupted
registers saved by flush_cache_all(), including lr register.
calling flush_cache_all() before turn off cache fixed the problem.

Signed-off-by: Zhizhou Zhang <[e-mail]>
---
 arch/arm/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Linus Walleij Sept. 8, 2023, 12:58 p.m. UTC | #1

Hi Zhizhou,

wow a great patch! I'm surprised no-one has been hit by this before.
I guess we were lucky.

On Thu, Sep 7, 2023 at 4:33 PM Zhizhou Zhang <[e-mail]> wrote:

> From: Zhizhou Zhang <[e-mail]>
>
> flush_cache_all() save registers to stack at function entry.
> If it's called after cache disabled, the data is written to
> memory directly. So the following clean cache operation corrupted
> registers saved by flush_cache_all(), including lr register.
> calling flush_cache_all() before turn off cache fixed the problem.
>
> Signed-off-by: Zhizhou Zhang <[e-mail]>

Reviewed-by: Linus Walleij <[e-mail]>

I would also add
Cc: [e-mail]

Then please put this into Russell's patch tracker once review
is complete.

Yours,
Linus Walleij

Russell King (Oracle) Sept. 8, 2023, 1:50 p.m. UTC | #2

On Fri, Sep 08, 2023 at 02:58:49PM +0200, Linus Walleij wrote:
> Hi Zhizhou,
> 
> wow a great patch! I'm surprised no-one has been hit by this before.
> I guess we were lucky.
> 
> On Thu, Sep 7, 2023 at 4:33 PM Zhizhou Zhang <[e-mail]> wrote:
> 
> > From: Zhizhou Zhang <[e-mail]>
> >
> > flush_cache_all() save registers to stack at function entry.
> > If it's called after cache disabled, the data is written to
> > memory directly. So the following clean cache operation corrupted
> > registers saved by flush_cache_all(), including lr register.
> > calling flush_cache_all() before turn off cache fixed the problem.
> >
> > Signed-off-by: Zhizhou Zhang <[e-mail]>
> 
> Reviewed-by: Linus Walleij <[e-mail]>
> 
> I would also add
> Cc: [e-mail]
> 
> Then please put this into Russell's patch tracker once review
> is complete.

However, it makes a total nonsense of the comment, which explains
precisely why the flush_cache_all() is where it is. Moving it before
that comment means that the comment is now rediculous.

So, please don't put it in the patch system.

The patch certainly needs to be tested on TI Keystone which is the
primary user of this code.

Linus Walleij Sept. 8, 2023, 9 p.m. UTC | #3

On Fri, Sep 8, 2023 at 3:50 PM Russell King (Oracle)
<[e-mail]> wrote:

> However, it makes a total nonsense of the comment, which explains
> precisely why the flush_cache_all() is where it is. Moving it before
> that comment means that the comment is now rediculous.

Zhizhou, can you look over the comment placement?

> So, please don't put it in the patch system.
>
> The patch certainly needs to be tested on TI Keystone which is the
> primary user of this code.

Added Andrew Davis and Nishanth Menon to the thread:
can you folks review and test this for Keystone?

Yours,
Linus Walleij

Zhi-zhou Zhang Sept. 9, 2023, 8:23 a.m. UTC | #4

On Fri, Sep 08, 2023 at 11:00:31PM +0200, Linus Walleij wrote:
> On Fri, Sep 8, 2023 at 3:50 PM Russell King (Oracle)
> <[e-mail]> wrote:
> 
> > However, it makes a total nonsense of the comment, which explains
> > precisely why the flush_cache_all() is where it is. Moving it before
> > that comment means that the comment is now rediculous.
> 
> Zhizhou, can you look over the comment placement?

Linus, I found the bug on a cortex-a55 cpu with high address memory.
Since the lr is also corruptted, when flush_cache_all() is done, the
program continues at the next instruction after fixup_pv_table(). So
the disabling cache and flush_cache_all() is executed a secondary time.
Then this time lr is correct so the kernel may boot up as usual.

I read the comment carefully, I am not sure how "to ensure nothing is
prefetched into the caches" affects the system. My patch doesn't
prevent instrution prefetch though. But in my board everythings looks
good.

So I come up with a new fixup plan, that's keep the location of
flush_cache_all() with adding a flush stack cache before disabling
cache, the code is as follow, the fix is a bit ugly -- it makes
assumption stack grow towards low address and flush_cache_all() will
not occupy more than 32 bytes in the future. Comparing with move
flush_cache_all() before disabling cache, Which one do you prefer?
Thanks!

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 03fb0fe926f3..83a54c61a86b 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1640,6 +1640,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
 	unsigned long pa_pgd;
 	unsigned int cr, ttbcr;
 	long long offset;
+	void *stack;
 
 	if (!mdesc->pv_fixup)
 		return;
@@ -1675,7 +1676,14 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
 	/* Run the patch stub to update the constants */
 	fixup_pv_table(&__pv_table_begin,
 		(&__pv_table_end - &__pv_table_begin) << 2);
-	flush_cache_all();
+
+	/*
+	 * clean stack in cacheline that undering memory will be changed in
+	 * the following flush_cache_all(). assuming 32 bytes is enough for
+	 * flush_cache_all().
+	 */
+	stack = (void *) (current_stack_pointer - 32);
+	__cpuc_flush_dcache_area(stack, 32);
 
 	/*
 	 * We changing not only the virtual to physical mapping, but also
@@ -1691,6 +1699,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
 	asm("mrc p15, 0, %0, c2, c0, 2" : "=r" (ttbcr));
 	asm volatile("mcr p15, 0, %0, c2, c0, 2"
 		: : "r" (ttbcr & ~(3 << 8 | 3 << 10)));
+	flush_cache_all();
 
 	/*
 	 * Fixup the page tables - this must be in the idmap region as

> 
> > So, please don't put it in the patch system.
> >
> > The patch certainly needs to be tested on TI Keystone which is the
> > primary user of this code.
> 
> Added Andrew Davis and Nishanth Menon to the thread:
> can you folks review and test this for Keystone?
> 
> Yours,
> Linus Walleij

Nishanth Menon Sept. 11, 2023, 1:04 p.m. UTC | #5

On 23:00-20230908, Linus Walleij wrote:
> On Fri, Sep 8, 2023 at 3:50 PM Russell King (Oracle)
> <[e-mail]> wrote:
> 
> > However, it makes a total nonsense of the comment, which explains
> > precisely why the flush_cache_all() is where it is. Moving it before
> > that comment means that the comment is now rediculous.
> 
> Zhizhou, can you look over the comment placement?
> 
> > So, please don't put it in the patch system.
> >
> > The patch certainly needs to be tested on TI Keystone which is the
> > primary user of this code.
> 
> Added Andrew Davis and Nishanth Menon to the thread:
> can you folks review and test this for Keystone?

next-20230911 alone: (boots fine):
https://gist.github.com/nmenon/c097b4a7ce3971964a5a56a34b018c4d


With
https://lore.kernel.org/all/[e-mail]/
applied on top (fails to boot):
https://gist.github.com/nmenon/308cfeb84098f41d340cd0e61845a507

Andrew Davis Oct. 2, 2023, 2:17 p.m. UTC | #6

On 9/9/23 3:23 AM, Zhi-zhou Zhang wrote:
> On Fri, Sep 08, 2023 at 11:00:31PM +0200, Linus Walleij wrote:
>> On Fri, Sep 8, 2023 at 3:50 PM Russell King (Oracle)
>> <[e-mail]> wrote:
>>
>>> However, it makes a total nonsense of the comment, which explains
>>> precisely why the flush_cache_all() is where it is. Moving it before
>>> that comment means that the comment is now rediculous.
>>
>> Zhizhou, can you look over the comment placement?
> 
> Linus, I found the bug on a cortex-a55 cpu with high address memory.
> Since the lr is also corruptted, when flush_cache_all() is done, the
> program continues at the next instruction after fixup_pv_table(). So
> the disabling cache and flush_cache_all() is executed a secondary time.
> Then this time lr is correct so the kernel may boot up as usual.
> 
> I read the comment carefully, I am not sure how "to ensure nothing is
> prefetched into the caches" affects the system. My patch doesn't
> prevent instrution prefetch though. But in my board everythings looks
> good.
> 
> So I come up with a new fixup plan, that's keep the location of
> flush_cache_all() with adding a flush stack cache before disabling
> cache, the code is as follow, the fix is a bit ugly -- it makes
> assumption stack grow towards low address and flush_cache_all() will
> not occupy more than 32 bytes in the future. Comparing with move
> flush_cache_all() before disabling cache, Which one do you prefer?
> Thanks!
> 
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 03fb0fe926f3..83a54c61a86b 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1640,6 +1640,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
>   	unsigned long pa_pgd;
>   	unsigned int cr, ttbcr;
>   	long long offset;
> +	void *stack;
>   
>   	if (!mdesc->pv_fixup)
>   		return;
> @@ -1675,7 +1676,14 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
>   	/* Run the patch stub to update the constants */
>   	fixup_pv_table(&__pv_table_begin,
>   		(&__pv_table_end - &__pv_table_begin) << 2);
> -	flush_cache_all();
> +
> +	/*
> +	 * clean stack in cacheline that undering memory will be changed in
> +	 * the following flush_cache_all(). assuming 32 bytes is enough for
> +	 * flush_cache_all().

Adding this extra clean here seems reasonable, but this comment needs
fixed to give the exact reasoning and warn others to not dirty the stack
after this point. Maybe something like

/*
  * The stack is currently in cacheable memory, after caching is disabled
  * writes to the stack will bypass the cached stack. If this now stale
  * cached stack is then evicted it will overwrite the updated stack in
  * memory. Clean the stack's cache-line and then ensure no writes to the
  * stack are made between here and disabling the cache below.
  */

Andrew

> +	 */
> +	stack = (void *) (current_stack_pointer - 32);
> +	__cpuc_flush_dcache_area(stack, 32);
>   
>   	/*
>   	 * We changing not only the virtual to physical mapping, but also
> @@ -1691,6 +1699,7 @@ static void __init early_paging_init(const struct machine_desc *mdesc)
>   	asm("mrc p15, 0, %0, c2, c0, 2" : "=r" (ttbcr));
>   	asm volatile("mcr p15, 0, %0, c2, c0, 2"
>   		: : "r" (ttbcr & ~(3 << 8 | 3 << 10)));
> +	flush_cache_all();
>   
>   	/*
>   	 * Fixup the page tables - this must be in the idmap region as
> 
>>
>>> So, please don't put it in the patch system.
>>>
>>> The patch certainly needs to be tested on TI Keystone which is the
>>> primary user of this code.
>>
>> Added Andrew Davis and Nishanth Menon to the thread:
>> can you folks review and test this for Keystone?
>>
>> Yours,
>> Linus Walleij

Patch

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 674ed71573a8..03fb0fe926f3 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1675,6 +1675,7 @@  static void __init early_paging_init(const struct machine_desc *mdesc)
 	/* Run the patch stub to update the constants */
 	fixup_pv_table(&__pv_table_begin,
 		(&__pv_table_end - &__pv_table_begin) << 2);
+	flush_cache_all();
 
 	/*
 	 * We changing not only the virtual to physical mapping, but also
@@ -1690,7 +1691,6 @@  static void __init early_paging_init(const struct machine_desc *mdesc)
 	asm("mrc p15, 0, %0, c2, c0, 2" : "=r" (ttbcr));
 	asm volatile("mcr p15, 0, %0, c2, c0, 2"
 		: : "r" (ttbcr & ~(3 << 8 | 3 << 10)));
-	flush_cache_all();
 
 	/*
 	 * Fixup the page tables - this must be in the idmap region as