Re: [PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address
On Thu, Sep 10, 2020 at 05:02:56PM +0200, Michal Simek wrote: > > > On 10. 09. 20 15:50, Tom Rini wrote: > > On Thu, Sep 10, 2020 at 03:38:25PM +0200, Michal Simek wrote: > >> > >> > >> On 10. 09. 20 15:06, André Przywara wrote: > >>> On 10/09/2020 13:38, Michal Simek wrote: > > > On 09. 09. 20 19:07, Edgar E. Iglesias wrote: > > From: "Edgar E. Iglesias" > > > > Trap non-PIE builds early if the start address doesn't > > match between run-time and link-time. This will trap the > > startup sequence rather than letting it run into obscure > > errors. > > > > Signed-off-by: Edgar E. Iglesias > > --- arch/arm/cpu/armv8/start.S > > | 13 + 1 file changed, 13 insertions(+) > > > > diff --git a/arch/arm/cpu/armv8/start.S > > b/arch/arm/cpu/armv8/start.S index e5c2856cf5..39e1b842c4 > > 100644 --- a/arch/arm/cpu/armv8/start.S +++ > > b/arch/arm/cpu/armv8/start.S @@ -101,6 +101,19 @@ > > pie_skip_reloc: cmp x2, x3 b.lo pie_fix_loop > > pie_fixup_done: +#else +adr x0, _start +ldr x1, > > _TEXT_BASE +cmp x0, x1 +beq 1f +0: +/* + > > * FATAL, can't > > continue. + * U-Boot needs to start executing at > > CONFIG_SYS_TEXT_BASE. + */ + wfi + b 0b +1: #endif > > > > #ifdef CONFIG_SYS_RESET_SCTRL > > > > NACK for this. > > 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text > base for U-Boot proper 2. It likely also breaks TPL flow for > the same reason > > 3. And last thing is that this code is used only for U-Boot > proper. .globl _TEXT_BASE _TEXT_BASE: .quad > CONFIG_SYS_TEXT_BASE > > The fixes are below. Point 3 should be likely be in separate > patch because it is unrelated. > >>> > >>> So if this patch causes issues, can't we just drop it? I mean > >>> right now you will probably just crash anyway if you load it at > >>> the wrong address, but maybe late enough that you get more > >>> hints or even some output. > >>> > >>> Now this patch makes sure that you don't get anything, so I > >>> don't see how this is really improving the situation. It seems > >>> like a case of "don't fix things that ain't broken". > >> > >> I am fine with dropping it. Tom: What do you think? > > > > OK, yes, we can set this aside for now at least. I assume this is > > all for v2021.01 anyhow? > > > > I would target it for 2021.01. > Dropping #4 and queueing the rest for 2021.01 sounds good to me too. We can revisit a possible check for non-PIE later. Cheers, Edgar
Re: [PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address
On 10. 09. 20 15:50, Tom Rini wrote: > On Thu, Sep 10, 2020 at 03:38:25PM +0200, Michal Simek wrote: >> >> >> On 10. 09. 20 15:06, André Przywara wrote: >>> On 10/09/2020 13:38, Michal Simek wrote: On 09. 09. 20 19:07, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Trap non-PIE builds early if the start address doesn't > match between run-time and link-time. This will trap the > startup sequence rather than letting it run into obscure > errors. > > Signed-off-by: Edgar E. Iglesias > --- arch/arm/cpu/armv8/start.S > | 13 + 1 file changed, 13 insertions(+) > > diff --git a/arch/arm/cpu/armv8/start.S > b/arch/arm/cpu/armv8/start.S index e5c2856cf5..39e1b842c4 > 100644 --- a/arch/arm/cpu/armv8/start.S +++ > b/arch/arm/cpu/armv8/start.S @@ -101,6 +101,19 @@ > pie_skip_reloc: cmp x2, x3 b.lo pie_fix_loop > pie_fixup_done: +#else + adr x0, _start +ldr x1, > _TEXT_BASE + cmp x0, x1 +beq 1f +0: +/* + > * FATAL, can't > continue. +* U-Boot needs to start executing at > CONFIG_SYS_TEXT_BASE. +*/ + wfi + b 0b +1: #endif > > #ifdef CONFIG_SYS_RESET_SCTRL > NACK for this. 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for U-Boot proper 2. It likely also breaks TPL flow for the same reason 3. And last thing is that this code is used only for U-Boot proper. .globl _TEXT_BASE _TEXT_BASE: .quad CONFIG_SYS_TEXT_BASE The fixes are below. Point 3 should be likely be in separate patch because it is unrelated. >>> >>> So if this patch causes issues, can't we just drop it? I mean >>> right now you will probably just crash anyway if you load it at >>> the wrong address, but maybe late enough that you get more >>> hints or even some output. >>> >>> Now this patch makes sure that you don't get anything, so I >>> don't see how this is really improving the situation. It seems >>> like a case of "don't fix things that ain't broken". >> >> I am fine with dropping it. Tom: What do you think? > > OK, yes, we can set this aside for now at least. I assume this is > all for v2021.01 anyhow? > I would target it for 2021.01. Thanks, Michal
Re: [PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address
On Thu, Sep 10, 2020 at 03:38:25PM +0200, Michal Simek wrote: > > > On 10. 09. 20 15:06, André Przywara wrote: > > On 10/09/2020 13:38, Michal Simek wrote: > >> > >> > >> On 09. 09. 20 19:07, Edgar E. Iglesias wrote: > >>> From: "Edgar E. Iglesias" > >>> > >>> Trap non-PIE builds early if the start address doesn't match > >>> between run-time and link-time. This will trap the startup > >>> sequence rather than letting it run into obscure errors. > >>> > >>> Signed-off-by: Edgar E. Iglesias > >>> --- > >>> arch/arm/cpu/armv8/start.S | 13 + > >>> 1 file changed, 13 insertions(+) > >>> > >>> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S > >>> index e5c2856cf5..39e1b842c4 100644 > >>> --- a/arch/arm/cpu/armv8/start.S > >>> +++ b/arch/arm/cpu/armv8/start.S > >>> @@ -101,6 +101,19 @@ pie_skip_reloc: > >>> cmp x2, x3 > >>> b.lopie_fix_loop > >>> pie_fixup_done: > >>> +#else > >>> + adr x0, _start > >>> + ldr x1, _TEXT_BASE > >>> + cmp x0, x1 > >>> + beq 1f > >>> +0: > >>> + /* > >>> + * FATAL, can't continue. > >>> + * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE. > >>> + */ > >>> + wfi > >>> + b 0b > >>> +1: > >>> #endif > >>> > >>> #ifdef CONFIG_SYS_RESET_SCTRL > >>> > >> > >> NACK for this. > >> > >> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for > >> U-Boot proper > >> 2. It likely also breaks TPL flow for the same reason > >> > >> 3. And last thing is that this code is used only for U-Boot proper. > >> .globl _TEXT_BASE > >> _TEXT_BASE: > >>.quad CONFIG_SYS_TEXT_BASE > >> > >> The fixes are below. Point 3 should be likely be in separate patch > >> because it is unrelated. > > > > So if this patch causes issues, can't we just drop it? I mean right now > > you will probably just crash anyway if you load it at the wrong address, > > but maybe late enough that you get more hints or even some output. > > > > Now this patch makes sure that you don't get anything, so I don't see > > how this is really improving the situation. It seems like a case of > > "don't fix things that ain't broken". > > I am fine with dropping it. > Tom: What do you think? OK, yes, we can set this aside for now at least. I assume this is all for v2021.01 anyhow? -- Tom signature.asc Description: PGP signature
Re: [PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address
On 10. 09. 20 15:06, André Przywara wrote: > On 10/09/2020 13:38, Michal Simek wrote: >> >> >> On 09. 09. 20 19:07, Edgar E. Iglesias wrote: >>> From: "Edgar E. Iglesias" >>> >>> Trap non-PIE builds early if the start address doesn't match >>> between run-time and link-time. This will trap the startup >>> sequence rather than letting it run into obscure errors. >>> >>> Signed-off-by: Edgar E. Iglesias >>> --- >>> arch/arm/cpu/armv8/start.S | 13 + >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S >>> index e5c2856cf5..39e1b842c4 100644 >>> --- a/arch/arm/cpu/armv8/start.S >>> +++ b/arch/arm/cpu/armv8/start.S >>> @@ -101,6 +101,19 @@ pie_skip_reloc: >>> cmp x2, x3 >>> b.lopie_fix_loop >>> pie_fixup_done: >>> +#else >>> + adr x0, _start >>> + ldr x1, _TEXT_BASE >>> + cmp x0, x1 >>> + beq 1f >>> +0: >>> + /* >>> +* FATAL, can't continue. >>> +* U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE. >>> +*/ >>> + wfi >>> + b 0b >>> +1: >>> #endif >>> >>> #ifdef CONFIG_SYS_RESET_SCTRL >>> >> >> NACK for this. >> >> 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for >> U-Boot proper >> 2. It likely also breaks TPL flow for the same reason >> >> 3. And last thing is that this code is used only for U-Boot proper. >> .globl _TEXT_BASE >> _TEXT_BASE: >> .quad CONFIG_SYS_TEXT_BASE >> >> The fixes are below. Point 3 should be likely be in separate patch >> because it is unrelated. > > So if this patch causes issues, can't we just drop it? I mean right now > you will probably just crash anyway if you load it at the wrong address, > but maybe late enough that you get more hints or even some output. > > Now this patch makes sure that you don't get anything, so I don't see > how this is really improving the situation. It seems like a case of > "don't fix things that ain't broken". I am fine with dropping it. Tom: What do you think? Thanks, Michal
Re: [PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address
On 10/09/2020 13:38, Michal Simek wrote: > > > On 09. 09. 20 19:07, Edgar E. Iglesias wrote: >> From: "Edgar E. Iglesias" >> >> Trap non-PIE builds early if the start address doesn't match >> between run-time and link-time. This will trap the startup >> sequence rather than letting it run into obscure errors. >> >> Signed-off-by: Edgar E. Iglesias >> --- >> arch/arm/cpu/armv8/start.S | 13 + >> 1 file changed, 13 insertions(+) >> >> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S >> index e5c2856cf5..39e1b842c4 100644 >> --- a/arch/arm/cpu/armv8/start.S >> +++ b/arch/arm/cpu/armv8/start.S >> @@ -101,6 +101,19 @@ pie_skip_reloc: >> cmp x2, x3 >> b.lopie_fix_loop >> pie_fixup_done: >> +#else >> +adr x0, _start >> +ldr x1, _TEXT_BASE >> +cmp x0, x1 >> +beq 1f >> +0: >> +/* >> + * FATAL, can't continue. >> + * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE. >> + */ >> +wfi >> +b 0b >> +1: >> #endif >> >> #ifdef CONFIG_SYS_RESET_SCTRL >> > > NACK for this. > > 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for > U-Boot proper > 2. It likely also breaks TPL flow for the same reason > > 3. And last thing is that this code is used only for U-Boot proper. > .globl_TEXT_BASE > _TEXT_BASE: > .quad CONFIG_SYS_TEXT_BASE > > The fixes are below. Point 3 should be likely be in separate patch > because it is unrelated. So if this patch causes issues, can't we just drop it? I mean right now you will probably just crash anyway if you load it at the wrong address, but maybe late enough that you get more hints or even some output. Now this patch makes sure that you don't get anything, so I don't see how this is really improving the situation. It seems like a case of "don't fix things that ain't broken". Cheers, Andre > > Thanks, > Michal > > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S > index b37dbabf4d42..cdc609e873fc 100644 > --- a/arch/arm/cpu/armv8/start.S > +++ b/arch/arm/cpu/armv8/start.S > @@ -33,10 +33,11 @@ _start: > > .align 3 > > +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD)) > .globl _TEXT_BASE > _TEXT_BASE: > .quad CONFIG_SYS_TEXT_BASE > - > +#endif > /* > * These are defined in the linker script. > */ > @@ -102,6 +103,7 @@ pie_skip_reloc: > b.lopie_fix_loop > pie_fixup_done: > #else > +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD)) > adr x0, _start > ldr x1, _TEXT_BASE > cmp x0, x1 > @@ -115,6 +117,7 @@ pie_fixup_done: > b 0b > 1: > #endif > +#endif > > #ifdef CONFIG_SYS_RESET_SCTRL > bl reset_sctrl >
Re: [PATCH v3 4/4] arm64: Trap non-PIE builds early if starting from wrong address
On 09. 09. 20 19:07, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" > > Trap non-PIE builds early if the start address doesn't match > between run-time and link-time. This will trap the startup > sequence rather than letting it run into obscure errors. > > Signed-off-by: Edgar E. Iglesias > --- > arch/arm/cpu/armv8/start.S | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S > index e5c2856cf5..39e1b842c4 100644 > --- a/arch/arm/cpu/armv8/start.S > +++ b/arch/arm/cpu/armv8/start.S > @@ -101,6 +101,19 @@ pie_skip_reloc: > cmp x2, x3 > b.lopie_fix_loop > pie_fixup_done: > +#else > + adr x0, _start > + ldr x1, _TEXT_BASE > + cmp x0, x1 > + beq 1f > +0: > + /* > + * FATAL, can't continue. > + * U-Boot needs to start executing at CONFIG_SYS_TEXT_BASE. > + */ > + wfi > + b 0b > +1: > #endif > > #ifdef CONFIG_SYS_RESET_SCTRL > NACK for this. 1. It breaks SPL flow because CONFIG_SYS_TEXT_BASE is text base for U-Boot proper 2. It likely also breaks TPL flow for the same reason 3. And last thing is that this code is used only for U-Boot proper. .globl _TEXT_BASE _TEXT_BASE: .quad CONFIG_SYS_TEXT_BASE The fixes are below. Point 3 should be likely be in separate patch because it is unrelated. Thanks, Michal diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S index b37dbabf4d42..cdc609e873fc 100644 --- a/arch/arm/cpu/armv8/start.S +++ b/arch/arm/cpu/armv8/start.S @@ -33,10 +33,11 @@ _start: .align 3 +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD)) .globl _TEXT_BASE _TEXT_BASE: .quad CONFIG_SYS_TEXT_BASE - +#endif /* * These are defined in the linker script. */ @@ -102,6 +103,7 @@ pie_skip_reloc: b.lopie_fix_loop pie_fixup_done: #else +#if !(defined(CONFIG_SPL_BUILD) || defined(CONFIG_TPL_BUILD)) adr x0, _start ldr x1, _TEXT_BASE cmp x0, x1 @@ -115,6 +117,7 @@ pie_fixup_done: b 0b 1: #endif +#endif #ifdef CONFIG_SYS_RESET_SCTRL bl reset_sctrl
