[PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Ard Biesheuvel]

Currently, the PE/COFF conversion routines in GenFw add a so-called
NB10 CodeView debug record to the image, and update the associated
directory entry in the PE/COFF optional header to contain its relative
virtual address (RVA) and size.

However, there are two levels of indirection at work here: the actual
NB10 CodeView record (which is simply a magic number and some unused
data fields followed by the NUL terminated filename) is emitted
separately, and a separate descriptor is emitted that identifies the
NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and records
its size. The directory entry in the PE/COFF optional header should
refer to this intermediate descriptor's address and size only, but
the WriteDebug## () routines in GenFw erroneously record the size of
both the descriptor and the NB10 CodeView record.

This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
GenFw to clear unused debug entry generated by VS tool chain",
2017-06-19), and GenFw now crashes when it attempts to iterate over
what it thinks are multiple intermediate descriptors for different
kinds of debug data embedded in the image.

The error is understandable, given that both are carved out of the
same file space allocation, but this is really an implementation detail
of GenFw, and is not required. (Note that the intermediate descriptor
does not require a RVA and so it does not even need to be inside a
section)

So omit the size of the NB10 CodeView record from the size recorded
in the optional header.

Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Co-debugged-or-whatever-by: Laszlo Ersek <lersek@redhat.com>
---
 BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
 BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
index f7b084dc9b84..14fe4a285857 100644
--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
@@ -1142,7 +1142,7 @@ WriteDebug32 (
   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
   DataDir = &NtHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
   DataDir->VirtualAddress = mDebugOffset;
-  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
 }
 
 STATIC
diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
index 7eed7b92d30f..c39bdff063ab 100644
--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -1095,7 +1095,7 @@ WriteDebug64 (
   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
   DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
   DataDir->VirtualAddress = mDebugOffset;
-  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
 }
 
 STATIC
-- 
2.9.3

Re: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Laszlo Ersek]

On 07/05/17 20:33, Ard Biesheuvel wrote:
> Currently, the PE/COFF conversion routines in GenFw add a so-called
> NB10 CodeView debug record to the image, and update the associated
> directory entry in the PE/COFF optional header to contain its relative
> virtual address (RVA) and size.
> 
> However, there are two levels of indirection at work here: the actual
> NB10 CodeView record (which is simply a magic number and some unused
> data fields followed by the NUL terminated filename) is emitted
> separately, and a separate descriptor is emitted that identifies the
> NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and records
> its size. The directory entry in the PE/COFF optional header should
> refer to this intermediate descriptor's address and size only, but
> the WriteDebug## () routines in GenFw erroneously record the size of
> both the descriptor and the NB10 CodeView record.
> 
> This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
> GenFw to clear unused debug entry generated by VS tool chain",
> 2017-06-19), and GenFw now crashes when it attempts to iterate over
> what it thinks are multiple intermediate descriptors for different
> kinds of debug data embedded in the image.
> 
> The error is understandable, given that both are carved out of the
> same file space allocation, but this is really an implementation detail
> of GenFw, and is not required. (Note that the intermediate descriptor
> does not require a RVA and so it does not even need to be inside a
> section)
> 
> So omit the size of the NB10 CodeView record from the size recorded
> in the optional header.
> 
> Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html

Please prepend:

Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012162.html

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Co-debugged-or-whatever-by: Laszlo Ersek <lersek@redhat.com>

Haha, great :)

> ---
>  BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
>  BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks, Ard!
Laszlo

> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c b/BaseTools/Source/C/GenFw/Elf32Convert.c
> index f7b084dc9b84..14fe4a285857 100644
> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
> @@ -1142,7 +1142,7 @@ WriteDebug32 (
>    NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>    DataDir = &NtHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>    DataDir->VirtualAddress = mDebugOffset;
> -  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>  }
>  
>  STATIC
> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c b/BaseTools/Source/C/GenFw/Elf64Convert.c
> index 7eed7b92d30f..c39bdff063ab 100644
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -1095,7 +1095,7 @@ WriteDebug64 (
>    NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile + mNtHdrOffset);
>    DataDir = &NtHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG];
>    DataDir->VirtualAddress = mDebugOffset;
> -  DataDir->Size = Dir->SizeOfData + sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>  }
>  
>  STATIC
> 

Re: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Gao, Liming]

