[edk2] ArmPlatformPkg/DS-5: fix 64-bit PE/COFF header parsing bug

Message ID 1459409803-15099-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit a8c39ba2986c1ffb1dab864cefedc86402a9695c
Headers show

Commit Message

Ard Biesheuvel March 31, 2016, 7:36 a.m.
The 64-bit version of the DS-5 debug script that retrieves the debug file
path from the PE/COFF image in memory assumes that the PE/COFF header is
packed, and that the debug directory entry in the optional header appears
at a fixed offset into file. This is no longer true, now that we pad
between the file header and the PE header if the section alignment exceeds
the size of the header (which may be the case when the module contains a
vector table or small model code, which requires 2 KB or 4 KB section
alignment, respectively), to allow this padding to be emitted if the image
is subsequently converted to TE format.

So replace the fixed offset with a dereference of the appropriate header
field.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reported-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
---
 ArmPlatformPkg/Scripts/Ds5/firmware_volume.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel April 8, 2016, 11:26 a.m. | #1
Ping?

On 31 March 2016 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The 64-bit version of the DS-5 debug script that retrieves the debug file

> path from the PE/COFF image in memory assumes that the PE/COFF header is

> packed, and that the debug directory entry in the optional header appears

> at a fixed offset into file. This is no longer true, now that we pad

> between the file header and the PE header if the section alignment exceeds

> the size of the header (which may be the case when the module contains a

> vector table or small model code, which requires 2 KB or 4 KB section

> alignment, respectively), to allow this padding to be emitted if the image

> is subsequently converted to TE format.

>

> So replace the fixed offset with a dereference of the appropriate header

> field.

>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Reported-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>

> ---

>  ArmPlatformPkg/Scripts/Ds5/firmware_volume.py | 5 ++---

>  1 file changed, 2 insertions(+), 3 deletions(-)

>

> diff --git a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

> index c434e3de19da..9a76ae066d9a 100644

> --- a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

> +++ b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

> @@ -138,11 +138,10 @@ class EfiSectionPE64:

>

>      def get_debug_filepath(self):

>          # Offset from dos hdr to PE file hdr (EFI_IMAGE_NT_HEADERS64)

> -        #file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)

> -        file_header_offset = 0x0

> +        file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)

>

>          # Offset to debug dir in PE hdrs

> -        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0x138)

> +        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0xB8)

>          if debug_dir_entry_rva == 0:

>              raise Exception("EfiFileSectionPE64","No Debug Directory")

>

> --

> 2.5.0

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 14, 2016, 12:10 p.m. | #2
Pretty ping?

On 8 April 2016 at 13:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Ping?

>

> On 31 March 2016 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> The 64-bit version of the DS-5 debug script that retrieves the debug file

>> path from the PE/COFF image in memory assumes that the PE/COFF header is

>> packed, and that the debug directory entry in the optional header appears

>> at a fixed offset into file. This is no longer true, now that we pad

>> between the file header and the PE header if the section alignment exceeds

>> the size of the header (which may be the case when the module contains a

>> vector table or small model code, which requires 2 KB or 4 KB section

>> alignment, respectively), to allow this padding to be emitted if the image

>> is subsequently converted to TE format.

>>

>> So replace the fixed offset with a dereference of the appropriate header

>> field.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Reported-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>

>> ---

>>  ArmPlatformPkg/Scripts/Ds5/firmware_volume.py | 5 ++---

>>  1 file changed, 2 insertions(+), 3 deletions(-)

>>

>> diff --git a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

>> index c434e3de19da..9a76ae066d9a 100644

>> --- a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

>> +++ b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

>> @@ -138,11 +138,10 @@ class EfiSectionPE64:

>>

>>      def get_debug_filepath(self):

>>          # Offset from dos hdr to PE file hdr (EFI_IMAGE_NT_HEADERS64)

>> -        #file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)

>> -        file_header_offset = 0x0

>> +        file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)

>>

>>          # Offset to debug dir in PE hdrs

>> -        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0x138)

>> +        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0xB8)

>>          if debug_dir_entry_rva == 0:

>>              raise Exception("EfiFileSectionPE64","No Debug Directory")

>>

>> --

>> 2.5.0

>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm April 14, 2016, 12:57 p.m. | #3
If this fixes a bug, by all means go ahead.
Not sure I could honestly say I've reviewed it.

Could Bernie (cc) have a look?

/
    Leif

On Thu, Apr 14, 2016 at 02:10:18PM +0200, Ard Biesheuvel wrote:
> Pretty ping?

> 

> On 8 April 2016 at 13:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > Ping?

> >

> > On 31 March 2016 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >> The 64-bit version of the DS-5 debug script that retrieves the debug file

> >> path from the PE/COFF image in memory assumes that the PE/COFF header is

> >> packed, and that the debug directory entry in the optional header appears

> >> at a fixed offset into file. This is no longer true, now that we pad

> >> between the file header and the PE header if the section alignment exceeds

> >> the size of the header (which may be the case when the module contains a

> >> vector table or small model code, which requires 2 KB or 4 KB section

> >> alignment, respectively), to allow this padding to be emitted if the image

> >> is subsequently converted to TE format.

> >>

> >> So replace the fixed offset with a dereference of the appropriate header

> >> field.

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >> Reported-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>

> >> ---

> >>  ArmPlatformPkg/Scripts/Ds5/firmware_volume.py | 5 ++---

> >>  1 file changed, 2 insertions(+), 3 deletions(-)

> >>

> >> diff --git a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

> >> index c434e3de19da..9a76ae066d9a 100644

> >> --- a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

> >> +++ b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py

> >> @@ -138,11 +138,10 @@ class EfiSectionPE64:

> >>

> >>      def get_debug_filepath(self):

> >>          # Offset from dos hdr to PE file hdr (EFI_IMAGE_NT_HEADERS64)

> >> -        #file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)

> >> -        file_header_offset = 0x0

> >> +        file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)

> >>

> >>          # Offset to debug dir in PE hdrs

> >> -        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0x138)

> >> +        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0xB8)

> >>          if debug_dir_entry_rva == 0:

> >>              raise Exception("EfiFileSectionPE64","No Debug Directory")

> >>

> >> --

> >> 2.5.0

> >>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py
index c434e3de19da..9a76ae066d9a 100644
--- a/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py
+++ b/ArmPlatformPkg/Scripts/Ds5/firmware_volume.py
@@ -138,11 +138,10 @@  class EfiSectionPE64:
 
     def get_debug_filepath(self):
         # Offset from dos hdr to PE file hdr (EFI_IMAGE_NT_HEADERS64)
-        #file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)
-        file_header_offset = 0x0
+        file_header_offset = self.ec.getMemoryService().readMemory32(self.base_pe64 + 0x3C)
 
         # Offset to debug dir in PE hdrs
-        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0x138)
+        debug_dir_entry_rva = self.ec.getMemoryService().readMemory32(self.base_pe64 + file_header_offset + 0xB8)
         if debug_dir_entry_rva == 0:
             raise Exception("EfiFileSectionPE64","No Debug Directory")