Re: [edk2-devel] : Query regarding IsTextShdr inside Basetools

Re: [edk2-devel] : Query regarding IsTextShdr inside Basetools

[Mukesh Ojha]

Apology for rushing into this.
Looking for a quick input on this.

-Mukesh

On 11/10/2020 6:37 PM, Mukesh Ojha wrote:
> Hi All,
>
> I have a doubt about the check we have put inside IsTextShdr() .
>
> STATIC
> BOOLEAN
> IsTextShdr (
>   Elf_Shdr *Shdr
>   )
> {
>   return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == 
> SHF_ALLOC);
> }
>
>
> We are observing one issue where while generate EFI using GenFW in 
> EDK2 because test/data section offset is different than calculated 
> mCoffSectionsOffset when scanning sections.
> I run GenFW with a failure dll in my local after adding some logs into 
> GenFW. and found that “mCoffSectionsOffset” for data ection seems not 
> to have expected value due to
> “.note.gnu.property” size. Because compiled dll has 
> “.note.gnu.property” section with alloc flag and GenFW thinks that 
> it’s a text section if alloc flag is set.
> So its size is added to the mCoffSectionsOffset.
>
> Could you please give us an advice whether we can fix IsTextShdr() 
> function like below ?
>
>
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -229,7 +229,7 @@ IsTextShdr (
>    Elf_Shdr *Shdr
>    )
> {
> -  return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == 
> SHF_ALLOC);
> +  return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE | 
> SHF_ALLOC)) == (SHF_ALLOC | SHF_EXECINSTR));^
>
>
> Thanks,
> Mukesh
>
>
> 
>
>

[edk2-devel] : Query regarding IsTextShdr inside Basetools

[mojha]

Hi All,

I have a doubt about the check we have put inside IsTextShdr() .

STATIC
BOOLEAN
IsTextShdr (
   Elf_Shdr *Shdr
   )
{
   return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == 
SHF_ALLOC);
}


We are observing one issue where while generate EFI using GenFW in EDK2 
because test/data section offset is different than calculated 
mCoffSectionsOffset when scanning sections.
I run GenFW with a failure dll in my local after adding some logs into 
GenFW. and found that “mCoffSectionsOffset” for data section seems not 
to have expected value due to
“.note.gnu.property” size. Because compiled dll has “.note.gnu.property” 
section with alloc flag and GenFW thinks that it’s a text section if 
alloc flag is set.
So its size is added to the mCoffSectionsOffset.

Could you please give us an advice whether we can fix IsTextShdr() 
function like below ?


--- a/BaseTools/Source/C/GenFw/Elf64Convert.c
+++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
@@ -229,7 +229,7 @@ IsTextShdr (
    Elf_Shdr *Shdr
    )
{
-  return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) == 
SHF_ALLOC);
+  return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE | 
SHF_ALLOC)) == (SHF_ALLOC | SHF_EXECINSTR));^


Thanks,
Mukesh

Re: [edk2-devel] : Query regarding IsTextShdr inside Basetools

[Laszlo Ersek]

Ard, Liming,

can you please take a look?

Thanks!
Laszlo

On 11/10/20 14:07, Mukesh Ojha wrote:
> Hi All,
> 
> I have a doubt about the check we have put inside IsTextShdr() .
> 
> STATIC
> BOOLEAN
> IsTextShdr (
>   Elf_Shdr *Shdr
>   )
> {
>   return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) ==
> SHF_ALLOC);
> }
> 
> 
> We are observing one issue where while generate EFI using GenFW in EDK2
> because test/data section offset is different than calculated
> mCoffSectionsOffset when scanning sections.
> I run GenFW with a failure dll in my local after adding some logs into
> GenFW. and found that “mCoffSectionsOffset” for data section seems not
> to have expected value due to
> “.note.gnu.property” size. Because compiled dll has “.note.gnu.property”
> section with alloc flag and GenFW thinks that it’s a text section if
> alloc flag is set.
> So its size is added to the mCoffSectionsOffset.
> 
> Could you please give us an advice whether we can fix IsTextShdr()
> function like below ?
> 
> 
> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
> @@ -229,7 +229,7 @@ IsTextShdr (
>    Elf_Shdr *Shdr
>    )
> {
> -  return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) ==
> SHF_ALLOC);
> +  return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE |
> SHF_ALLOC)) == (SHF_ALLOC | SHF_EXECINSTR));^
> 
> 
> Thanks,
> Mukesh
> 
> 
> 
> 
> 

