* Regression with arm in next with stack protector
@ 2018-03-27 9:04 ` Russell King - ARM Linux
0 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 9:04 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> Hi,
>
> Looks like commit 5638790dadae ("zboot: fix stack protector in
> compressed boot phase") breaks booting on arm.
>
> This is all I get from the bootloader on omap3:
>
> Starting kernel ...
>
> data abort
> pc : [<810002d0>] lr : [<100110a8>]
> reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> Flags: nZCv IRQs off FIQs off Mode SVC_32
> Resetting CPU ...
>
> resetting ...
The reason for this is the following code that was introduced by the
referenced patch:
+ ldr r0, =__stack_chk_guard
+ ldr r1, =0x000a0dff
+ str r1, [r0]
This uses the absolute address of __stack_chk_guard in the decompressor,
which is a self-relocatable image. As with all constructs like the
above, this absolute address doesn't get fixed up, and so it ends up
pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
and the decompressor looks to be around 0x81000000.
Such constructs can not be used in the decompressor for exactly this
reason - they need to use PC-relative addressing instead just like
everything else does in head.S.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Regression with arm in next with stack protector
2018-03-27 9:04 ` Russell King - ARM Linux
@ 2018-03-27 9:11 ` Russell King - ARM Linux
-1 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 9:11 UTC (permalink / raw)
To: Tony Lindgren
Cc: Stephen Rothwell, Rich Felker, Yoshinori Sato, linux-kernel,
Ralf Baechle, linux-omap, Huacai Chen, Andrew Morton,
James Hogan, linux-arm-kernel
On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
> >
> > This is all I get from the bootloader on omap3:
> >
> > Starting kernel ...
> >
> > data abort
> > pc : [<810002d0>] lr : [<100110a8>]
> > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Resetting CPU ...
> >
> > resetting ...
>
> The reason for this is the following code that was introduced by the
> referenced patch:
>
> + ldr r0, =__stack_chk_guard
> + ldr r1, =0x000a0dff
> + str r1, [r0]
>
> This uses the absolute address of __stack_chk_guard in the decompressor,
> which is a self-relocatable image. As with all constructs like the
> above, this absolute address doesn't get fixed up, and so it ends up
> pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> and the decompressor looks to be around 0x81000000.
>
> Such constructs can not be used in the decompressor for exactly this
> reason - they need to use PC-relative addressing instead just like
> everything else does in head.S.
I guess someone's not going to see my message to correct their patch:
chenhc@lemote.com
SMTP error from remote mail server after end of data:
host mxbiz1.qq.com [184.105.206.88]: 550 Mail content denied.
http://service.exmail.qq.com/cgi-bin/help?subtype=1&&id=20022&&no=1000726
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread* Regression with arm in next with stack protector
@ 2018-03-27 9:11 ` Russell King - ARM Linux
0 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 9:11 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
> >
> > This is all I get from the bootloader on omap3:
> >
> > Starting kernel ...
> >
> > data abort
> > pc : [<810002d0>] lr : [<100110a8>]
> > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Resetting CPU ...
> >
> > resetting ...
>
> The reason for this is the following code that was introduced by the
> referenced patch:
>
> + ldr r0, =__stack_chk_guard
> + ldr r1, =0x000a0dff
> + str r1, [r0]
>
> This uses the absolute address of __stack_chk_guard in the decompressor,
> which is a self-relocatable image. As with all constructs like the
> above, this absolute address doesn't get fixed up, and so it ends up
> pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> and the decompressor looks to be around 0x81000000.
>
> Such constructs can not be used in the decompressor for exactly this
> reason - they need to use PC-relative addressing instead just like
> everything else does in head.S.
I guess someone's not going to see my message to correct their patch:
chenhc at lemote.com
SMTP error from remote mail server after end of data:
host mxbiz1.qq.com [184.105.206.88]: 550 Mail content denied.
http://service.exmail.qq.com/cgi-bin/help?subtype=1&&id=20022&&no=1000726
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Regression with arm in next with stack protector
2018-03-27 9:04 ` Russell King - ARM Linux
@ 2018-03-27 11:34 ` Russell King - ARM Linux
-1 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 11:34 UTC (permalink / raw)
To: Tony Lindgren
Cc: Stephen Rothwell, Rich Felker, Yoshinori Sato, linux-kernel,
Ralf Baechle, linux-omap, Huacai Chen, Andrew Morton,
James Hogan, linux-arm-kernel
On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
> >
> > This is all I get from the bootloader on omap3:
> >
> > Starting kernel ...
> >
> > data abort
> > pc : [<810002d0>] lr : [<100110a8>]
> > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Resetting CPU ...
> >
> > resetting ...
>
> The reason for this is the following code that was introduced by the
> referenced patch:
>
> + ldr r0, =__stack_chk_guard
> + ldr r1, =0x000a0dff
> + str r1, [r0]
>
> This uses the absolute address of __stack_chk_guard in the decompressor,
> which is a self-relocatable image. As with all constructs like the
> above, this absolute address doesn't get fixed up, and so it ends up
> pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> and the decompressor looks to be around 0x81000000.
>
> Such constructs can not be used in the decompressor for exactly this
> reason - they need to use PC-relative addressing instead just like
> everything else does in head.S.
Is there any reason we can't do this in misc.c:
const unsigned int __stack_chk_guard = 0x000a0dff;
? That would save having runtime code to set that value up, and after
all, it is constant. Arguments about it potentially ending up at a
fixed offset into the image need not be said - that's already the case
with placing it in the early assembly code, and as has been established,
it needs to be initialised prior to any C code being called.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread* Regression with arm in next with stack protector
@ 2018-03-27 11:34 ` Russell King - ARM Linux
0 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 11:34 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
> >
> > This is all I get from the bootloader on omap3:
> >
> > Starting kernel ...
> >
> > data abort
> > pc : [<810002d0>] lr : [<100110a8>]
> > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Resetting CPU ...
> >
> > resetting ...
>
> The reason for this is the following code that was introduced by the
> referenced patch:
>
> + ldr r0, =__stack_chk_guard
> + ldr r1, =0x000a0dff
> + str r1, [r0]
>
> This uses the absolute address of __stack_chk_guard in the decompressor,
> which is a self-relocatable image. As with all constructs like the
> above, this absolute address doesn't get fixed up, and so it ends up
> pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> and the decompressor looks to be around 0x81000000.
>
> Such constructs can not be used in the decompressor for exactly this
> reason - they need to use PC-relative addressing instead just like
> everything else does in head.S.
Is there any reason we can't do this in misc.c:
const unsigned int __stack_chk_guard = 0x000a0dff;
? That would save having runtime code to set that value up, and after
all, it is constant. Arguments about it potentially ending up at a
fixed offset into the image need not be said - that's already the case
with placing it in the early assembly code, and as has been established,
it needs to be initialised prior to any C code being called.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Regression with arm in next with stack protector
2018-03-27 11:34 ` Russell King - ARM Linux
@ 2018-03-27 15:36 ` Rich Felker
-1 siblings, 0 replies; 30+ messages in thread
From: Rich Felker @ 2018-03-27 15:36 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Tony Lindgren, Stephen Rothwell, Yoshinori Sato, linux-kernel,
Ralf Baechle, linux-omap, Huacai Chen, Andrew Morton,
James Hogan, linux-arm-kernel
On Tue, Mar 27, 2018 at 12:34:53PM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > > Hi,
> > >
> > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > compressed boot phase") breaks booting on arm.
> > >
> > > This is all I get from the bootloader on omap3:
> > >
> > > Starting kernel ...
> > >
> > > data abort
> > > pc : [<810002d0>] lr : [<100110a8>]
> > > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > > Resetting CPU ...
> > >
> > > resetting ...
> >
> > The reason for this is the following code that was introduced by the
> > referenced patch:
> >
> > + ldr r0, =__stack_chk_guard
> > + ldr r1, =0x000a0dff
> > + str r1, [r0]
> >
> > This uses the absolute address of __stack_chk_guard in the decompressor,
> > which is a self-relocatable image. As with all constructs like the
> > above, this absolute address doesn't get fixed up, and so it ends up
> > pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> > and the decompressor looks to be around 0x81000000.
> >
> > Such constructs can not be used in the decompressor for exactly this
> > reason - they need to use PC-relative addressing instead just like
> > everything else does in head.S.
>
> Is there any reason we can't do this in misc.c:
>
> const unsigned int __stack_chk_guard = 0x000a0dff;
>
> ? That would save having runtime code to set that value up, and after
> all, it is constant. Arguments about it potentially ending up at a
> fixed offset into the image need not be said - that's already the case
> with placing it in the early assembly code, and as has been established,
> it needs to be initialised prior to any C code being called.
I've asked this multiple times in this thread and as far as I know
nobody has answered.
Rich
^ permalink raw reply [flat|nested] 30+ messages in thread* Regression with arm in next with stack protector
@ 2018-03-27 15:36 ` Rich Felker
0 siblings, 0 replies; 30+ messages in thread
From: Rich Felker @ 2018-03-27 15:36 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 27, 2018 at 12:34:53PM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > > Hi,
> > >
> > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > compressed boot phase") breaks booting on arm.
> > >
> > > This is all I get from the bootloader on omap3:
> > >
> > > Starting kernel ...
> > >
> > > data abort
> > > pc : [<810002d0>] lr : [<100110a8>]
> > > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > > Resetting CPU ...
> > >
> > > resetting ...
> >
> > The reason for this is the following code that was introduced by the
> > referenced patch:
> >
> > + ldr r0, =__stack_chk_guard
> > + ldr r1, =0x000a0dff
> > + str r1, [r0]
> >
> > This uses the absolute address of __stack_chk_guard in the decompressor,
> > which is a self-relocatable image. As with all constructs like the
> > above, this absolute address doesn't get fixed up, and so it ends up
> > pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> > and the decompressor looks to be around 0x81000000.
> >
> > Such constructs can not be used in the decompressor for exactly this
> > reason - they need to use PC-relative addressing instead just like
> > everything else does in head.S.
>
> Is there any reason we can't do this in misc.c:
>
> const unsigned int __stack_chk_guard = 0x000a0dff;
>
> ? That would save having runtime code to set that value up, and after
> all, it is constant. Arguments about it potentially ending up at a
> fixed offset into the image need not be said - that's already the case
> with placing it in the early assembly code, and as has been established,
> it needs to be initialised prior to any C code being called.
I've asked this multiple times in this thread and as far as I know
nobody has answered.
Rich
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Regression with arm in next with stack protector
2018-03-27 9:04 ` Russell King - ARM Linux
@ 2018-03-27 15:35 ` Rich Felker
-1 siblings, 0 replies; 30+ messages in thread
From: Rich Felker @ 2018-03-27 15:35 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Tony Lindgren, Huacai Chen, Andrew Morton, Stephen Rothwell,
Ralf Baechle, James Hogan, Yoshinori Sato, linux-kernel,
linux-arm-kernel, linux-omap
On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
> >
> > This is all I get from the bootloader on omap3:
> >
> > Starting kernel ...
> >
> > data abort
> > pc : [<810002d0>] lr : [<100110a8>]
> > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Resetting CPU ...
> >
> > resetting ...
>
> The reason for this is the following code that was introduced by the
> referenced patch:
>
> + ldr r0, =__stack_chk_guard
> + ldr r1, =0x000a0dff
> + str r1, [r0]
>
> This uses the absolute address of __stack_chk_guard in the decompressor,
> which is a self-relocatable image. As with all constructs like the
> above, this absolute address doesn't get fixed up, and so it ends up
> pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> and the decompressor looks to be around 0x81000000.
>
> Such constructs can not be used in the decompressor for exactly this
> reason - they need to use PC-relative addressing instead just like
> everything else does in head.S.
Can someone please answer why this is even needed to begin with? I
don't see any compelling reason __stack_chk_guard needs a particular
value in the decompressor, which is not dealing with any non-constant
input. Just putting __stack_chk_guard in its bss should be fine and
would eliminate all the risks of wrong code to load a value into it.
Alternatively put it in initialized data with the desired value.
Rich
^ permalink raw reply [flat|nested] 30+ messages in thread* Regression with arm in next with stack protector
@ 2018-03-27 15:35 ` Rich Felker
0 siblings, 0 replies; 30+ messages in thread
From: Rich Felker @ 2018-03-27 15:35 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
> >
> > This is all I get from the bootloader on omap3:
> >
> > Starting kernel ...
> >
> > data abort
> > pc : [<810002d0>] lr : [<100110a8>]
> > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > Resetting CPU ...
> >
> > resetting ...
>
> The reason for this is the following code that was introduced by the
> referenced patch:
>
> + ldr r0, =__stack_chk_guard
> + ldr r1, =0x000a0dff
> + str r1, [r0]
>
> This uses the absolute address of __stack_chk_guard in the decompressor,
> which is a self-relocatable image. As with all constructs like the
> above, this absolute address doesn't get fixed up, and so it ends up
> pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> and the decompressor looks to be around 0x81000000.
>
> Such constructs can not be used in the decompressor for exactly this
> reason - they need to use PC-relative addressing instead just like
> everything else does in head.S.
Can someone please answer why this is even needed to begin with? I
don't see any compelling reason __stack_chk_guard needs a particular
value in the decompressor, which is not dealing with any non-constant
input. Just putting __stack_chk_guard in its bss should be fine and
would eliminate all the risks of wrong code to load a value into it.
Alternatively put it in initialized data with the desired value.
Rich
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Regression with arm in next with stack protector
2018-03-27 15:35 ` Rich Felker
@ 2018-03-27 17:20 ` Russell King - ARM Linux
-1 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 17:20 UTC (permalink / raw)
To: Rich Felker
Cc: Tony Lindgren, Huacai Chen, Andrew Morton, Stephen Rothwell,
Ralf Baechle, James Hogan, Yoshinori Sato, linux-kernel,
linux-arm-kernel, linux-omap
On Tue, Mar 27, 2018 at 11:35:25AM -0400, Rich Felker wrote:
> On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > > Hi,
> > >
> > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > compressed boot phase") breaks booting on arm.
> > >
> > > This is all I get from the bootloader on omap3:
> > >
> > > Starting kernel ...
> > >
> > > data abort
> > > pc : [<810002d0>] lr : [<100110a8>]
> > > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > > Resetting CPU ...
> > >
> > > resetting ...
> >
> > The reason for this is the following code that was introduced by the
> > referenced patch:
> >
> > + ldr r0, =__stack_chk_guard
> > + ldr r1, =0x000a0dff
> > + str r1, [r0]
> >
> > This uses the absolute address of __stack_chk_guard in the decompressor,
> > which is a self-relocatable image. As with all constructs like the
> > above, this absolute address doesn't get fixed up, and so it ends up
> > pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> > and the decompressor looks to be around 0x81000000.
> >
> > Such constructs can not be used in the decompressor for exactly this
> > reason - they need to use PC-relative addressing instead just like
> > everything else does in head.S.
>
> Can someone please answer why this is even needed to begin with? I
> don't see any compelling reason __stack_chk_guard needs a particular
> value in the decompressor, which is not dealing with any non-constant
> input.
Untrue - it can do some parsing of the DT and updating/appending
information from ATAGs. However, all that should be coming from
a trusted environment, so I don't see much of a "trust" issue here.
(If the parent environment is not trusted, then the environment we're
running in is not trusted.)
> Just putting __stack_chk_guard in its bss should be fine and
> would eliminate all the risks of wrong code to load a value into it.
> Alternatively put it in initialized data with the desired value.
I'm no expert with this, so I can't comment. I build my kernels
with gcc 4.7.4, which I don't think supports this feature.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread* Regression with arm in next with stack protector
@ 2018-03-27 17:20 ` Russell King - ARM Linux
0 siblings, 0 replies; 30+ messages in thread
From: Russell King - ARM Linux @ 2018-03-27 17:20 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 27, 2018 at 11:35:25AM -0400, Rich Felker wrote:
> On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > > Hi,
> > >
> > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > compressed boot phase") breaks booting on arm.
> > >
> > > This is all I get from the bootloader on omap3:
> > >
> > > Starting kernel ...
> > >
> > > data abort
> > > pc : [<810002d0>] lr : [<100110a8>]
> > > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > > Resetting CPU ...
> > >
> > > resetting ...
> >
> > The reason for this is the following code that was introduced by the
> > referenced patch:
> >
> > + ldr r0, =__stack_chk_guard
> > + ldr r1, =0x000a0dff
> > + str r1, [r0]
> >
> > This uses the absolute address of __stack_chk_guard in the decompressor,
> > which is a self-relocatable image. As with all constructs like the
> > above, this absolute address doesn't get fixed up, and so it ends up
> > pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> > and the decompressor looks to be around 0x81000000.
> >
> > Such constructs can not be used in the decompressor for exactly this
> > reason - they need to use PC-relative addressing instead just like
> > everything else does in head.S.
>
> Can someone please answer why this is even needed to begin with? I
> don't see any compelling reason __stack_chk_guard needs a particular
> value in the decompressor, which is not dealing with any non-constant
> input.
Untrue - it can do some parsing of the DT and updating/appending
information from ATAGs. However, all that should be coming from
a trusted environment, so I don't see much of a "trust" issue here.
(If the parent environment is not trusted, then the environment we're
running in is not trusted.)
> Just putting __stack_chk_guard in its bss should be fine and
> would eliminate all the risks of wrong code to load a value into it.
> Alternatively put it in initialized data with the desired value.
I'm no expert with this, so I can't comment. I build my kernels
with gcc 4.7.4, which I don't think supports this feature.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Regression with arm in next with stack protector
2018-03-27 17:20 ` Russell King - ARM Linux
@ 2018-03-27 17:27 ` Rich Felker
-1 siblings, 0 replies; 30+ messages in thread
From: Rich Felker @ 2018-03-27 17:27 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Tony Lindgren, Huacai Chen, Andrew Morton, Stephen Rothwell,
Ralf Baechle, James Hogan, Yoshinori Sato, linux-kernel,
linux-arm-kernel, linux-omap
On Tue, Mar 27, 2018 at 06:20:27PM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 27, 2018 at 11:35:25AM -0400, Rich Felker wrote:
> > On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> > > On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > > > Hi,
> > > >
> > > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > > compressed boot phase") breaks booting on arm.
> > > >
> > > > This is all I get from the bootloader on omap3:
> > > >
> > > > Starting kernel ...
> > > >
> > > > data abort
> > > > pc : [<810002d0>] lr : [<100110a8>]
> > > > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > > > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > > > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > > > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > > > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > > > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > > > Resetting CPU ...
> > > >
> > > > resetting ...
> > >
> > > The reason for this is the following code that was introduced by the
> > > referenced patch:
> > >
> > > + ldr r0, =__stack_chk_guard
> > > + ldr r1, =0x000a0dff
> > > + str r1, [r0]
> > >
> > > This uses the absolute address of __stack_chk_guard in the decompressor,
> > > which is a self-relocatable image. As with all constructs like the
> > > above, this absolute address doesn't get fixed up, and so it ends up
> > > pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> > > and the decompressor looks to be around 0x81000000.
> > >
> > > Such constructs can not be used in the decompressor for exactly this
> > > reason - they need to use PC-relative addressing instead just like
> > > everything else does in head.S.
> >
> > Can someone please answer why this is even needed to begin with? I
> > don't see any compelling reason __stack_chk_guard needs a particular
> > value in the decompressor, which is not dealing with any non-constant
> > input.
>
> Untrue - it can do some parsing of the DT and updating/appending
> information from ATAGs. However, all that should be coming from
> a trusted environment, so I don't see much of a "trust" issue here.
> (If the parent environment is not trusted, then the environment we're
> running in is not trusted.)
OK, I was considering DT constant, but it doesn't really matter as you
say since the input comes from a trusted environment and could subvert
the system in much more direct ways than blowing away the
decompressor's stack buffers if it wanted to.
> > Just putting __stack_chk_guard in its bss should be fine and
> > would eliminate all the risks of wrong code to load a value into it.
> > Alternatively put it in initialized data with the desired value.
>
> I'm no expert with this, so I can't comment. I build my kernels
> with gcc 4.7.4, which I don't think supports this feature.
By "this feature" do you mean stack protector? I still have a 4.7.3
for x86 around and -fstack-protector-all works fine on it. Not sure if
there are issues using stack protector with kernel, or on ARM, for
older GCCs. In any case defining __stack_chk_guard as initialized data
should work on any gcc version regardless of whether stack protector
is actually used; it doesn't require any compiler features just basic
C.
Rich
^ permalink raw reply [flat|nested] 30+ messages in thread* Regression with arm in next with stack protector
@ 2018-03-27 17:27 ` Rich Felker
0 siblings, 0 replies; 30+ messages in thread
From: Rich Felker @ 2018-03-27 17:27 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 27, 2018 at 06:20:27PM +0100, Russell King - ARM Linux wrote:
> On Tue, Mar 27, 2018 at 11:35:25AM -0400, Rich Felker wrote:
> > On Tue, Mar 27, 2018 at 10:04:10AM +0100, Russell King - ARM Linux wrote:
> > > On Fri, Mar 23, 2018 at 11:14:53AM -0700, Tony Lindgren wrote:
> > > > Hi,
> > > >
> > > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > > compressed boot phase") breaks booting on arm.
> > > >
> > > > This is all I get from the bootloader on omap3:
> > > >
> > > > Starting kernel ...
> > > >
> > > > data abort
> > > > pc : [<810002d0>] lr : [<100110a8>]
> > > > reloc pc : [<9d6002d0>] lr : [<2c6110a8>]
> > > > sp : 81467c18 ip : 81466bf0 fp : 81466bf0
> > > > r10: 80fc2c40 r9 : 81000258 r8 : 86fec000
> > > > r7 : ffffffff r6 : 81466bf8 r5 : 00000000 r4 : 80008000
> > > > r3 : 81466c14 r2 : 81466c18 r1 : 000a0dff r0 : 00466bf8
> > > > Flags: nZCv IRQs off FIQs off Mode SVC_32
> > > > Resetting CPU ...
> > > >
> > > > resetting ...
> > >
> > > The reason for this is the following code that was introduced by the
> > > referenced patch:
> > >
> > > + ldr r0, =__stack_chk_guard
> > > + ldr r1, =0x000a0dff
> > > + str r1, [r0]
> > >
> > > This uses the absolute address of __stack_chk_guard in the decompressor,
> > > which is a self-relocatable image. As with all constructs like the
> > > above, this absolute address doesn't get fixed up, and so it ends up
> > > pointing at invalid memory (in this case 0x466bf8) vs RAM at 0x80000000,
> > > and the decompressor looks to be around 0x81000000.
> > >
> > > Such constructs can not be used in the decompressor for exactly this
> > > reason - they need to use PC-relative addressing instead just like
> > > everything else does in head.S.
> >
> > Can someone please answer why this is even needed to begin with? I
> > don't see any compelling reason __stack_chk_guard needs a particular
> > value in the decompressor, which is not dealing with any non-constant
> > input.
>
> Untrue - it can do some parsing of the DT and updating/appending
> information from ATAGs. However, all that should be coming from
> a trusted environment, so I don't see much of a "trust" issue here.
> (If the parent environment is not trusted, then the environment we're
> running in is not trusted.)
OK, I was considering DT constant, but it doesn't really matter as you
say since the input comes from a trusted environment and could subvert
the system in much more direct ways than blowing away the
decompressor's stack buffers if it wanted to.
> > Just putting __stack_chk_guard in its bss should be fine and
> > would eliminate all the risks of wrong code to load a value into it.
> > Alternatively put it in initialized data with the desired value.
>
> I'm no expert with this, so I can't comment. I build my kernels
> with gcc 4.7.4, which I don't think supports this feature.
By "this feature" do you mean stack protector? I still have a 4.7.3
for x86 around and -fstack-protector-all works fine on it. Not sure if
there are issues using stack protector with kernel, or on ARM, for
older GCCs. In any case defining __stack_chk_guard as initialized data
should work on any gcc version regardless of whether stack protector
is actually used; it doesn't require any compiler features just basic
C.
Rich
^ permalink raw reply [flat|nested] 30+ messages in thread