From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <SRS0=yNeg=BG=oss.cipunited.com=ying.huang@sourceware.org>
Received: from s02.bc.larksuite.com (s02.bc.larksuite.com [209.127.230.20])
	by sourceware.org (Postfix) with UTF8SMTPS id B74E73858D1E
	for <elfutils-devel@sourceware.org>; Wed, 17 May 2023 09:14:53 +0000 (GMT)
DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B74E73858D1E
Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=oss.cipunited.com
Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=oss.cipunited.com
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;
 s=s1; d=oss-cipunited-com.20200927.dkim.feishu.cn; t=1684314887;
  h=from:subject:mime-version:from:date:message-id:subject:to:cc:
 reply-to:content-type:mime-version:in-reply-to:message-id;
 bh=3uYpD+/P+Tc55tGAZ2+Jz/whXZK2ySfE8O3o7xP/uaA=;
 b=3JUnGnVFQrdrTg/66cD9G96hEY+PjPS9VzrsgcNu59XhbHekDN6fLbGvQJIZnJf6cYHa+S
 eaCgPPQ8Bmc7eXU7lg6BGnx8srn/r1H1qxBNFthwAaaxRjL7r2XnPyPfToUPbrLvMQP2uh
 pb4C+PmJs4+DgdX7d/KZIbarw+ngOF/a8bjEe5z9W5CFcNHg1CAxitcDGYDjXbup2IT/lg
 icPLTQ+Vj/t3Scv8rtFrJIwxdy+Q963bCp5l2jejuREqg14HgI2Fz2TVsQ1Uso34F1P+Bp
 1OoPeZa+RVqnJN7GNSXuZIhWNiDNiKJkl+DAyPtSdo0Cj2uTYb3M5Qvl53uyyQ==
Message-Id: <344cdd8f-cb0f-e705-5f10-024045b14f17@oss.cipunited.com>
Content-Language: en-US
Content-Transfer-Encoding: 8bit
From: "Ying Huang" <ying.huang@oss.cipunited.com>
X-Lms-Return-Path: <lba+264649afc+8bf984+sourceware.org+ying.huang@oss.cipunited.com>
Content-Type: multipart/alternative;
 boundary=bffb9f5498ed8c3d921a24d1908a5890c9b6b398add10132c341a4381a26
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0
To: "Mark Wielaard" <mark@klomp.org>, <elfutils-devel@sourceware.org>
Cc: <yunqiang.su@oss.cipunited.com>
Mime-Version: 1.0
In-Reply-To: <66059a3000132413a3d7ebede540309256c85cfd.camel@klomp.org>
Subject: Re: [PATCH 3/5] elflint: Fix invalid type of relocation info and other issues on mips
Date: Wed, 17 May 2023 17:14:36 +0800
References: <20230411081141.1762395-1-ying.huang@oss.cipunited.com> <20230411081141.1762395-4-ying.huang@oss.cipunited.com> <66059a3000132413a3d7ebede540309256c85cfd.camel@klomp.org>
X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,HTML_MESSAGE,HTML_NONELEMENT_30_40,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org
List-Id: <elfutils-devel.sourceware.org>

--bffb9f5498ed8c3d921a24d1908a5890c9b6b398add10132c341a4381a26
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

Hi,

=E5=9C=A8 2023/5/11 23:59, Mark Wielaard =E5=86=99=E9=81=93:
> 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

=C2=A0=C2=A0=C2=A0=C2=A0 OK, I would add hook reloc_valid_use.


