Re: [PATCH 3/5] elflint: Fix invalid type of relocation info and other issues on mips

["Ying Huang" <ying.huang@oss.cipunited.com>]

[-- Attachment #1: Type: text/plain, Size: 8226 bytes --]

Hi,

在 2023/5/11 23:59, Mark Wielaard 写道:
> Hi,
>
> On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote:
>> From: Ying Huang <ying.huang@oss.cipunited.com>
>>
>> add some check related functions
>> ---
>>  backends/mips_init.c      |  3 +++
>>  backends/mips_symbol.c    | 33 +++++++++++++++++++++++++++++++++
>>  libebl/eblrelocvaliduse.c |  8 ++++++--
>>  src/elflint.c             | 23 ++++++++++++++++++++---
>>  4 files changed, 62 insertions(+), 5 deletions(-)
>>
>> diff --git a/backends/mips_init.c b/backends/mips_init.c
>> index 5bba822b..4c2f21b9 100644
>> --- a/backends/mips_init.c
>> +++ b/backends/mips_init.c
>> @@ -51,6 +51,9 @@ mips_init (Elf *elf __attribute__ ((unused)),
>>    HOOK (eh, segment_type_name);
>>    HOOK (eh, dynamic_tag_check);
>>    HOOK (eh, dynamic_tag_name);
>> +  HOOK (eh, machine_section_flag_check);
>>    HOOK (eh, check_object_attribute);
>> +  HOOK (eh, check_special_symbol);
>> +  HOOK (eh, check_reloc_target_type);
>>    return eh;
>>  }
> OK. But see below for hooking reloc_valid_use

     OK, I would add hook reloc_valid_use.


>> +/* Check whether given symbol's st_value and st_size are OK despite failing
>> +   normal checks.  */
>> +bool
>> +mips_check_special_symbol (Elf *elf,
>> +			    const GElf_Sym *sym __attribute__ ((unused)),
>> +			    const char *name __attribute__ ((unused)),
>> +			    const GElf_Shdr *destshdr)
>> +{
>> +  size_t shstrndx;
>> +  if (elf_getshdrstrndx (elf, &shstrndx) != 0)
>> +    return false;
>> +  const char *sname = elf_strptr (elf, shstrndx, destshdr->sh_name);
>> +  if (sname == NULL)
>> +    return false;
>> +  return (strcmp (sname, ".got") == 0 || strcmp (sname, ".bss") == 0);
>> +}
> Could you add a comment why .got and .bss are special in this case?

    raw code:

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint  src/nm
    section [38] '.symtab': symbol 781 (_gp): st_value out of bounds
    section [38] '.symtab': _DYNAMIC symbol size 0 does not match dynamic segment size 624
    section [38] '.symtab': _GLOBAL_OFFSET_TABLE_ symbol size 0 does not match .got section size 3736

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint --gnu src/nm
    section [38] '.symtab': symbol 781 (_gp): st_value out of bounds

    After add '.got':

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint  src/nm
    section [38] '.symtab': _DYNAMIC symbol size 0 does not match dynamic segment size 624
    section [38] '.symtab': symbol 912 (_fbss): st_value out of bounds
    section [38] '.symtab': symbol 1160 (__bss_start): st_value out of bounds

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint  --gnu src/nm
    section [38] '.symtab': symbol 912 (_fbss): st_value out of bounds

    After add '.bss':

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint  src/nm
    section [38] '.symtab': _DYNAMIC symbol size 0 does not match dynamic segment size 624

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint  --gnu src/nm
    No errors

    And also has error , but make check is OK.

   

>
>> +/* Check whether SHF_MASKPROC flags are valid.  */
>> +bool
>> +mips_machine_section_flag_check (GElf_Xword sh_flags)
>> +{
>> +  return ((sh_flags &~ (SHF_MIPS_GPREL |
>> +		        SHF_MIPS_MERGE |
>> +			SHF_MIPS_ADDR |
>> +			SHF_MIPS_STRINGS)) == 0);
>> +}
> OK. But see below for checking other SHF_MIPS flags.

    OK, add it:

diff --git a/backends/mips_symbol.c b/backends/mips_symbol.c
index 8787fcee..f4dee731 100644
--- a/backends/mips_symbol.c
+++ b/backends/mips_symbol.c
@@ -188,7 +188,11 @@ mips_machine_section_flag_check (GElf_Xword sh_flags)
   return ((sh_flags &~ (SHF_MIPS_GPREL |
                        SHF_MIPS_MERGE |
                        SHF_MIPS_ADDR |
-                       SHF_MIPS_STRINGS)) == 0);
+                       SHF_MIPS_STRINGS |
+                       SHF_MIPS_NOSTRIP |
+                       SHF_MIPS_LOCAL |
+                       SHF_MIPS_NAMES |
+                       SHF_MIPS_NODUPE)) == 0);
 }

