[PATCH] arm/cpu/lowlevel: fix: possible processor mode change

[PATCH] arm/cpu/lowlevel: fix: possible processor mode change

[Alexander Kurz]

This is a re-application of fix 17644b55.
arm_cpu_lowlevel_init() will set the processor mode to 0x13 (supervisor).
When this function is entered via a different processor mode, register
banking will happen to lr (r14), resulting in an invalid return address.
This fix will preserve the return address manually.

Signed-off-by: Alexander Kurz <akurz@blala.de>
---
 arch/arm/cpu/lowlevel.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
index b76222d..e5baa12 100644
--- a/arch/arm/cpu/lowlevel.S
+++ b/arch/arm/cpu/lowlevel.S
@@ -4,6 +4,8 @@
 
 .section ".text_bare_init_","ax"
 ENTRY(arm_cpu_lowlevel_init)
+	/* save lr, since it may be banked away with a processor mode change */
+	mov	r2, lr
 	/* set the cpu to SVC32 mode, mask irq and fiq */
 	mrs	r12, cpsr
 	bic	r12, r12, #0x1f
@@ -54,5 +56,5 @@ ENTRY(arm_cpu_lowlevel_init)
 
 	mcr	p15, 0, r12, c1, c0, 0		/* SCTLR */
 
-	mov	pc, lr
+	mov	pc, r2
 ENDPROC(arm_cpu_lowlevel_init)
-- 
2.1.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] arm/cpu/lowlevel: fix: possible processor mode change

[Sascha Hauer]

Hi Alexander,

On Wed, Mar 02, 2016 at 11:51:28PM +0100, Alexander Kurz wrote:
> This is a re-application of fix 17644b55.
> arm_cpu_lowlevel_init() will set the processor mode to 0x13 (supervisor).
> When this function is entered via a different processor mode, register
> banking will happen to lr (r14), resulting in an invalid return address.
> This fix will preserve the return address manually.
> 
> Signed-off-by: Alexander Kurz <akurz@blala.de>
> ---
>  arch/arm/cpu/lowlevel.S | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> index b76222d..e5baa12 100644
> --- a/arch/arm/cpu/lowlevel.S
> +++ b/arch/arm/cpu/lowlevel.S
> @@ -4,6 +4,8 @@
>  
>  .section ".text_bare_init_","ax"
>  ENTRY(arm_cpu_lowlevel_init)
> +	/* save lr, since it may be banked away with a processor mode change */
> +	mov	r2, lr

Thanks for fixing this and for adding a comment why this is done. This
hopefully prevents us from breaking it again.

Out of interest, what system are you using where this fix is necesssary?

Uwe, now we know why that was done and why e190bcf (arm/cpu/lowlevel:
Don't save the return address in another register) was a bad idea.

Applied this one to master.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] arm/cpu/lowlevel: fix: possible processor mode change

[Uwe Kleine-König]

Hello Sascha,

On Fri, Mar 04, 2016 at 08:04:48AM +0100, Sascha Hauer wrote:
> Hi Alexander,
> 
> On Wed, Mar 02, 2016 at 11:51:28PM +0100, Alexander Kurz wrote:
> > This is a re-application of fix 17644b55.
> > arm_cpu_lowlevel_init() will set the processor mode to 0x13 (supervisor).
> > When this function is entered via a different processor mode, register
> > banking will happen to lr (r14), resulting in an invalid return address.
> > This fix will preserve the return address manually.
> > 
> > Signed-off-by: Alexander Kurz <akurz@blala.de>
> > ---
> >  arch/arm/cpu/lowlevel.S | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> > index b76222d..e5baa12 100644
> > --- a/arch/arm/cpu/lowlevel.S
> > +++ b/arch/arm/cpu/lowlevel.S
> > @@ -4,6 +4,8 @@
> >  
> >  .section ".text_bare_init_","ax"
> >  ENTRY(arm_cpu_lowlevel_init)
> > +	/* save lr, since it may be banked away with a processor mode change */
> > +	mov	r2, lr

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

> Thanks for fixing this and for adding a comment why this is done. This
> hopefully prevents us from breaking it again.
> 
> Out of interest, what system are you using where this fix is necesssary?
> 
> Uwe, now we know why that was done and why e190bcf (arm/cpu/lowlevel:
> Don't save the return address in another register) was a bad idea.

Right. :-) But even if I had seen
17644b55cae9c234b26213d644e9fd939b0ec815 back then I would have wondered
because the commit log isn't that verbose :-(

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] arm/cpu/lowlevel: fix: possible processor mode change

