| Message ID | 20160615211359.GJ1041@n2100.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote: >> I'm much less familiar with the ARM decompression stub, but is there a >> boot image header (like x86 has)? If not, perhaps we can invent one, >> and it can carry all the details needed for a bootloader to do the >> right things. > > With a bit of tinkering around, I now have this: > > 00000000 <.data>: > 0: e1a00000 nop ; (mov r0, r0) > 4: e1a00000 nop ; (mov r0, r0) > 8: e1a00000 nop ; (mov r0, r0) > c: e1a00000 nop ; (mov r0, r0) > 10: e1a00000 nop ; (mov r0, r0) > 14: e1a00000 nop ; (mov r0, r0) > 18: e1a00000 nop ; (mov r0, r0) > 1c: e1a00000 nop ; (mov r0, r0) > 20: ea00000f b 0x64 > > Then follows the existing "header" which we've had there for years: > > 24: 016f2818 ; LE magic number > 28: 00000000 ; LE zImage start address (always zero now) > 2c: 00431fe0 ; LE zImage _edata > 30: 04030201 ; endian flag > > And now comes the new header: > > 34: 016f2818 ; LE magic number Should this be a different magic from the existing header's magic? > 38: 00000001 ; LE version number (v1) Should a "size" follow the version number instead of the explicit 64 bytes of zeros? > 3c: 01287000 ; LE total space required for decompressor > 40: 00e54000 ; LE uncompressed image size > > Up to 64 bytes available here for future expansion, currently filled > with zeros. > ... > > Remainder of the zImage code: > 64: e10f9000 mrs r9, CPSR > > I'm rather on the fence whether we need to give the uncompressed image > size - the important thing is the size of memory that's required for > the decompressor to run, which is sizeof(uncompressed kernel) rounded > up to 256 bytes, and the relocated decompressor image size. I think it's important information since it allows a boot loader to figure out if there's room in a given range for the result. While there's no relocation support yet, if we gain it on ARM and we want to support KASLR, it will be very handy to have the uncompressed size available. > > The "total space required for decompressor" is slightly cheating at the > figure - I'm including the uncompressed image rounded up and the entire > compressed image in that size, so it's a safe over-estimate. > > I'm not sure there's a need to provide the uncompressed image size, the > boot environment shouldn't have a reason to know that, so I'm tempted to > omit it. > > We could dispense with the endian conversions, and push the responsibility > for interpreting that onto the reader of this data: we have the endian > flag in the existing header block, so the boot environment can work out > the endianness of the image and apply fixups as appropriate. > > Why generate this in the linker script? We need the size of the zImage > here, which is only known to the linker. > > diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile > index d50430c40045..1d5467e05250 100644 > --- a/arch/arm/boot/compressed/Makefile > +++ b/arch/arm/boot/compressed/Makefile > @@ -119,6 +119,10 @@ asflags-y := -DZIMAGE > KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \ > awk 'END{print $$3}') > LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ) > + > +KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image) > +LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE) > + > # Supply ZRELADDR to the decompressor via a linker symbol. > ifneq ($(CONFIG_AUTO_ZRELADDR),y) > LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR) > diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S > index e2e0dcb42ca2..395c60dcc4f7 100644 > --- a/arch/arm/boot/compressed/head.S > +++ b/arch/arm/boot/compressed/head.S > @@ -131,11 +131,7 @@ start: > THUMB( badr r12, 1f ) > THUMB( bx r12 ) > > - .word _magic_sig @ Magic numbers to help the loader > - .word _magic_start @ absolute load/run zImage address > - .word _magic_end @ zImage end address > - .word 0x04030201 @ endianness flag > - > + .section ".start2", #alloc, #execinstr > THUMB( .thumb ) > 1: __EFI_HEADER > > diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S > index 81c493156ce8..77267724ec8a 100644 > --- a/arch/arm/boot/compressed/vmlinux.lds.S > +++ b/arch/arm/boot/compressed/vmlinux.lds.S > @@ -37,6 +37,19 @@ SECTIONS > .text : { > _start = .; > *(.start) > + _header = .; > + LONG(ZIMAGE_MAGIC(0x016f2818)); /* Magic numbers to help the loader */ > + LONG(ZIMAGE_MAGIC(_start)); /* absolute load/run zImage address */ > + LONG(ZIMAGE_MAGIC(_edata)); /* zImage end address */ > + LONG(0x04030201); /* Endianness flag */ > + LONG(ZIMAGE_MAGIC(0x016f2818)); /* Further header indicator */ > + LONG(ZIMAGE_MAGIC(1)); /* Version 1 */ > + LONG(ZIMAGE_MAGIC(((_kernel_image_size + 255) & ~ 255) + \ > + _edata - _text + _end_stack - __bss_start)); > + LONG(ZIMAGE_MAGIC(_kernel_image_size)); > + /* Reserve 64 bytes for the header block */ > + . = _header + 64; > + *(.start2) > *(.text) > *(.text.*) > *(.fixup) > @@ -72,10 +85,6 @@ SECTIONS > .pad : { BYTE(0); . = ALIGN(8); } > _edata = .; > > - _magic_sig = ZIMAGE_MAGIC(0x016f2818); > - _magic_start = ZIMAGE_MAGIC(_start); > - _magic_end = ZIMAGE_MAGIC(_edata); > - > . = BSS_START; > __bss_start = .; > .bss : { *(.bss) } > @@ -83,6 +92,7 @@ SECTIONS > > . = ALIGN(8); /* the stack must be 64-bit aligned */ > .stack : { *(.stack) } > + _end_stack = .; > > .stab 0 : { *(.stab) } > .stabstr 0 : { *(.stabstr) } Regardless, this looks good to me! -Kees
On Wed, Jun 15, 2016 at 03:20:05PM -0700, Kees Cook wrote: > On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote: > >> I'm much less familiar with the ARM decompression stub, but is there a > >> boot image header (like x86 has)? If not, perhaps we can invent one, > >> and it can carry all the details needed for a bootloader to do the > >> right things. > > > > With a bit of tinkering around, I now have this: > > > > 00000000 <.data>: > > 0: e1a00000 nop ; (mov r0, r0) > > 4: e1a00000 nop ; (mov r0, r0) > > 8: e1a00000 nop ; (mov r0, r0) > > c: e1a00000 nop ; (mov r0, r0) > > 10: e1a00000 nop ; (mov r0, r0) > > 14: e1a00000 nop ; (mov r0, r0) > > 18: e1a00000 nop ; (mov r0, r0) > > 1c: e1a00000 nop ; (mov r0, r0) > > 20: ea00000f b 0x64 > > > > Then follows the existing "header" which we've had there for years: > > > > 24: 016f2818 ; LE magic number > > 28: 00000000 ; LE zImage start address (always zero now) > > 2c: 00431fe0 ; LE zImage _edata > > 30: 04030201 ; endian flag > > > > And now comes the new header: > > > > 34: 016f2818 ; LE magic number > > Should this be a different magic from the existing header's magic? What use would a different number be? It's not something you scan for, because this block is always at 0x24 bytes into the zImage for a binary zImage. > > 38: 00000001 ; LE version number (v1) > > Should a "size" follow the version number instead of the explicit 64 > bytes of zeros? I don't see the use of a size field. If we have a size field, then we need to validate the version number (which tells us the format of the data) against the size, and it all gets really messy - what if we have a version number which indicates that we have more data than the size field indicates? > > 3c: 01287000 ; LE total space required for decompressor > > 40: 00e54000 ; LE uncompressed image size > > > > Up to 64 bytes available here for future expansion, currently filled > > with zeros. > > ... > > > > Remainder of the zImage code: > > 64: e10f9000 mrs r9, CPSR > > > > I'm rather on the fence whether we need to give the uncompressed image > > size - the important thing is the size of memory that's required for > > the decompressor to run, which is sizeof(uncompressed kernel) rounded > > up to 256 bytes, and the relocated decompressor image size. > > I think it's important information since it allows a boot loader to > figure out if there's room in a given range for the result. While > there's no relocation support yet, if we gain it on ARM and we want to > support KASLR, it will be very handy to have the uncompressed size > available. It's really not useful for that purpose. The size of the uncompressed image doesn't tell you that you can touch the next few bytes after the image. In fact, you can't, because that's where the decompressor potentially relocates itself to. The one which is of importance to the boot environment layout is the "total space required for decompressor", which is the one which must be respected. In fact, the apparent confusion over this reinforces my belief that we should _not_ give the size of the uncompressed image at all. The boot environment must be setup such that there is room for the uncompressed image (aligned currently to 256 bytes) followed by the size of the compressed image, with any appended DTBs included. Anything which is located below that is likely to get trampled by the decompressor.
On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > In fact, the apparent confusion over this reinforces my belief that we > should _not_ give the size of the uncompressed image at all. > > The boot environment must be setup such that there is room for the > uncompressed image (aligned currently to 256 bytes) followed by the > size of the compressed image, with any appended DTBs included. > Anything which is located below that is likely to get trampled by > the decompressor. Okay, sounds reasonable to me. :) -Kees
On Wed, Jun 15, 2016 at 03:54:38PM -0700, Kees Cook wrote: > On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux > <linux@armlinux.org.uk> wrote: > > In fact, the apparent confusion over this reinforces my belief that we > > should _not_ give the size of the uncompressed image at all. > > > > The boot environment must be setup such that there is room for the > > uncompressed image (aligned currently to 256 bytes) followed by the > > size of the compressed image, with any appended DTBs included. > > Anything which is located below that is likely to get trampled by > > the decompressor. > > Okay, sounds reasonable to me. :) I should point out that this method should work for a zImage without an appended DTB, and we have no way to update this header block for the appended DTB case. So, an alternative standpoint is that we supply only the uncompressed image size. Then, the boot environment needs to understand that they must allow for the compressed image and any appended DTB on top of that (which it would see as one - the size of the combined image.) However, while that may sound like a good idea, we're falling into the same trap that we've fallen into at the beginning of this thread: the boot environment has to understand how the decompressor currently works, and if we were to change that, this we're back to the calculation which the boot environment is using not matching reality.
Hi Russell, On 16/06/2016:12:13:03 AM, Russell King - ARM Linux wrote: > On Wed, Jun 15, 2016 at 03:54:38PM -0700, Kees Cook wrote: > > On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux > > <linux@armlinux.org.uk> wrote: > > > In fact, the apparent confusion over this reinforces my belief that we > > > should _not_ give the size of the uncompressed image at all. > > > > > > The boot environment must be setup such that there is room for the > > > uncompressed image (aligned currently to 256 bytes) followed by the > > > size of the compressed image, with any appended DTBs included. > > > Anything which is located below that is likely to get trampled by > > > the decompressor. > > > > Okay, sounds reasonable to me. :) > > I should point out that this method should work for a zImage without > an appended DTB, and we have no way to update this header block for > the appended DTB case. Well, I might be missing with my limited knowledge. So, just to clarify, zImage is "compressed image + uncompressor". In some cases it might have "+DTB". So, zImage should be located atleast "aligned uncompressed size" above "kernel base" and, initrd should be placed at "kernel base" + "aligned uncompressed size" + "compressed image size" + "uncompressor size" + DTB(if any). However, I am unable to understand that why can't we have a flag in zImage header block, which tells that whether a DTB has been appended in the zImage or not? > > So, an alternative standpoint is that we supply only the uncompressed > image size. Then, the boot environment needs to understand that they > must allow for the compressed image and any appended DTB on top of that > (which it would see as one - the size of the combined image.) So, why not we can have all three information in header ie. "uncompressed image size", "compressed image size" and "DTB size" if appended flag is set. > > However, while that may sound like a good idea, we're falling into the > same trap that we've fallen into at the beginning of this thread: the > boot environment has to understand how the decompressor currently works, > and if we were to change that, this we're back to the calculation which > the boot environment is using not matching reality. Currently we are discussing between 4 times to 11 times. So, if we know *atleast* "uncompressed image size" then this heuristic variation can be minimized a lot. If we do not provide header, then may be we can provide library to the kexec-tools which will enable us to know the length of "uncompressed image" when "compressed image" is provided as input. ~Pratyush > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
On Tue, Jun 21, 2016 at 05:18:49PM +0530, Pratyush Anand wrote: > On 16/06/2016:12:13:03 AM, Russell King - ARM Linux wrote: > > I should point out that this method should work for a zImage without > > an appended DTB, and we have no way to update this header block for > > the appended DTB case. > > Well, I might be missing with my limited knowledge. So, just to clarify, zImage > is "compressed image + uncompressor". In some cases it might have "+DTB". > So, zImage should be located atleast "aligned uncompressed size" above "kernel > base" and, initrd should be placed at "kernel base" + "aligned uncompressed > size" + "compressed image size" + "uncompressor size" + DTB(if any). > > However, I am unable to understand that why can't we have a flag in zImage > header block, which tells that whether a DTB has been appended in the zImage or > not? cat zImage foo.dtb > newImage obviously doesn't result in the header block inside zImage being modified. The only way around that is to have a tool which concatenates the two and updates the header. At that point, it's no longer simply a concatenation. > > So, an alternative standpoint is that we supply only the uncompressed > > image size. Then, the boot environment needs to understand that they > > must allow for the compressed image and any appended DTB on top of that > > (which it would see as one - the size of the combined image.) > > So, why not we can have all three information in header ie. "uncompressed image > size", "compressed image size" and "DTB size" if appended flag is set. How big is the DTB size? Without a tool to update the header, there's no way to really know. > > However, while that may sound like a good idea, we're falling into the > > same trap that we've fallen into at the beginning of this thread: the > > boot environment has to understand how the decompressor currently works, > > and if we were to change that, this we're back to the calculation which > > the boot environment is using not matching reality. > > Currently we are discussing between 4 times to 11 times. So, if we know > *atleast* "uncompressed image size" then this heuristic variation can be > minimized a lot. If we do not provide header, then may be we can provide library > to the kexec-tools which will enable us to know the length of "uncompressed > image" when "compressed image" is provided as input. If we provide the uncompressed size, then we know how big the uncompressed image will be. That much is fine, but there's more than that. What is the padding applied to the uncompressed size? Where does the compressed image self-relocate to? Where does the compressed image relocate the DTB to? What is the spacing between the DTB and the compressed image? All those are knowledge which is internal to the decompressor code. If we supply, say, the padded uncompressed size, then we can work around not knowing the padding in kexec. However, we still need to know the other details - so we still end up needing to code into kexec (or other boot environment) a load of details specific to the current decompressor implementation.
On Wed, Jun 15, 2016 at 11:42:28PM +0100, Russell King - ARM Linux wrote: > The boot environment must be setup such that there is room for the > uncompressed image (aligned currently to 256 bytes) followed by the > size of the compressed image, with any appended DTBs included. > Anything which is located below that is likely to get trampled by > the decompressor. There's a question that's been lingering, which hasn't been brought up by anyone yet: should we even support zImage files with an appended DTB for kexec? When kexec is operating in DTB mode, it will provide the DTB to the kernel from the host environment, possibly with some modifications (eg, to change the command line arguments.) Since an appended DTB would override the kexec-provided DTB, this would wipe out those changes, some of which are necessary for things like crashdump to work. So, I'm thinking that kexec should not support a zImage with appended DTB - in fact, I think it should truncate the zImage to be loaded, and ensure that the word following the zImage does not contain a DTB magic number, in case the zImage has been built with CONFIG_ARM_APPENDED_DTB enabled. Thoughts?
* Russell King - ARM Linux <linux@armlinux.org.uk> [160707 03:25]: > On Wed, Jun 15, 2016 at 11:42:28PM +0100, Russell King - ARM Linux wrote: > > The boot environment must be setup such that there is room for the > > uncompressed image (aligned currently to 256 bytes) followed by the > > size of the compressed image, with any appended DTBs included. > > Anything which is located below that is likely to get trampled by > > the decompressor. > > There's a question that's been lingering, which hasn't been brought up > by anyone yet: should we even support zImage files with an appended > DTB for kexec? > > When kexec is operating in DTB mode, it will provide the DTB to the > kernel from the host environment, possibly with some modifications (eg, > to change the command line arguments.) Since an appended DTB would > override the kexec-provided DTB, this would wipe out those changes, > some of which are necessary for things like crashdump to work. > > So, I'm thinking that kexec should not support a zImage with appended > DTB - in fact, I think it should truncate the zImage to be loaded, and > ensure that the word following the zImage does not contain a DTB magic > number, in case the zImage has been built with CONFIG_ARM_APPENDED_DTB > enabled. > > Thoughts? Sorry for a late reply, I've been offline. Sounds like a good idea to me as this as we can properly error out with appendd dtb before anything nasty happens making new kernel fail. And if somebody really needs it, we can add an option for --force-appended-dtb. Regards, Tony
diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile index d50430c40045..1d5467e05250 100644 --- a/arch/arm/boot/compressed/Makefile +++ b/arch/arm/boot/compressed/Makefile @@ -119,6 +119,10 @@ asflags-y := -DZIMAGE KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \ awk 'END{print $$3}') LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ) + +KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image) +LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE) + # Supply ZRELADDR to the decompressor via a linker symbol. ifneq ($(CONFIG_AUTO_ZRELADDR),y) LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR) diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S index e2e0dcb42ca2..395c60dcc4f7 100644 --- a/arch/arm/boot/compressed/head.S +++ b/arch/arm/boot/compressed/head.S @@ -131,11 +131,7 @@ start: THUMB( badr r12, 1f ) THUMB( bx r12 ) - .word _magic_sig @ Magic numbers to help the loader - .word _magic_start @ absolute load/run zImage address - .word _magic_end @ zImage end address - .word 0x04030201 @ endianness flag - + .section ".start2", #alloc, #execinstr THUMB( .thumb ) 1: __EFI_HEADER diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S index 81c493156ce8..77267724ec8a 100644 --- a/arch/arm/boot/compressed/vmlinux.lds.S +++ b/arch/arm/boot/compressed/vmlinux.lds.S @@ -37,6 +37,19 @@ SECTIONS .text : { _start = .; *(.start) + _header = .; + LONG(ZIMAGE_MAGIC(0x016f2818)); /* Magic numbers to help the loader */ + LONG(ZIMAGE_MAGIC(_start)); /* absolute load/run zImage address */ + LONG(ZIMAGE_MAGIC(_edata)); /* zImage end address */ + LONG(0x04030201); /* Endianness flag */ + LONG(ZIMAGE_MAGIC(0x016f2818)); /* Further header indicator */ + LONG(ZIMAGE_MAGIC(1)); /* Version 1 */ + LONG(ZIMAGE_MAGIC(((_kernel_image_size + 255) & ~ 255) + \ + _edata - _text + _end_stack - __bss_start)); + LONG(ZIMAGE_MAGIC(_kernel_image_size)); + /* Reserve 64 bytes for the header block */ + . = _header + 64; + *(.start2) *(.text) *(.text.*) *(.fixup) @@ -72,10 +85,6 @@ SECTIONS .pad : { BYTE(0); . = ALIGN(8); } _edata = .; - _magic_sig = ZIMAGE_MAGIC(0x016f2818); - _magic_start = ZIMAGE_MAGIC(_start); - _magic_end = ZIMAGE_MAGIC(_edata); - . = BSS_START; __bss_start = .; .bss : { *(.bss) } @@ -83,6 +92,7 @@ SECTIONS . = ALIGN(8); /* the stack must be 64-bit aligned */ .stack : { *(.stack) } + _end_stack = .; .stab 0 : { *(.stab) } .stabstr 0 : { *(.stabstr) }