From: Russell King - ARM Linux <linux@armlinux.org.uk> To: jeffy <jeffy.chen@rock-chips.com> Cc: Ingo Molnar <mingo@kernel.org>, chris.zhong@rock-chips.com, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: Re: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled Date: Mon, 23 Oct 2017 12:45:49 +0100 [thread overview] Message-ID: <20171023114549.GU20805@n2100.armlinux.org.uk> (raw) In-Reply-To: <20171023105046.GT20805@n2100.armlinux.org.uk> On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote: > On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote: > > Hi Russell, > > > > Thanks for your reply. > > > > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote: > > >>> > > >>>hmm, right, didn't notice the data is already aligned... > > >>>so it's indeed caused by the ksym: > > >>> > > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 > > >>>0 4096 > > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 > > >>>0 4 > > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 > > >>>0 4 > > >It's earlier - look for __ksymtab_strings. > > > > the problem i meet is the appended dtb code found dtb invalid. i thought > > that is because of unaligned zImage size, but i was wrong... > > Hmm, you really ought not to be using the appended dtb code for modern > systems - the appended dtb system is there for old boot loaders that > are incapable of dealing with a dtb. As is said in the option's help > text: > > This is meant as a backward compatibility convenience for those > systems with a bootloader that can't be upgraded to accommodate > the documented boot protocol using a device tree. > > Beware that there is very little in terms of protection against > this option being confused by leftover garbage in memory that might > look like a DTB header after a reboot if no actual DTB is appended > to zImage. Do not leave this option active in a production kernel > if you don't intend to always append a DTB. Proper passing of the > location into r2 of a bootloader provided DTB is always preferable > to this option. > > If you rely on it, and you have something that looks like a dtb after > the image, then things will go wrong, so it's better _not_ to use it > and to keep it disabled. > > That aside, thanks for doing a more in-depth analysis of what is going > on, which helps to understand /why/ Ard's fix works (whereas before > it was rather nebulous.) > > I wonder whether we ought to tell the linker to discard any unknown > sections by adding at the bottom: > > /DISCARD/ { *(*) } > > but I do think we need to document this, specifically that _edata must > point to the first byte after the binary file, and that the only > sections after it are allowed to be the .bss and stack sections. Short of adding the discard (which I think is itself risky, we've had problems in the main kernel's vmlinux.lds in this area), I think we ought to verify the size of the zImage file, so that the build fails when we generate a zImage which is wrong, rather than producing a zImage that is incorrect. Maybe something like this? 8<===== From: Russell King <rmk+kernel@armlinux.org.uk> Subject: [PATCH] ARM: verify size of zImage The linker can sometimes add additional sections to the zImage ELF file which results in the zImage binary being larger than expected. This causes appended DT blobs to fail. Verify that the zImage binary is the expected size, and fail the build if this is not the case. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- arch/arm/boot/Makefile | 6 +++++- arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100755 arch/arm/boot/verify_zimage.sh diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile index a3af4dc08c3e..1290875556ae 100644 --- a/arch/arm/boot/Makefile +++ b/arch/arm/boot/Makefile @@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE else +quiet_cmd_mkzimage = ZIMAGE $@ +cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \ + '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }' + $(obj)/xipImage: FORCE @echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)' @false @@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE $(Q)$(MAKE) $(build)=$(obj)/compressed $@ $(obj)/zImage: $(obj)/compressed/vmlinux FORCE - $(call if_changed,objcopy) + $(call if_changed,mkzimage) endif diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh new file mode 100755 index 000000000000..922a93e61aa7 --- /dev/null +++ b/arch/arm/boot/verify_zimage.sh @@ -0,0 +1,21 @@ +#!/bin/sh +set -e + +vmlinuz="$1" +zimage="$2" +nm="$3" + +magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) { + $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/; + $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/; +}; printf "%d\n", $magic_end - $magic_start;') + +zimage_size=$(stat -c '%s' "$zimage") + +# Verify that the resulting binary matches the size contained within +# the binary (iow, the linker has not added any additional sections.) +if [ $magic_size -ne $zimage_size ]; then + echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2 + exit 1 +fi +exit 0 -- 2.7.4 -- 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
WARNING: multiple messages have this Message-ID (diff)
From: linux@armlinux.org.uk (Russell King - ARM Linux) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled Date: Mon, 23 Oct 2017 12:45:49 +0100 [thread overview] Message-ID: <20171023114549.GU20805@n2100.armlinux.org.uk> (raw) In-Reply-To: <20171023105046.GT20805@n2100.armlinux.org.uk> On Mon, Oct 23, 2017 at 11:50:46AM +0100, Russell King - ARM Linux wrote: > On Mon, Oct 23, 2017 at 06:24:12PM +0800, jeffy wrote: > > Hi Russell, > > > > Thanks for your reply. > > > > On 10/23/2017 04:50 PM, Russell King - ARM Linux wrote: > > >>> > > >>>hmm, right, didn't notice the data is already aligned... > > >>>so it's indeed caused by the ksym: > > >>> > > >>> [ 9] .data PROGBITS 006ce000 6d6000 000200 00 WA 0 > > >>>0 4096 > > >>> [10] ___ksymtab+sort PROGBITS 006ce200 6d6200 000008 00 WA 0 > > >>>0 4 > > >>> [11] .bss NOBITS 006ce208 6d6208 00001c 00 WA 0 > > >>>0 4 > > >It's earlier - look for __ksymtab_strings. > > > > the problem i meet is the appended dtb code found dtb invalid. i thought > > that is because of unaligned zImage size, but i was wrong... > > Hmm, you really ought not to be using the appended dtb code for modern > systems - the appended dtb system is there for old boot loaders that > are incapable of dealing with a dtb. As is said in the option's help > text: > > This is meant as a backward compatibility convenience for those > systems with a bootloader that can't be upgraded to accommodate > the documented boot protocol using a device tree. > > Beware that there is very little in terms of protection against > this option being confused by leftover garbage in memory that might > look like a DTB header after a reboot if no actual DTB is appended > to zImage. Do not leave this option active in a production kernel > if you don't intend to always append a DTB. Proper passing of the > location into r2 of a bootloader provided DTB is always preferable > to this option. > > If you rely on it, and you have something that looks like a dtb after > the image, then things will go wrong, so it's better _not_ to use it > and to keep it disabled. > > That aside, thanks for doing a more in-depth analysis of what is going > on, which helps to understand /why/ Ard's fix works (whereas before > it was rather nebulous.) > > I wonder whether we ought to tell the linker to discard any unknown > sections by adding at the bottom: > > /DISCARD/ { *(*) } > > but I do think we need to document this, specifically that _edata must > point to the first byte after the binary file, and that the only > sections after it are allowed to be the .bss and stack sections. Short of adding the discard (which I think is itself risky, we've had problems in the main kernel's vmlinux.lds in this area), I think we ought to verify the size of the zImage file, so that the build fails when we generate a zImage which is wrong, rather than producing a zImage that is incorrect. Maybe something like this? 8<===== From: Russell King <rmk+kernel@armlinux.org.uk> Subject: [PATCH] ARM: verify size of zImage The linker can sometimes add additional sections to the zImage ELF file which results in the zImage binary being larger than expected. This causes appended DT blobs to fail. Verify that the zImage binary is the expected size, and fail the build if this is not the case. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- arch/arm/boot/Makefile | 6 +++++- arch/arm/boot/verify_zimage.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100755 arch/arm/boot/verify_zimage.sh diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile index a3af4dc08c3e..1290875556ae 100644 --- a/arch/arm/boot/Makefile +++ b/arch/arm/boot/Makefile @@ -53,6 +53,10 @@ $(obj)/Image $(obj)/zImage: FORCE else +quiet_cmd_mkzimage = ZIMAGE $@ +cmd_mkzimage = $(cmd_objcopy) && $(CONFIG_SHELL) -c \ + '$(srctree)/$(src)/verify_zimage.sh $< $@ "$(NM)" || { rm -f $@; false; }' + $(obj)/xipImage: FORCE @echo 'Kernel not configured for XIP (CONFIG_XIP_KERNEL!=y)' @false @@ -64,7 +68,7 @@ $(obj)/compressed/vmlinux: $(obj)/Image FORCE $(Q)$(MAKE) $(build)=$(obj)/compressed $@ $(obj)/zImage: $(obj)/compressed/vmlinux FORCE - $(call if_changed,objcopy) + $(call if_changed,mkzimage) endif diff --git a/arch/arm/boot/verify_zimage.sh b/arch/arm/boot/verify_zimage.sh new file mode 100755 index 000000000000..922a93e61aa7 --- /dev/null +++ b/arch/arm/boot/verify_zimage.sh @@ -0,0 +1,21 @@ +#!/bin/sh +set -e + +vmlinuz="$1" +zimage="$2" +nm="$3" + +magic_size=$("$nm" "$vmlinuz" | perl -e 'while (<>) { + $magic_start = hex($1) if /^([[:xdigit:]]+) . _magic_start$/; + $magic_end = hex($1) if /^([[:xdigit:]]+) . _magic_end$/; +}; printf "%d\n", $magic_end - $magic_start;') + +zimage_size=$(stat -c '%s' "$zimage") + +# Verify that the resulting binary matches the size contained within +# the binary (iow, the linker has not added any additional sections.) +if [ $magic_size -ne $zimage_size ]; then + echo "zImage size ($zimage_size) disagrees with linked size ($magic_size)" >&2 + exit 1 +fi +exit 0 -- 2.7.4 -- 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
next prev parent reply other threads:[~2017-10-23 11:46 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-18 5:01 [PATCH] ARM: Fix zImage file size not aligned with CONFIG_EFI_STUB enabled Jeffy Chen 2017-10-18 5:01 ` Jeffy Chen 2017-10-18 6:19 ` Chris Zhong 2017-10-18 6:19 ` Chris Zhong 2017-10-22 11:01 ` Ard Biesheuvel 2017-10-22 11:01 ` Ard Biesheuvel 2017-10-22 12:47 ` Russell King - ARM Linux 2017-10-22 12:47 ` Russell King - ARM Linux 2017-10-22 13:01 ` Ard Biesheuvel 2017-10-22 13:01 ` Ard Biesheuvel 2017-10-23 3:26 ` jeffy 2017-10-23 3:26 ` jeffy 2017-10-23 8:50 ` Russell King - ARM Linux 2017-10-23 8:50 ` Russell King - ARM Linux 2017-10-23 10:24 ` jeffy 2017-10-23 10:24 ` jeffy 2017-10-23 10:50 ` Russell King - ARM Linux 2017-10-23 10:50 ` Russell King - ARM Linux 2017-10-23 11:45 ` Russell King - ARM Linux [this message] 2017-10-23 11:45 ` Russell King - ARM Linux 2017-10-24 8:09 ` Ard Biesheuvel 2017-10-24 8:09 ` Ard Biesheuvel 2017-10-24 9:09 ` Russell King - ARM Linux 2017-10-24 9:09 ` Russell King - ARM Linux 2017-10-24 9:13 ` Ard Biesheuvel 2017-10-24 9:13 ` Ard Biesheuvel 2017-10-24 9:22 ` Russell King - ARM Linux 2017-10-24 9:22 ` Russell King - ARM Linux 2017-10-24 9:26 ` Ard Biesheuvel 2017-10-24 9:26 ` Ard Biesheuvel 2017-10-24 9:30 ` Ard Biesheuvel 2017-10-24 9:30 ` Ard Biesheuvel 2017-10-24 9:38 ` Russell King - ARM Linux 2017-10-24 9:38 ` Russell King - ARM Linux 2017-10-24 9:44 ` Ard Biesheuvel 2017-10-24 9:44 ` Ard Biesheuvel 2017-10-24 9:54 ` Russell King - ARM Linux 2017-10-24 9:54 ` Russell King - ARM Linux 2017-10-24 10:03 ` Ard Biesheuvel 2017-10-24 10:03 ` Ard Biesheuvel 2017-10-24 9:16 ` jeffy 2017-10-24 9:16 ` jeffy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20171023114549.GU20805@n2100.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=ard.biesheuvel@linaro.org \ --cc=chris.zhong@rock-chips.com \ --cc=jeffy.chen@rock-chips.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.