>> +/* Check whether given symbol's st_value and st_size are OK despite fai=
ling
>> +   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) !=3D 0)
>> +    return false;
>> +  const char *sname =3D elf_strptr (elf, shstrndx, destshdr->sh_name);
>> +  if (sname =3D=3D NULL)
>> +    return false;
>> +  return (strcmp (sname, ".got") =3D=3D 0 || strcmp (sname, ".bss") =3D=
=3D 0);
>> +}
> Could you add a comment why .got and .bss are special in this case?

=C2=A0=C2=A0=C2=A0 raw code:

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

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

=C2=A0=C2=A0=C2=A0 After add '.got':

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

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

=C2=A0=C2=A0=C2=A0 After add '.bss':

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

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

=C2=A0=C2=A0=C2=A0 And also has error , but make check is OK.

=C2=A0=C2=A0=C2=A0

>
>> +/* 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)) =3D=3D 0);
>> +}
> OK. But see below for checking other SHF_MIPS flags.

=C2=A0=C2=A0=C2=A0 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)
=C2=A0=C2=A0 return ((sh_flags &~ (SHF_MIPS_GPREL |
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_ME=
RGE |
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_AD=
DR |
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_STRIN=
GS)) =3D=3D 0);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_STRIN=
GS |
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_NOSTR=
IP |
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_LOCAL=
 |
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_NAMES=
 |
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 SHF_MIPS_NODUP=
E)) =3D=3D 0);
=C2=A0}

>
>> 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 symb=
ol outside range described in
>>  	}
>>=20=20
>>        if (GELF_ST_TYPE (sym->st_info) =3D=3D STT_SECTION
>> -	  && GELF_ST_BIND (sym->st_info) !=3D STB_LOCAL)
>> +	  && GELF_ST_BIND (sym->st_info) !=3D STB_LOCAL
>> +	  && ehdr->e_machine !=3D EM_MIPS
>> +	  && strcmp (name, "_DYNAMIC_LINK") !=3D 0
>> +	  && strcmp (name, "_DYNAMIC_LINKING") !=3D 0)
> Could you add a comment here about both symbols not being local on
> MIPS?

=C2=A0=C2=A0=C2=A0 raw code:

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

>
>>  	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 =3D SHT_NOBITS;
>>=20=20
>> +	        if (ehdr->e_machine =3D=3D EM_MIPS
>> +	            && (strstr(special_sections[s].name, ".debug") !=3D NULL))
>> +		  {
>> +	            good_type =3D SHT_MIPS_DWARF;
>> +		  }
> OK. You don't need explicit brackets here
=C2=A0=C2=A0=C2=A0 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 !=3D good_type
>> @@ -3953,8 +3962,16 @@ section [%2zu] '%s': size not multiple of entry s=
ize\n"),
>>  	      sh_flags &=3D ~(GElf_Xword) SHF_MASKPROC;
>>  	    }
>>  	  if (sh_flags & SHF_MASKOS)
>> -	    if (gnuld)
>> -	      sh_flags &=3D ~(GElf_Xword) SHF_GNU_RETAIN;
>> +	    {
>> +	      if (gnuld)
>> +	        sh_flags &=3D ~(GElf_Xword) SHF_GNU_RETAIN;
>> +	      if (ehdr->e_machine =3D=3D EM_MIPS)
>> +	        {
>> +	          if(sh_flags =3D=3D SHF_MIPS_NOSTRIP || sh_flags =3D=3D SHF_M=
IPS_LOCAL
>> +		   || sh_flags =3D=3D SHF_MIPS_NAMES || sh_flags =3D=3D SHF_MIPS_NODU=
PE)
>> +	            sh_flags &=3D ~shdr->sh_flags;
>> +	        }
>> +	    }
>>  	  if (sh_flags !=3D 0)
>>  	    ERROR (_("section [%2zu] '%s' contains unknown flag(s)"
>>  			    " %#" PRIx64 "\n"),
> Can this be checked with the machine_section_flag_check hook?
>
> Thanks,
>
> Mark

=C2=A0=C2=A0=C2=A0 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 siz=
e\n"),
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 {
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 if (gnuld)
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 sh_flags &=3D ~(GElf_Xword) SHF_GNU_RETAIN;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 i=
f (ehdr->e_machine =3D=3D EM_MIPS)
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 {
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 if(sh_flags =3D=3D SHF_MIPS_NOSTRIP || sh_flags =
=3D=3D SHF_MIPS_LOCAL
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 || sh_flags =3D=3D SHF_MIPS_NAMES || sh_flag=
s =3D=3D SHF_MIPS_NODUPE)
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sh_flags &=3D ~shdr->sh_flags;
-=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 }
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 i=
f (!ebl_machine_section_flag_check (ebl,
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sh=
_flags & SHF_MASKOS))
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 ERROR (_("section [%2zu] '%s'"
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 " contains invalid os-specific flag(s)"
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 " %#" PRIx64 "\n"),
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 cnt, section_name (e=
bl, cnt), sh_flags & SHF_MASKOS);
+=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 s=
h_flags &=3D ~(GElf_Xword) SHF_MASKOS;

Thanks,

Ying=

--bffb9f5498ed8c3d921a24d1908a5890c9b6b398add10132c341a4381a26--