diff mbox series

objcopy: check input flavor before setting PE/COFF section alignment

Message ID e40a8e36-e79a-47df-8af4-4936e03752b6@suse.com
State New
Headers
Series objcopy: check input flavor before setting PE/COFF section alignment |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed

Commit Message

Jan Beulich April 19, 2024, 9:27 a.m. UTC 
  coff_section_data() and elf_section_data() use the same underlying
field. The pointer being non-NULL therefore isn't sufficient to know
that pei_section_data() can validly be used on the incoming object.
Apparently in 64-bit-host builds the resulting memory corruption is
benign, whereas in 32-bit-host builds a segmentation fault occurs upon
de-referencing pei_section_data()'s return value.
---
Of course the value (first) being set on the input bfd is suspicious
in the first place: When copying e.g. ELF to PE/COFF, the option ought
to be similarly respected, yet clearly it can't be set like this then on
the incoming object. The change here is merely to address the testsuite
failures observed for Arm64 and RISC-V ("Check if efi app format is
recognized") as well as the (latent) memory corruption.

Comments

Nick Clifton April 19, 2024, 10:37 a.m. UTC | #1 
Hi Jan,

> coff_section_data() and elf_section_data() use the same underlying
> field. The pointer being non-NULL therefore isn't sufficient to know
> that pei_section_data() can validly be used on the incoming object.
> Apparently in 64-bit-host builds the resulting memory corruption is
> benign, whereas in 32-bit-host builds a segmentation fault occurs upon
> de-referencing pei_section_data()'s return value.
> ---
> Of course the value (first) being set on the input bfd is suspicious
> in the first place: When copying e.g. ELF to PE/COFF, the option ought
> to be similarly respected, yet clearly it can't be set like this then on
> the incoming object. The change here is merely to address the testsuite
> failures observed for Arm64 and RISC-V ("Check if efi app format is
> recognized") as well as the (latent) memory corruption.

Thanks for fixing my oversight!

Cheers
   Nick
Jan Beulich April 22, 2024, 7:15 a.m. UTC | #2 
On 19.04.2024 12:37, Nick Clifton wrote:
>> coff_section_data() and elf_section_data() use the same underlying
>> field. The pointer being non-NULL therefore isn't sufficient to know
>> that pei_section_data() can validly be used on the incoming object.
>> Apparently in 64-bit-host builds the resulting memory corruption is
>> benign, whereas in 32-bit-host builds a segmentation fault occurs upon
>> de-referencing pei_section_data()'s return value.
>> ---
>> Of course the value (first) being set on the input bfd is suspicious
>> in the first place: When copying e.g. ELF to PE/COFF, the option ought
>> to be similarly respected, yet clearly it can't be set like this then on
>> the incoming object. The change here is merely to address the testsuite
>> failures observed for Arm64 and RISC-V ("Check if efi app format is
>> recognized") as well as the (latent) memory corruption.
> 
> Thanks for fixing my oversight!

Well, before putting it in - any thoughts on the post-commit-message remark
above? Is it really meant to stay the way of the input bfd's data is being
altered, rather than keeping that intact and fiddling only with the output?
And thus - afaict - rendering the command line option (silently) useless
when copying ELF to PE?

Jan
Nick Clifton May 8, 2024, 10:26 a.m. UTC | #3 
Hi Jan,

>>> Of course the value (first) being set on the input bfd is suspicious
>>> in the first place: When copying e.g. ELF to PE/COFF, the option ought
>>> to be similarly respected, yet clearly it can't be set like this then on
>>> the incoming object. The change here is merely to address the testsuite
>>> failures observed for Arm64 and RISC-V ("Check if efi app format is
>>> recognized") as well as the (latent) memory corruption.

> Well, before putting it in - any thoughts on the post-commit-message remark
> above?

Sorry.  Well in the first place converting from ELF to PE is always going to
be a difficult process.  So trying to combine it with adjustments to other
properties, such as alignment, is just asking for trouble.  My feeling therefore
is that we ought to be have a warning in the documentation telling users
to be careful and maybe take things one step at a time.

> Is it really meant to stay the way of the input bfd's data is being
> altered, rather than keeping that intact and fiddling only with the output?
> And thus - afaict - rendering the command line option (silently) useless
> when copying ELF to PE?

You are right.  For this particular case maybe we should add a test for
ELF->PE format changing and refuse to adjust the alignments.

Cheers
   Nick
diff mbox series

Patch

--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -4310,6 +4310,7 @@  setup_section (bfd *ibfd, sec_ptr isecti
   if (p != NULL)
     alignment = p->alignment;
   else if (pe_section_alignment != (bfd_vma) -1
+	   && bfd_get_flavour (ibfd) == bfd_target_coff_flavour
 	   && bfd_get_flavour (obfd) == bfd_target_coff_flavour)
     {
       alignment = power_of_two (pe_section_alignment);