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 ...
Regards,
Tony
On Fri, Mar 23, 2018 at 3:14 PM, Tony Lindgren <[email protected]> wrote:
> Hi,
>
> Looks like commit 5638790dadae ("zboot: fix stack protector in
> compressed boot phase") breaks booting on arm.
Yes, almost all imx are broken in linux-next due to this commit:
https://kernelci.org/soc/imx/job/next/kernel/next-20180323/
On Fri, 23 Mar 2018 15:45:27 -0300 Fabio Estevam <[email protected]> wrote:
> On Fri, Mar 23, 2018 at 3:14 PM, Tony Lindgren <[email protected]> wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
>
> Yes, almost all imx are broken in linux-next due to this commit:
> https://kernelci.org/soc/imx/job/next/kernel/next-20180323/
Thanks, I dropped it and I expect Stephen will also do that.
Hi Andrew,
On Fri, 23 Mar 2018 12:15:30 -0700 Andrew Morton <[email protected]> wrote:
>
> On Fri, 23 Mar 2018 15:45:27 -0300 Fabio Estevam <[email protected]> wrote:
>
> > On Fri, Mar 23, 2018 at 3:14 PM, Tony Lindgren <[email protected]> wrote:
> > > Hi,
> > >
> > > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > > compressed boot phase") breaks booting on arm.
> >
> > Yes, almost all imx are broken in linux-next due to this commit:
> > https://kernelci.org/soc/imx/job/next/kernel/next-20180323/
>
> Thanks, I dropped it and I expect Stephen will also do that.
OK, dropped.
--
Cheers,
Stephen Rothwell
Hi, Tony and Fabio,
Could you please upload your kernel binary to somewhere for me? I don't understand why some ARM boards is OK while others are broken.
Huacai
------------------ Original ------------------
From: "Andrew Morton"<[email protected]>;
Date: Sat, Mar 24, 2018 03:15 AM
To: "Fabio Estevam"<[email protected]>;
Cc: "Tony Lindgren"<[email protected]>; "Huacai Chen"<[email protected]>; "Stephen Rothwell"<[email protected]>; "Rich Felker"<[email protected]>; "Russell King"<[email protected]>; "Yoshinori Sato"<[email protected]>; "linux-kernel"<[email protected]>; "Ralf Baechle"<[email protected]>; "linux-omap"<[email protected]>; "James Hogan"<[email protected]>; "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"<[email protected]>;
Subject: Re: Regression with arm in next with stack protector
On Fri, 23 Mar 2018 15:45:27 -0300 Fabio Estevam <[email protected]> wrote:
> On Fri, Mar 23, 2018 at 3:14 PM, Tony Lindgren <[email protected]> wrote:
> > Hi,
> >
> > Looks like commit 5638790dadae ("zboot: fix stack protector in
> > compressed boot phase") breaks booting on arm.
>
> Yes, almost all imx are broken in linux-next due to this commit:
> https://kernelci.org/soc/imx/job/next/kernel/next-20180323/
Thanks, I dropped it and I expect Stephen will also do that.
* 陈华才 <[email protected]> [180326 06:59]:
> Hi, Tony and Fabio,
>
> Could you please upload your kernel binary to somewhere for me? I don't understand why some ARM boards is OK while others are broken.
Well the kernel I'm testing is just current Linux next cross
compiled omap2plus_defconfig kernel. I do have few more config
options enabled like LOCKDEP and DEBUG_ATOMIC_SLEEP, but I
doubt they matter here :)
Then I'm using gcc-7.3.0 and binutils-2.30 built with the
buildall.git scripts:
git://git.infradead.org/users/segher/buildall.git
If you still need binaries, let me know.
Do you know which arm devices are working with your patch?
Regards,
Tony
Allwinner devices are OK, Exynos devices (except 4210) are OK.
------------------ Original ------------------
From: "Tony Lindgren"<[email protected]>;
Date: Mon, Mar 26, 2018 11:37 PM
To: "陈华才"<[email protected]>;
Cc: "Andrew Morton"<[email protected]>; "Fabio Estevam"<[email protected]>; "Stephen Rothwell"<[email protected]>; "Rich Felker"<[email protected]>; "Russell King"<[email protected]>; "Yoshinori Sato"<[email protected]>; "linux-kernel"<[email protected]>; "Ralf Baechle"<[email protected]>; "linux-omap"<[email protected]>; "James Hogan"<[email protected]>; "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"<[email protected]>;
Subject: Re: Regression with arm in next with stack protector
* 陈华才 <[email protected]> [180326 06:59]:
> Hi, Tony and Fabio,
>
> Could you please upload your kernel binary to somewhere for me? I don't understand why some ARM boards is OK while others are broken.
Well the kernel I'm testing is just current Linux next cross
compiled omap2plus_defconfig kernel. I do have few more config
options enabled like LOCKDEP and DEBUG_ATOMIC_SLEEP, but I
doubt they matter here :)
Then I'm using gcc-7.3.0 and binutils-2.30 built with the
buildall.git scripts:
git://git.infradead.org/users/segher/buildall.git
If you still need binaries, let me know.
Do you know which arm devices are working with your patch?
Regards,
Tony
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
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:
[email protected]
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
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
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
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
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
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