[PATCH] arm: mm: Poison freed init memory

[PATCH] arm: mm: Poison freed init memory

[Stephen Boyd]

Poisoning __init marked memory can be useful when tracking down
obscure memory corruption bugs. When a pointer is 0xCCCCCCCC in an
oops it's much more obvious that somebody is using __init marked
memory after kernel initialization. This should help find
incorrect __init markings earlier and mimics what other
architectures are doing already.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

This is a minimal patch to get the idea across. I'm tempted to duplicate
free_area() and rename it to free_init_area() and then have it take virtual
addresses instead of pfns. Then the call sites could be cleaned up to pass
virtual addresses in the case of init memory, and we could remove the NULL
argument for the highpages and DMA users. Thoughts?

 arch/arm/mm/init.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 5164069..b7535ec 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -19,6 +19,7 @@
 #include <linux/gfp.h>
 #include <linux/memblock.h>
 #include <linux/sort.h>
+#include <linux/poison.h>
 
 #include <asm/mach-types.h>
 #include <asm/sections.h>
@@ -358,7 +359,8 @@ void __init bootmem_init(void)
 	max_pfn = max_high - PHYS_PFN_OFFSET;
 }
 
-static inline int free_area(unsigned long pfn, unsigned long end, char *s)
+static inline int free_area(unsigned long pfn, unsigned long end, char *s,
+		bool init_mem)
 {
 	unsigned int pages = 0, size = (end - pfn) << (PAGE_SHIFT - 10);
 
@@ -366,6 +368,9 @@ static inline int free_area(unsigned long pfn, unsigned long end, char *s)
 		struct page *page = pfn_to_page(pfn);
 		ClearPageReserved(page);
 		init_page_count(page);
+		if (init_mem)
+			memset(__va(__pfn_to_phys(pfn)), POISON_FREE_INITMEM,
+					PAGE_SIZE);
 		__free_page(page);
 		pages++;
 	}
@@ -472,7 +477,7 @@ static void __init free_highpages(void)
 				res_end = end;
 			if (res_start != start)
 				totalhigh_pages += free_area(start, res_start,
-							     NULL);
+							     NULL, false);
 			start = res_end;
 			if (start == end)
 				break;
@@ -480,7 +485,7 @@ static void __init free_highpages(void)
 
 		/* And now free anything which remains */
 		if (start < end)
-			totalhigh_pages += free_area(start, end, NULL);
+			totalhigh_pages += free_area(start, end, NULL, false);
 	}
 	totalram_pages += totalhigh_pages;
 #endif
@@ -512,7 +517,8 @@ void __init mem_init(void)
 #ifdef CONFIG_SA1111
 	/* now that our DMA memory is actually so designated, we can free it */
 	totalram_pages += free_area(PHYS_PFN_OFFSET,
-				    __phys_to_pfn(__pa(swapper_pg_dir)), NULL);
+				    __phys_to_pfn(__pa(swapper_pg_dir)), NULL,
+				    false);
 #endif
 
 	free_highpages();
@@ -644,13 +650,13 @@ void free_initmem(void)
 
 	totalram_pages += free_area(__phys_to_pfn(__pa(&__tcm_start)),
 				    __phys_to_pfn(__pa(&__tcm_end)),
-				    "TCM link");
+				    "TCM link", true);
 #endif
 
 	if (!machine_is_integrator() && !machine_is_cintegrator())
 		totalram_pages += free_area(__phys_to_pfn(__pa(__init_begin)),
 					    __phys_to_pfn(__pa(__init_end)),
-					    "init");
+					    "init", true);
 }
 
 #ifdef CONFIG_BLK_DEV_INITRD
@@ -662,7 +668,7 @@ void free_initrd_mem(unsigned long start, unsigned long end)
 	if (!keep_initrd)
 		totalram_pages += free_area(__phys_to_pfn(__pa(start)),
 					    __phys_to_pfn(__pa(end)),
-					    "initrd");
+					    "initrd", true);
 }
 
 static int __init keepinitrd_setup(char *__unused)
-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] arm: mm: Poison freed init memory

[Russell King - ARM Linux]