Re: [edk2-devel] : Query regarding IsTextShdr inside Basetools

[Laszlo Ersek]

On 11/11/20 23:40, Laszlo Ersek wrote:
> Ard, Liming,
> 
> can you please take a look?
> 
> Thanks!
> Laszlo

Darn, I used Liming's old email address. Correcting it now. Sorry!

Laszlo

> 
> On 11/10/20 14:07, Mukesh Ojha wrote:
>> Hi All,
>>
>> I have a doubt about the check we have put inside IsTextShdr() .
>>
>> STATIC
>> BOOLEAN
>> IsTextShdr (
>>   Elf_Shdr *Shdr
>>   )
>> {
>>   return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) ==
>> SHF_ALLOC);
>> }
>>
>>
>> We are observing one issue where while generate EFI using GenFW in EDK2
>> because test/data section offset is different than calculated
>> mCoffSectionsOffset when scanning sections.
>> I run GenFW with a failure dll in my local after adding some logs into
>> GenFW. and found that “mCoffSectionsOffset” for data section seems not
>> to have expected value due to
>> “.note.gnu.property” size. Because compiled dll has “.note.gnu.property”
>> section with alloc flag and GenFW thinks that it’s a text section if
>> alloc flag is set.
>> So its size is added to the mCoffSectionsOffset.
>>
>> Could you please give us an advice whether we can fix IsTextShdr()
>> function like below ?
>>
>>
>> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>> @@ -229,7 +229,7 @@ IsTextShdr (
>>    Elf_Shdr *Shdr
>>    )
>> {
>> -  return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) ==
>> SHF_ALLOC);
>> +  return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE |
>> SHF_ALLOC)) == (SHF_ALLOC | SHF_EXECINSTR));^
>>
>>
>> Thanks,
>> Mukesh
>>
>>
>> 
>>
>>
> 

Re: [edk2-devel] : Query regarding IsTextShdr inside Basetools

[Ard Biesheuvel]

On 11/11/20 11:41 PM, Laszlo Ersek wrote:
> On 11/11/20 23:40, Laszlo Ersek wrote:
>> Ard, Liming,
>>
>> can you please take a look?
>>
>> Thanks!
>> Laszlo
> 
> Darn, I used Liming's old email address. Correcting it now. Sorry!
> 
> Laszlo
> 
>>
>> On 11/10/20 14:07, Mukesh Ojha wrote:
>>> Hi All,
>>>
>>> I have a doubt about the check we have put inside IsTextShdr() .
>>>
>>> STATIC
>>> BOOLEAN
>>> IsTextShdr (
>>>    Elf_Shdr *Shdr
>>>    )
>>> {
>>>    return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) ==
>>> SHF_ALLOC);
>>> }
>>>
>>>
>>> We are observing one issue where while generate EFI using GenFW in EDK2
>>> because test/data section offset is different than calculated
>>> mCoffSectionsOffset when scanning sections.
>>> I run GenFW with a failure dll in my local after adding some logs into
>>> GenFW. and found that “mCoffSectionsOffset” for data section seems not
>>> to have expected value due to
>>> “.note.gnu.property” size. Because compiled dll has “.note.gnu.property”
>>> section with alloc flag and GenFW thinks that it’s a text section if
>>> alloc flag is set.
>>> So its size is added to the mCoffSectionsOffset.
>>>
>>> Could you please give us an advice whether we can fix IsTextShdr()
>>> function like below ?
>>>
>>>
>>> --- a/BaseTools/Source/C/GenFw/Elf64Convert.c
>>> +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c
>>> @@ -229,7 +229,7 @@ IsTextShdr (
>>>     Elf_Shdr *Shdr
>>>     )
>>> {
>>> -  return (BOOLEAN) ((Shdr->sh_flags & (SHF_WRITE | SHF_ALLOC)) ==
>>> SHF_ALLOC);
>>> +  return (BOOLEAN) ((Shdr->sh_flags & (SHF_EXECINSTR | SHF_WRITE |
>>> SHF_ALLOC)) == (SHF_ALLOC | SHF_EXECINSTR));^
>>>


Was this ELF executable built using the GccBase.lds linker script? If 
so, we should fix it to disregard .note sections.

If you are not using GccBase.lds, I'm afraid you are simply in 
unsupported territory - there are too many assumptions in GenFw that are 
not guaranteed to hold for arbitrary ELF executables.

I don't think changing IsTextShdr() is the right approach here.