>
>> diff --git a/src/elflint.c b/src/elflint.c
>> index dd42dcb4..04f1ee92 100644
>> --- a/src/elflint.c
>> +++ b/src/elflint.c
>> @@ -935,7 +935,10 @@ section [%2d] '%s': symbol %zu (%s): non-local symbol outside range described in
>>  	}
>>  
>>        if (GELF_ST_TYPE (sym->st_info) == STT_SECTION
>> -	  && GELF_ST_BIND (sym->st_info) != STB_LOCAL)
>> +	  && GELF_ST_BIND (sym->st_info) != STB_LOCAL
>> +	  && ehdr->e_machine != EM_MIPS
>> +	  && strcmp (name, "_DYNAMIC_LINK") != 0
>> +	  && strcmp (name, "_DYNAMIC_LINKING") != 0)
> Could you add a comment here about both symbols not being local on
> MIPS?

    raw code:

    huangying@Sleepygon:~/elf/elfutils_4$ src/elflint --gnu src/nm
    section [ 7] '.dynsym': symbol 166 (_DYNAMIC_LINKING): non-local section symbol
    section [38] '.symtab': symbol 1052 (_DYNAMIC_LINKING): non-local section symbol
   

>
>>  	ERROR (_("\
>>  section [%2d] '%s': symbol %zu (%s): non-local section symbol\n"),
>>  	       idx, section_name (ebl, idx), cnt, name);
>> @@ -3789,6 +3792,12 @@ cannot get section header for section [%2zu] '%s': %s\n"),
>>  		    && ebl_bss_plt_p (ebl))
>>  		  good_type = SHT_NOBITS;
>>  
>> +	        if (ehdr->e_machine == EM_MIPS
>> +	            && (strstr(special_sections[s].name, ".debug") != NULL))
>> +		  {
>> +	            good_type = SHT_MIPS_DWARF;
>> +		  }
> OK. You don't need explicit brackets here
    OK
>
>>  		/* In a debuginfo file, any normal section can be SHT_NOBITS.
>>  		   This is only invalid for DWARF sections and .shstrtab.  */
>>  		if (shdr->sh_type != good_type
>> @@ -3953,8 +3962,16 @@ section [%2zu] '%s': size not multiple of entry size\n"),
>>  	      sh_flags &= ~(GElf_Xword) SHF_MASKPROC;
>>  	    }
>>  	  if (sh_flags & SHF_MASKOS)
>> -	    if (gnuld)
>> -	      sh_flags &= ~(GElf_Xword) SHF_GNU_RETAIN;
>> +	    {
>> +	      if (gnuld)
>> +	        sh_flags &= ~(GElf_Xword) SHF_GNU_RETAIN;
>> +	      if (ehdr->e_machine == EM_MIPS)
>> +	        {
>> +	          if(sh_flags == SHF_MIPS_NOSTRIP || sh_flags == SHF_MIPS_LOCAL
>> +		   || sh_flags == SHF_MIPS_NAMES || sh_flags == SHF_MIPS_NODUPE)
>> +	            sh_flags &= ~shdr->sh_flags;
>> +	        }
>> +	    }
>>  	  if (sh_flags != 0)
>>  	    ERROR (_("section [%2zu] '%s' contains unknown flag(s)"
>>  			    " %#" PRIx64 "\n"),
> Can this be checked with the machine_section_flag_check hook?
>
> Thanks,
>
> Mark

    OK, add it:

diff --git a/src/elflint.c b/src/elflint.c
index 04f1ee92..56244051 100644
--- a/src/elflint.c
+++ b/src/elflint.c
@@ -3965,12 +3965,13 @@ section [%2zu] '%s': size not multiple of entry size\n"),
            {
              if (gnuld)
                sh_flags &= ~(GElf_Xword) SHF_GNU_RETAIN;
-             if (ehdr->e_machine == EM_MIPS)
-               {
-                 if(sh_flags == SHF_MIPS_NOSTRIP || sh_flags == SHF_MIPS_LOCAL
-                  || sh_flags == SHF_MIPS_NAMES || sh_flags == SHF_MIPS_NODUPE)
-                   sh_flags &= ~shdr->sh_flags;
-               }
+             if (!ebl_machine_section_flag_check (ebl,
+                                                  sh_flags & SHF_MASKOS))
+               ERROR (_("section [%2zu] '%s'"
+                               " contains invalid os-specific flag(s)"
+                               " %#" PRIx64 "\n"),
+                      cnt, section_name (ebl, cnt), sh_flags & SHF_MASKOS);
+             sh_flags &= ~(GElf_Xword) SHF_MASKOS;

Thanks,

Ying