On Wed, Jan 05, 2011 at 11:47:25AM -0800, Stephen Boyd wrote:
> Poisoning __init marked memory can be useful when tracking down
> obscure memory corruption bugs. When a pointer is 0xCCCCCCCC in an

That's a bad idea for a value.  With a 3GB page offset and 256MB or
more memory, accesses to such an address will always succeed.

There's two things to be considered when selecting a possible poison
value:

1. what value is guaranteed to provoke an undefined instruction exception?
2. what value when used as an address and dereferenced is mostly always
   going to abort?

1 for ARM mode implies an 0xe7fXXXfX value.  For Thumb mode 0xdeXX.  We
use this space for breakpoints.

2 unfortunately depends on the platform. 

[PATCH] arm: mm: Poison freed init memory

[Stephen Boyd]

On 01/05/2011 12:26 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 05, 2011 at 11:47:25AM -0800, Stephen Boyd wrote:
>> Poisoning __init marked memory can be useful when tracking down
>> obscure memory corruption bugs. When a pointer is 0xCCCCCCCC in an
>
> That's a bad idea for a value.  With a 3GB page offset and 256MB or
> more memory, accesses to such an address will always succeed.
>
> There's two things to be considered when selecting a possible poison
> value:
>
> 1. what value is guaranteed to provoke an undefined instruction exception?
> 2. what value when used as an address and dereferenced is mostly always
>    going to abort?
>
> 1 for ARM mode implies an 0xe7fXXXfX value.  For Thumb mode 0xdeXX.  We
> use this space for breakpoints.
>
> 2 unfortunately depends on the platform. 

A coworker proposed we use a SWI instruction. We could do that if the
poison is 0xEF and then do something in the SWI handler where that
number causes us to blow up?

If I'm following correctly, point 1 is about __init functions and point
2 is about __initdata. I'm more concerned about __initdata because
__init functions called from non __init marked functions are usually
caught with section mismatch checks. Also, if we're jumping to
0xCCCCCCCC we're probably not in the text section of the kernel with a
3GB offset anymore, right? __initdata is easier to reference from
anywhere (ignoring function pointers) and it would definitely be nice to
find those invalid accesses quicker. For point 2, perhaps an unaligned
access would trigger sometimes?

Swapping the cheese for some cheese flavored poison is better than nothing.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] arm: mm: Poison freed init memory

[Russell King - ARM Linux]

On Wed, Jan 05, 2011 at 09:25:55PM -0800, Stephen Boyd wrote:
> On 01/05/2011 12:26 PM, Russell King - ARM Linux wrote:
> > On Wed, Jan 05, 2011 at 11:47:25AM -0800, Stephen Boyd wrote:
> >> Poisoning __init marked memory can be useful when tracking down
> >> obscure memory corruption bugs. When a pointer is 0xCCCCCCCC in an
> >
> > That's a bad idea for a value.  With a 3GB page offset and 256MB or
> > more memory, accesses to such an address will always succeed.
> >
> > There's two things to be considered when selecting a possible poison
> > value:
> >
> > 1. what value is guaranteed to provoke an undefined instruction exception?
> > 2. what value when used as an address and dereferenced is mostly always
> >    going to abort?
> >
> > 1 for ARM mode implies an 0xe7fXXXfX value.  For Thumb mode 0xdeXX.  We
> > use this space for breakpoints.
> >
> > 2 unfortunately depends on the platform. 
> 
> A coworker proposed we use a SWI instruction. We could do that if the
> poison is 0xEF and then do something in the SWI handler where that
> number causes us to blow up?

Doesn't work with EABI - the comment field in the SWI instruction is
ignored on EABI.

> If I'm following correctly, point 1 is about __init functions and point
> 2 is about __initdata. I'm more concerned about __initdata because
> __init functions called from non __init marked functions are usually
> caught with section mismatch checks. Also, if we're jumping to
> 0xCCCCCCCC we're probably not in the text section of the kernel with a

But, as I pointed out, you don't know that 0xCCCCCCCC isn't a valid
address _and_ on modern platforms it won't fault.  So it's pointless
to use it as a poison value.

[PATCH] arm: mm: Poison freed init memory

