[U-Boot] [PATCH] Makefile: ensure DTB doesn't overflow into initial stack
From: Stephen Warren swarren@nvidia.com
With CONFIG_SYS_INIT_SP_BSS_OFFSET enabled, the initial (pre-relocation) stack is placed some distance after bss_start. The control DTB is appended to the U-Boot binary at bss_start. If the DTB is too large, or the SP BSS offset too small, then the initial stack could corrupt the DTB. Enhance the Makefile to check whether this is likely to occur.
Signed-off-by: Stephen Warren swarren@nvidia.com --- This builds on top of my previous patch "ARMv8: Allow dynamic early stack pointer". However, since all the logic is conditional and only activated if CONFIG_SYS_INIT_SP_BSS_OFFSET is defined, it can be applied with or without that other patch. It'd make sense to apply it afterwards and in the same branch though, or the change won't make a lot of sense to someone reading history in order.
Makefile | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/Makefile b/Makefile index d8f419bcd900..52cd6ea72161 100644 --- a/Makefile +++ b/Makefile @@ -811,6 +811,10 @@ ifneq ($(CONFIG_BUILD_TARGET),) ALL-y += $(CONFIG_BUILD_TARGET:"%"=%) endif
+ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ALL-y += init_sp_bss_offset_check +endif + LDFLAGS_u-boot += $(LDFLAGS_FINAL)
# Avoid 'Not enough room for program headers' error on binutils 2.28 onwards. @@ -939,6 +943,33 @@ binary_size_check: u-boot-nodtb.bin FORCE fi \ fi
+ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ifneq ($(CONFIG_SYS_MALLOC_F_LEN),) +subtract_sys_malloc_f_len = space=$$(($${space} - $(CONFIG_SYS_MALLOC_F_LEN))) +else +subtract_sys_malloc_f_len = true +endif +# The 1/4 margin below is somewhat arbitrary. The likely initial SP usage is +# so low that the DTB could probably use 90%+ of the available space, for +# current values of CONFIG_SYS_INIT_SP_BSS_OFFSET at least. However, let's be +# safe for now and tweak this later if space becomes tight. +# A rejected alternative would be to check that some absolute minimum stack +# space was available. However, since CONFIG_SYS_INIT_SP_BSS_OFFSET is +# deliberately build-specific, to take account of build-to-build stack usage +# differences due to different feature sets, there is no common absolute value +# to check against. +init_sp_bss_offset_check: u-boot.dtb FORCE + @dtb_size=$(shell wc -c u-boot.dtb | awk '{print $$1}') ; \ + space=$(CONFIG_SYS_INIT_SP_BSS_OFFSET) ; \ + $(subtract_sys_malloc_f_len) ; \ + quarter_space=$$(($${space} / 4)) ; \ + if [ $${dtb_size} -gt $${quarter_space} ]; then \ + echo "u-boot.dtb is larger than 1 quarter of " >&2 ; \ + echo "(CONFIG_SYS_INIT_SP_BSS_OFFSET - CONFIG_SYS_MALLOC_F_LEN)" >&2 ; \ + exit 1 ; \ + fi +endif + u-boot-nodtb.bin: u-boot FORCE $(call if_changed,objcopy) $(call DO_STATIC_RELA,$<,$@,$(CONFIG_SYS_TEXT_BASE))
Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: Tuesday, January 9, 2018 12:52 PM To: u-boot@lists.denx.de; Simon Glass sjg@chromium.org; Tom Warren TWarren@nvidia.com; Stephen Warren swarren@nvidia.com Cc: Tom Rini trini@konsulko.com Subject: [PATCH] Makefile: ensure DTB doesn't overflow into initial stack
From: Stephen Warren swarren@nvidia.com
With CONFIG_SYS_INIT_SP_BSS_OFFSET enabled, the initial (pre-relocation) stack is placed some distance after bss_start. The control DTB is appended to the U-Boot binary at bss_start. If the DTB is too large, or the SP BSS offset too small, then the initial stack could corrupt the DTB. Enhance the Makefile to check whether this is likely to occur.
Signed-off-by: Stephen Warren swarren@nvidia.com --- This builds on top of my previous patch "ARMv8: Allow dynamic early stack pointer". However, since all the logic is conditional and only activated if CONFIG_SYS_INIT_SP_BSS_OFFSET is defined, it can be applied with or without that other patch. It'd make sense to apply it afterwards and in the same branch though, or the change won't make a lot of sense to someone reading history in order.
I've got this in my TOT u-boot-tegra/master, ready to send a PR to TomR. But it's delegated to him in Patchwork. Do you want me to remove it from my PR, or leave it in?
Tom
Makefile | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/Makefile b/Makefile index d8f419bcd900..52cd6ea72161 100644 --- a/Makefile +++ b/Makefile @@ -811,6 +811,10 @@ ifneq ($(CONFIG_BUILD_TARGET),) ALL-y += $(CONFIG_BUILD_TARGET:"%"=%) endif
+ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ALL-y += init_sp_bss_offset_check +endif + LDFLAGS_u-boot += $(LDFLAGS_FINAL)
# Avoid 'Not enough room for program headers' error on binutils 2.28 onwards. @@ -939,6 +943,33 @@ binary_size_check: u-boot-nodtb.bin FORCE fi \ fi
+ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ifneq ($(CONFIG_SYS_MALLOC_F_LEN),) +subtract_sys_malloc_f_len = space=$$(($${space} - +$(CONFIG_SYS_MALLOC_F_LEN))) else subtract_sys_malloc_f_len = true +endif # The 1/4 margin below is somewhat arbitrary. The likely initial +SP usage is # so low that the DTB could probably use 90%+ of the +available space, for # current values of CONFIG_SYS_INIT_SP_BSS_OFFSET +at least. However, let's be # safe for now and tweak this later if +space becomes tight. +# A rejected alternative would be to check that some absolute minimum +stack # space was available. However, since +CONFIG_SYS_INIT_SP_BSS_OFFSET is # deliberately build-specific, to take +account of build-to-build stack usage # differences due to different +feature sets, there is no common absolute value # to check against. +init_sp_bss_offset_check: u-boot.dtb FORCE + @dtb_size=$(shell wc -c u-boot.dtb | awk '{print $$1}') ; \ + space=$(CONFIG_SYS_INIT_SP_BSS_OFFSET) ; \ + $(subtract_sys_malloc_f_len) ; \ + quarter_space=$$(($${space} / 4)) ; \ + if [ $${dtb_size} -gt $${quarter_space} ]; then \ + echo "u-boot.dtb is larger than 1 quarter of " >&2 ; \ + echo "(CONFIG_SYS_INIT_SP_BSS_OFFSET - CONFIG_SYS_MALLOC_F_LEN)" >&2 ; \ + exit 1 ; \ + fi +endif + u-boot-nodtb.bin: u-boot FORCE $(call if_changed,objcopy) $(call DO_STATIC_RELA,$<,$@,$(CONFIG_SYS_TEXT_BASE)) -- 2.15.1 -- nvpublic
On 01/10/2018 02:56 PM, Tom Warren wrote:
Stephen Warren wrote at Tuesday, January 9, 2018 12:52 PM:
With CONFIG_SYS_INIT_SP_BSS_OFFSET enabled, the initial (pre-relocation) stack is placed some distance after bss_start. The control DTB is appended to the U-Boot binary at bss_start. If the DTB is too large, or the SP BSS offset too small, then the initial stack could corrupt the DTB. Enhance the Makefile to check whether this is likely to occur.
Signed-off-by: Stephen Warren swarren@nvidia.com --- This builds on top of my previous patch "ARMv8: Allow dynamic early stack pointer". However, since all the logic is conditional and only activated if CONFIG_SYS_INIT_SP_BSS_OFFSET is defined, it can be applied with or without that other patch. It'd make sense to apply it afterwards and in the same branch though, or the change won't make a lot of sense to someone reading history in order.
I've got this in my TOT u-boot-tegra/master, ready to send a PR to TomR. But it's delegated to him in Patchwork. Do you want me to remove it from my PR, or leave it in?
From my perspective, either way is fine. Since you've already applied it, you may as well include it in your pull request.
Hi Stephen,
On 9 January 2018 at 11:52, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
With CONFIG_SYS_INIT_SP_BSS_OFFSET enabled, the initial (pre-relocation) stack is placed some distance after bss_start. The control DTB is appended to the U-Boot binary at bss_start. If the DTB is too large, or the SP BSS offset too small, then the initial stack could corrupt the DTB. Enhance the Makefile to check whether this is likely to occur.
Signed-off-by: Stephen Warren swarren@nvidia.com
This builds on top of my previous patch "ARMv8: Allow dynamic early stack pointer". However, since all the logic is conditional and only activated if CONFIG_SYS_INIT_SP_BSS_OFFSET is defined, it can be applied with or without that other patch. It'd make sense to apply it afterwards and in the same branch though, or the change won't make a lot of sense to someone reading history in order.
Makefile | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/Makefile b/Makefile index d8f419bcd900..52cd6ea72161 100644 --- a/Makefile +++ b/Makefile @@ -811,6 +811,10 @@ ifneq ($(CONFIG_BUILD_TARGET),) ALL-y += $(CONFIG_BUILD_TARGET:"%"=%) endif
+ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ALL-y += init_sp_bss_offset_check +endif
LDFLAGS_u-boot += $(LDFLAGS_FINAL)
# Avoid 'Not enough room for program headers' error on binutils 2.28 onwards. @@ -939,6 +943,33 @@ binary_size_check: u-boot-nodtb.bin FORCE fi \ fi
+ifneq ($(CONFIG_SYS_INIT_SP_BSS_OFFSET),) +ifneq ($(CONFIG_SYS_MALLOC_F_LEN),) +subtract_sys_malloc_f_len = space=$$(($${space} - $(CONFIG_SYS_MALLOC_F_LEN))) +else +subtract_sys_malloc_f_len = true +endif +# The 1/4 margin below is somewhat arbitrary. The likely initial SP usage is +# so low that the DTB could probably use 90%+ of the available space, for +# current values of CONFIG_SYS_INIT_SP_BSS_OFFSET at least. However, let's be +# safe for now and tweak this later if space becomes tight. +# A rejected alternative would be to check that some absolute minimum stack +# space was available. However, since CONFIG_SYS_INIT_SP_BSS_OFFSET is +# deliberately build-specific, to take account of build-to-build stack usage +# differences due to different feature sets, there is no common absolute value +# to check against. +init_sp_bss_offset_check: u-boot.dtb FORCE
@dtb_size=$(shell wc -c u-boot.dtb | awk '{print $$1}') ; \space=$(CONFIG_SYS_INIT_SP_BSS_OFFSET) ; \$(subtract_sys_malloc_f_len) ; \quarter_space=$$(($${space} / 4)) ; \if [ $${dtb_size} -gt $${quarter_space} ]; then \echo "u-boot.dtb is larger than 1 quarter of " >&2 ; \echo "(CONFIG_SYS_INIT_SP_BSS_OFFSET - CONFIG_SYS_MALLOC_F_LEN)" >&2 ; \exit 1 ; \fi+endif
u-boot-nodtb.bin: u-boot FORCE $(call if_changed,objcopy) $(call DO_STATIC_RELA,$<,$@,$(CONFIG_SYS_TEXT_BASE)) -- 2.15.1
Looking at the code here I wonder if it would be easy to check this in binman?
Regards, Simon
On 01/10/2018 04:04 PM, Simon Glass wrote:
On 9 January 2018 at 11:52, Stephen Warren swarren@wwwdotorg.org wrote:
With CONFIG_SYS_INIT_SP_BSS_OFFSET enabled, the initial (pre-relocation) stack is placed some distance after bss_start. The control DTB is appended to the U-Boot binary at bss_start. If the DTB is too large, or the SP BSS offset too small, then the initial stack could corrupt the DTB. Enhance the Makefile to check whether this is likely to occur.
...
Looking at the code here I wonder if it would be easy to check this in binman?
Well, considering it's already implemented in the Makefile, and quite simply, I'd have to say no:-)
participants (3)
-
Simon Glass -
Stephen Warren -
Tom Warren