[Alexander Kurz]

Hi Sascha,
I stumbled across this while starting a barebox image uploaded via 
JTAG to RAM onto an old Olimex EP9302 demo board which eventually
ran into abort mode (unrelated to barebox).

Anyway, this fix might affect any ARM target/board.
I do not have a good example, when barebox might get started by some
platform in mode != supervisor, but even though - barebox should be 
perpared for this.

Thanks, Alexander

On Fri, 4 Mar 2016, Sascha Hauer wrote:

> Hi Alexander,
> 
> On Wed, Mar 02, 2016 at 11:51:28PM +0100, Alexander Kurz wrote:
> > This is a re-application of fix 17644b55.
> > arm_cpu_lowlevel_init() will set the processor mode to 0x13 (supervisor).
> > When this function is entered via a different processor mode, register
> > banking will happen to lr (r14), resulting in an invalid return address.
> > This fix will preserve the return address manually.
> > 
> > Signed-off-by: Alexander Kurz <akurz@blala.de>
> > ---
> >  arch/arm/cpu/lowlevel.S | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> > index b76222d..e5baa12 100644
> > --- a/arch/arm/cpu/lowlevel.S
> > +++ b/arch/arm/cpu/lowlevel.S
> > @@ -4,6 +4,8 @@
> >  
> >  .section ".text_bare_init_","ax"
> >  ENTRY(arm_cpu_lowlevel_init)
> > +	/* save lr, since it may be banked away with a processor mode change */
> > +	mov	r2, lr
> 
> Thanks for fixing this and for adding a comment why this is done. This
> hopefully prevents us from breaking it again.
> 
> Out of interest, what system are you using where this fix is necesssary?
> 
> Uwe, now we know why that was done and why e190bcf (arm/cpu/lowlevel:
> Don't save the return address in another register) was a bad idea.
> 
> Applied this one to master.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] arm/cpu/lowlevel: fix: possible processor mode change

[Uwe Kleine-König]

On Fri, Mar 04, 2016 at 08:04:48AM +0100, Sascha Hauer wrote:
> Hi Alexander,
> 
> On Wed, Mar 02, 2016 at 11:51:28PM +0100, Alexander Kurz wrote:
> > This is a re-application of fix 17644b55.
> > arm_cpu_lowlevel_init() will set the processor mode to 0x13 (supervisor).
> > When this function is entered via a different processor mode, register
> > banking will happen to lr (r14), resulting in an invalid return address.
> > This fix will preserve the return address manually.
> > 
> > Signed-off-by: Alexander Kurz <akurz@blala.de>
> > ---
> >  arch/arm/cpu/lowlevel.S | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/cpu/lowlevel.S b/arch/arm/cpu/lowlevel.S
> > index b76222d..e5baa12 100644
> > --- a/arch/arm/cpu/lowlevel.S
> > +++ b/arch/arm/cpu/lowlevel.S
> > @@ -4,6 +4,8 @@
> >  
> >  .section ".text_bare_init_","ax"
> >  ENTRY(arm_cpu_lowlevel_init)
> > +	/* save lr, since it may be banked away with a processor mode change */
> > +	mov	r2, lr
> 
> Thanks for fixing this and for adding a comment why this is done. This
> hopefully prevents us from breaking it again.
> 
> Out of interest, what system are you using where this fix is necesssary?

arch/arm/boot/compressed/head.S in Linux has:

                /*
                 * Booting from Angel - need to enter SVC mode and disable
                 * FIQs/IRQs (numeric definitions from angel arm.h source).
                 * We only do this if we were in user mode on entry.
                 */
                mrs     r2, cpsr                @ get current mode
                tst     r2, #3                  @ not user?
                bne     not_angel
                mov     r0, #0x17               @ angel_SWIreason_EnterSVC
 ARM(           swi     0x123456        )       @ angel_SWI_ARM
 THUMB(         svc     0xab            )       @ angel_SWI_THUMB
not_angel:

Not that the patch under discussion would fix running barebox from
Angel, but it seems that this is another situation where the image is
not started in SVC.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox