Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mcuboot: force .hex and .bin same content #56219

Closed
wants to merge 1 commit into from
Closed

mcuboot: force .hex and .bin same content #56219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Kconfig.zephyr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,16 @@ config MCUBOOT_GENERATE_CONFIRMED_IMAGE
The existence of bin and hex files depends on CONFIG_BUILD_OUTPUT_BIN
and CONFIG_BUILD_OUTPUT_HEX.

config MCUBOOT_GENERATED_HEX_BIN_SAME_CONTENT
bool "Ensure that generated .hex and .bin files have the same logical content"
default y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite honestly I don't think the majority of people care about this, some yes but the minority.

depends on BUILD_OUTPUT_HEX && BUILD_OUTPUT_BIN
help
Enabling this configuration ensures that the signed hex and binary files generated
by the signing tool have the same logical content. Disabling this option will
result in the hex and binary files having different image signatures due to the
multiple calls to the signing tool.

endif # BOOTLOADER_MCUBOOT

config BOOTLOADER_ESP_IDF
Expand Down
36 changes: 36 additions & 0 deletions cmake/mcuboot.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@ function(zephyr_runner_file type path)
set_target_properties(runners_yaml_props_target PROPERTIES "${type}_file" "${path}")
endfunction()

function(zephyr_mcuboot_hex_bin_convert hex_file bin_file)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a transferable way of doing this and requires use of this specific file to achieve this (and this file can now be completely replaced out of tree), would rather something generic is added to the cmake code that can convert any hex to bin (or just use objcopy) and can be called as is needed

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would rather something generic is added to the cmake code that can convert any hex to bin (or just use objcopy) and can be called as is needed

@nordicjm not sure exactly what you mean by this ?
The use of the elfconvert_command target property from bintools is the way toolchains can specify how files can be converted, and is used also by Zephyr in many places, for example here and elsewhere in that file:

zephyr/CMakeLists.txt

Line 1175 in ccd88a4

COMMAND $<TARGET_PROPERTY:bintools,elfconvert_command>

if you are requesting a common CMake function to the this, but how the current function is implemented is acceptable, then I have mis-read your comment, and I apologize the noise ?

or just use objcopy

not all toolchains support objcopy, which is why toolchains should setup their own elfconvert_command property, which is objcopy for gnu.
Ref:

zephyr/cmake/bintools/gnu/target_bintools.cmake

Lines 23 to 25 in ccd88a4

# elfconvert to use for transforming an elf file into another format,
# such as intel hex, s-rec, binary, etc.
set_property(TARGET bintools PROPERTY elfconvert_command ${CMAKE_OBJCOPY})

zephyr/cmake/bintools/arcmwdt/target_bintools.cmake

Lines 12 to 15 in ccd88a4

# MWDT toolchain does not support all options in a single command
# Therefore a CMake script is used, so that multiple commands can be executed
# successively.
set_property(TARGET bintools PROPERTY elfconvert_command ${CMAKE_COMMAND})

set_property(GLOBAL APPEND PROPERTY extra_post_build_commands COMMAND
$<TARGET_PROPERTY:bintools,elfconvert_command>
$<TARGET_PROPERTY:bintools,elfconvert_flag>
${GAP_FILL}
$<TARGET_PROPERTY:bintools,elfconvert_flag_intarget>ihex
$<TARGET_PROPERTY:bintools,elfconvert_flag_outtarget>binary
$<TARGET_PROPERTY:bintools,elfconvert_flag_infile>${hex_file}
$<TARGET_PROPERTY:bintools,elfconvert_flag_outfile>${bin_file}
$<TARGET_PROPERTY:bintools,elfconvert_flag_final>
)
endfunction()

function(zephyr_mcuboot_tasks)
set(keyfile "${CONFIG_MCUBOOT_SIGNATURE_KEY_FILE}")
set(keyfile_enc "${CONFIG_MCUBOOT_ENCRYPTION_KEY_FILE}")
Expand Down Expand Up @@ -158,6 +171,29 @@ function(zephyr_mcuboot_tasks)
${west_sign} ${encrypted_args} ${imgtool_args} --encrypt "${keyfile_enc}")
endif()
set_property(GLOBAL APPEND PROPERTY extra_post_build_byproducts ${byproducts})

# Regenerate the binary files from the hex files so that the content of both is
# the same. Otherwise the .hex and .bin will have different signatures due to the
# independent calls to the signing tool. This cannot be done as part of 'west sign'
# as we cannot evaluate the bintools arguments at configuration time.
if (CONFIG_MCUBOOT_GENERATED_HEX_BIN_SAME_CONTENT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of introducing yet another Kconfig setting, then why no swap the code here:

zephyr/cmake/mcuboot.cmake

Line 110 in ccd88a4

if(CONFIG_BUILD_OUTPUT_BIN)

and here:

zephyr/cmake/mcuboot.cmake

Line 127 in ccd88a4

if(CONFIG_BUILD_OUTPUT_HEX)

so that order becomes:

  if(CONFIG_BUILD_OUTPUT_HEX)
    # Do signing of hex file
  endif()

  if(CONFIG_BUILD_OUTPUT_BIN AND CONFIG_BUILD_OUTPUT_HEX)
    # Both hex and bin output is enabled, convert signed hex to signed bin to mitigate #52271.
  elseif(CONFIG_BUILD_OUTPUT_BIN)
    # Only bin output is enabled, keep existing image bin signing.
  endif()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this approach as west sign current automatically discovers files to be signed, so even without the bin options it still actually signs the .bin. If this file is generated anyway the easiest option IMO is just to overwrite it. Obviously this can be updated but is a larger change.

zephyr/scripts/west_commands/sign.py

Lines 155 to 175 in fd94ca7

# Decide on output formats.
formats = []
bin_exists = build_conf.getboolean('CONFIG_BUILD_OUTPUT_BIN')
if args.gen_bin:
self.check_force(bin_exists,
'--bin given but CONFIG_BUILD_OUTPUT_BIN not set '
"in build directory's ({}) .config".
format(build_dir))
formats.append('bin')
elif args.gen_bin is None and bin_exists:
formats.append('bin')
hex_exists = build_conf.getboolean('CONFIG_BUILD_OUTPUT_HEX')
if args.gen_hex:
self.check_force(hex_exists,
'--hex given but CONFIG_BUILD_OUTPUT_HEX not set '
"in build directory's ({}) .config".
format(build_dir))
formats.append('hex')
elif args.gen_hex is None and hex_exists:
formats.append('hex')

I didn't go with if(CONFIG_BUILD_OUTPUT_BIN AND CONFIG_BUILD_OUTPUT_HEX) directly in the cmake just to provide the option to disable this behaviour if it happens to cause issues for someone.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with this approach as west sign current automatically discovers files to be signed

Right, I had forgotten about the pure west sign behavior.
Having different behavior on west sign and during build is not good.

I didn't go with if(CONFIG_BUILD_OUTPUT_BIN AND CONFIG_BUILD_OUTPUT_HEX) directly in the cmake just to provide the option to disable this behaviour if it happens to cause issues for someone.

and that's a fair point.
But I dislike having an extra Kconfig for this.
and even though it's configurable, there is still a mismatch to how it works when west sign is invoked.

zephyr_mcuboot_hex_bin_convert(
${output}.signed.hex
${output}.signed.bin
)
if(confirmed_args)
zephyr_mcuboot_hex_bin_convert(
${output}.signed.confirmed.hex
${output}.signed.confirmed.bin
)
endif()
if(encrypted_args)
zephyr_mcuboot_hex_bin_convert(
${output}.signed.encrypted.hex
${output}.signed.encrypted.bin
)
endif()
endif()
endfunction()

zephyr_mcuboot_tasks()