[PATCH] powerpc/kernel: Avoid memory corruption at early stage

When calling to early_setup(), we picks "boot_paca" up for the
master CPU and initialize that with initialise_paca(). At the
point, SLB shadow buffer isn't populated yet. Updating the SLB
shadow buffer should corrupt what we had in physical address 0
where the trap instruction is usually stored.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/kernel/paca.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index d6e195e..048a6ee 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c

@@ -115,6 +115,9 @@ static struct slb_shadow * __init init_slb_shadow(int cpu)
 {
 	struct slb_shadow *s = &slb_shadow[cpu];
 
+	if (!slb_shadow)
+		return NULL;
+
 	s->persistent = cpu_to_be32(SLB_NUM_BOLTED);
 	s->buffer_length = cpu_to_be32(sizeof(*s));
 

-- 
1.8.3.2

On Thu, 2015-01-08 at 16:40 +1100, Gavin Shan wrote:

When calling to early_setup(), we picks "boot_paca" up for the
master CPU and initialize that with initialise_paca(). At the
point, SLB shadow buffer isn't populated yet. Updating the SLB
shadow buffer should corrupt what we had in physical address 0
where the trap instruction is usually stored.

Ouch.

Introduced in 6f4441ef7009 ("powerpc: Dynamically allocate slb_shadow from
memblock") - December 2013.

So it seems it doesn't cause us any harm in general.

Did you actually hit a bug with it?

Yeah I guess that's an OK fix.

We must have a valid SLB shadow before we ever call _switch(), which is much
later. The only way we could hit this case for the real paca is if
allocate_slb_shadows() failed to allocate, but it would have panicked if it
did.

cheers

On Thu, Jan 22, 2015 at 05:21:22PM +1100, Michael Ellerman wrote:

On Thu, 2015-01-08 at 16:40 +1100, Gavin Shan wrote:

quoted

When calling to early_setup(), we picks "boot_paca" up for the
master CPU and initialize that with initialise_paca(). At the
point, SLB shadow buffer isn't populated yet. Updating the SLB
shadow buffer should corrupt what we had in physical address 0
where the trap instruction is usually stored.

Ouch.

Introduced in 6f4441ef7009 ("powerpc: Dynamically allocate slb_shadow from
memblock") - December 2013.

So it seems it doesn't cause us any harm in general.

Did you actually hit a bug with it?

I didn't hit any bugs with it. So I guess it's unnecessary to mark it as
"stable". I found the issue (not bug) occasionally: starting kernel in
simulator, dumping the instruction at physical address 0x0 and found
it's not "nop".

quoted

diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index d6e195e..048a6ee 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c

@@ -115,6 +115,9 @@ static struct slb_shadow * __init init_slb_shadow(int cpu)
 {
 	struct slb_shadow *s = &slb_shadow[cpu];
 
+	if (!slb_shadow)
+		return NULL;
+
 	s->persistent = cpu_to_be32(SLB_NUM_BOLTED);
 	s->buffer_length = cpu_to_be32(sizeof(*s));

Yeah I guess that's an OK fix.

We must have a valid SLB shadow before we ever call _switch(), which is much
later. The only way we could hit this case for the real paca is if
allocate_slb_shadows() failed to allocate, but it would have panicked if it
did.

Perhaps I can add BUG_ON(!slb_shadow) in allocate_slb_shadows() if you agree.

Thanks,
Gavin

cheers


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

On Thu, 2015-01-22 at 17:40 +1100, Gavin Shan wrote:

On Thu, Jan 22, 2015 at 05:21:22PM +1100, Michael Ellerman wrote:

quoted

On Thu, 2015-01-08 at 16:40 +1100, Gavin Shan wrote:

quoted

When calling to early_setup(), we picks "boot_paca" up for the
master CPU and initialize that with initialise_paca(). At the
point, SLB shadow buffer isn't populated yet. Updating the SLB
shadow buffer should corrupt what we had in physical address 0
where the trap instruction is usually stored.

Ouch.

Introduced in 6f4441ef7009 ("powerpc: Dynamically allocate slb_shadow from
memblock") - December 2013.

So it seems it doesn't cause us any harm in general.

Did you actually hit a bug with it?

I didn't hit any bugs with it. So I guess it's unnecessary to mark it as
"stable". I found the issue (not bug) occasionally: starting kernel in
simulator, dumping the instruction at physical address 0x0 and found
it's not "nop".

Yeah OK. So we'll merge it but it doesn't need to go to stable.

quoted

quoted

diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index d6e195e..048a6ee 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c

@@ -115,6 +115,9 @@ static struct slb_shadow * __init init_slb_shadow(int cpu)
 {
 	struct slb_shadow *s = &slb_shadow[cpu];
 
+	if (!slb_shadow)
+		return NULL;
+
 	s->persistent = cpu_to_be32(SLB_NUM_BOLTED);
 	s->buffer_length = cpu_to_be32(sizeof(*s));

Yeah I guess that's an OK fix.

We must have a valid SLB shadow before we ever call _switch(), which is much
later. The only way we could hit this case for the real paca is if
allocate_slb_shadows() failed to allocate, but it would have panicked if it
did.

Perhaps I can add BUG_ON(!slb_shadow) in allocate_slb_shadows() if you agree.

No need, memblock_alloc_base() already panics if it can't allocate.

cheers