Ard and Laszlo:
  Thanks for your quick fix. For my patch, I think it only impacts VS tool chain. So, I don't verify GCC tool chain. Sorry for it.  

Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Thursday, July 06, 2017 2:34 AM
>To: edk2-devel@lists.01.org; lersek@redhat.com
>Cc: leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Zhu,
>Yonghong <yonghong.zhu@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory
>entry size
>
>Currently, the PE/COFF conversion routines in GenFw add a so-called
>NB10 CodeView debug record to the image, and update the associated
>directory entry in the PE/COFF optional header to contain its relative
>virtual address (RVA) and size.
>
>However, there are two levels of indirection at work here: the actual
>NB10 CodeView record (which is simply a magic number and some unused
>data fields followed by the NUL terminated filename) is emitted
>separately, and a separate descriptor is emitted that identifies the
>NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and
>records
>its size. The directory entry in the PE/COFF optional header should
>refer to this intermediate descriptor's address and size only, but
>the WriteDebug## () routines in GenFw erroneously record the size of
>both the descriptor and the NB10 CodeView record.
>
>This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
>GenFw to clear unused debug entry generated by VS tool chain",
>2017-06-19), and GenFw now crashes when it attempts to iterate over
>what it thinks are multiple intermediate descriptors for different
>kinds of debug data embedded in the image.
>
>The error is understandable, given that both are carved out of the
>same file space allocation, but this is really an implementation detail
>of GenFw, and is not required. (Note that the intermediate descriptor
>does not require a RVA and so it does not even need to be inside a
>section)
>
>So omit the size of the NB10 CodeView record from the size recorded
>in the optional header.
>
>Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Co-debugged-or-whatever-by: Laszlo Ersek <lersek@redhat.com>
>---
> BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
> BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c
>b/BaseTools/Source/C/GenFw/Elf32Convert.c
>index f7b084dc9b84..14fe4a285857 100644
>--- a/BaseTools/Source/C/GenFw/Elf32Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
>@@ -1142,7 +1142,7 @@ WriteDebug32 (
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>mNtHdrOffset);
>   DataDir = &NtHdr-
>>Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
>];
>   DataDir->VirtualAddress = mDebugOffset;
>-  DataDir->Size = Dir->SizeOfData +
>sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
>
> STATIC
>diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>b/BaseTools/Source/C/GenFw/Elf64Convert.c
>index 7eed7b92d30f..c39bdff063ab 100644
>--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>@@ -1095,7 +1095,7 @@ WriteDebug64 (
>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>mNtHdrOffset);
>   DataDir = &NtHdr-
>>Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DE
>BUG];
>   DataDir->VirtualAddress = mDebugOffset;
>-  DataDir->Size = Dir->SizeOfData +
>sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>+  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
> }
>
> STATIC
>--
>2.9.3

Re: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory entry size

[Laszlo Ersek]

On 07/06/17 05:32, Gao, Liming wrote:
> Ard and Laszlo:
>   Thanks for your quick fix. For my patch, I think it only impacts VS tool chain. So, I don't verify GCC tool chain. Sorry for it.  
> 
> Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks Liming (and obviously: Ard), pushed as commit 60e85a39fe49.

Laszlo

>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Thursday, July 06, 2017 2:34 AM
>> To: edk2-devel@lists.01.org; lersek@redhat.com
>> Cc: leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Zhu,
>> Yonghong <yonghong.zhu@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Subject: [PATCH] BaseTools/GenFw: disregard payload in PE debug directory
>> entry size
>>
>> Currently, the PE/COFF conversion routines in GenFw add a so-called
>> NB10 CodeView debug record to the image, and update the associated
>> directory entry in the PE/COFF optional header to contain its relative
>> virtual address (RVA) and size.
>>
>> However, there are two levels of indirection at work here: the actual
>> NB10 CodeView record (which is simply a magic number and some unused
>> data fields followed by the NUL terminated filename) is emitted
>> separately, and a separate descriptor is emitted that identifies the
>> NB10 CodeView record as type EFI_IMAGE_DEBUG_TYPE_CODEVIEW, and
>> records
>> its size. The directory entry in the PE/COFF optional header should
>> refer to this intermediate descriptor's address and size only, but
>> the WriteDebug## () routines in GenFw erroneously record the size of
>> both the descriptor and the NB10 CodeView record.
>>
>> This problem was exposed by commit e4129b0e5897 ("BaseTools: Update
>> GenFw to clear unused debug entry generated by VS tool chain",
>> 2017-06-19), and GenFw now crashes when it attempts to iterate over
>> what it thinks are multiple intermediate descriptors for different
>> kinds of debug data embedded in the image.
>>
>> The error is understandable, given that both are carved out of the
>> same file space allocation, but this is really an implementation detail
>> of GenFw, and is not required. (Note that the intermediate descriptor
>> does not require a RVA and so it does not even need to be inside a
>> section)
>>
>> So omit the size of the NB10 CodeView record from the size recorded
>> in the optional header.
>>
>> Link: https://lists.01.org/pipermail/edk2-devel/2017-July/012181.html
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Co-debugged-or-whatever-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> BaseTools/Source/C/GenFw/Elf32Convert.c | 2 +-
>> BaseTools/Source/C/GenFw/Elf64Convert.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Source/C/GenFw/Elf32Convert.c
>> b/BaseTools/Source/C/GenFw/Elf32Convert.c
>> index f7b084dc9b84..14fe4a285857 100644
>> --- a/BaseTools/Source/C/GenFw/Elf32Convert.c
>> +++ b/BaseTools/Source/C/GenFw/Elf32Convert.c
>> @@ -1142,7 +1142,7 @@ WriteDebug32 (
>>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>> mNtHdrOffset);
>>   DataDir = &NtHdr-
>>> Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG
>> ];
>>   DataDir->VirtualAddress = mDebugOffset;
>> -  DataDir->Size = Dir->SizeOfData +
>> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> }
>>
>> STATIC
>> diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c
>> b/BaseTools/Source/C/GenFw/Elf64Convert.c
>> index 7eed7b92d30f..c39bdff063ab 100644
>> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>> @@ -1095,7 +1095,7 @@ WriteDebug64 (
>>   NtHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)(mCoffFile +
>> mNtHdrOffset);
>>   DataDir = &NtHdr-
>>> Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DE
>> BUG];
>>   DataDir->VirtualAddress = mDebugOffset;
>> -  DataDir->Size = Dir->SizeOfData +
>> sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> +  DataDir->Size = sizeof(EFI_IMAGE_DEBUG_DIRECTORY_ENTRY);
>> }
>>
>> STATIC
>> --
>> 2.9.3
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>