[Stephen Boyd]

On 01/06/2011 01:07 AM, Russell King - ARM Linux wrote:
> On Wed, Jan 05, 2011 at 09:25:55PM -0800, Stephen Boyd wrote:
>> On 01/05/2011 12:26 PM, Russell King - ARM Linux wrote:
>>> On Wed, Jan 05, 2011 at 11:47:25AM -0800, Stephen Boyd wrote:
>>>> Poisoning __init marked memory can be useful when tracking down
>>>> obscure memory corruption bugs. When a pointer is 0xCCCCCCCC in an
>>>
>>> That's a bad idea for a value.  With a 3GB page offset and 256MB or
>>> more memory, accesses to such an address will always succeed.
>>>
>>> There's two things to be considered when selecting a possible poison
>>> value:
>>>
>>> 1. what value is guaranteed to provoke an undefined instruction exception?
>>> 2. what value when used as an address and dereferenced is mostly always
>>>    going to abort?
>>>
>>> 1 for ARM mode implies an 0xe7fXXXfX value.  For Thumb mode 0xdeXX.  We
>>> use this space for breakpoints.
>>>
>>> 2 unfortunately depends on the platform. 
>>
>> A coworker proposed we use a SWI instruction. We could do that if the
>> poison is 0xEF and then do something in the SWI handler where that
>> number causes us to blow up?
>
> Doesn't work with EABI - the comment field in the SWI instruction is
> ignored on EABI.
>
>> If I'm following correctly, point 1 is about __init functions and point
>> 2 is about __initdata. I'm more concerned about __initdata because
>> __init functions called from non __init marked functions are usually
>> caught with section mismatch checks. Also, if we're jumping to
>> 0xCCCCCCCC we're probably not in the text section of the kernel with a
>
> But, as I pointed out, you don't know that 0xCCCCCCCC isn't a valid
> address _and_ on modern platforms it won't fault.  So it's pointless
> to use it as a poison value.

Ok it seems that 0xcc was chosen by Pavel since it's a breakpoint
instruction (sorry for not noticing that earlier [1]). There was some
discussion about handling initdata with Pavel's patch but it seems that
nothing came of it? I assume that's because we don't differentiate
between the two types of init markings.

How about we use 0xe7fddef0? This seems to satisfy at least your first
point for both ARM and Thumb mode (Thumb will branch to the 0xdef0
instruction).

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0410.0/2040.html for
those interested

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH] arm: mm: Poison freed init memory

[Russell King - ARM Linux]

On Mon, Jan 10, 2011 at 09:00:17PM -0800, Stephen Boyd wrote:
> Ok it seems that 0xcc was chosen by Pavel since it's a breakpoint
> instruction (sorry for not noticing that earlier [1]).

It may be on x86, but on ARM:

   0:   cccccccc        stclgt  12, cr12, [ip], {204}

That's a co-processor #12 instruction which will only be executed of
the processor condition codes satisfy 'gt'.

> There was some discussion about handling initdata with Pavel's patch
> but it seems that nothing came of it?

I'm not sure who this Pavel is who you keep referring to - the message
you link to is a discussion between William Irvin and hpa.

> How about we use 0xe7fddef0? This seems to satisfy at least your first
> point for both ARM and Thumb mode (Thumb will branch to the 0xdef0
> instruction).

Yup.

[PATCH] arm: mm: Poison freed init memory

[Pavel Machek]

Hi!

> > Ok it seems that 0xcc was chosen by Pavel since it's a breakpoint
> > instruction (sorry for not noticing that earlier [1]).
> 
> It may be on x86, but on ARM:
> 
>    0:   cccccccc        stclgt  12, cr12, [ip], {204}
> 
> That's a co-processor #12 instruction which will only be executed of
> the processor condition codes satisfy 'gt'.
> 
> > There was some discussion about handling initdata with Pavel's patch
> > but it seems that nothing came of it?
> 
> I'm not sure who this Pavel is who you keep referring to - the message
> you link to is a discussion between William Irvin and hpa.

I was surprised, too, but it looks like I was the one who ported patch
from x86-64.

I'm not really sure if initdata poisoning happened... it is too long
ago.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html