From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id C60433858C54 for ; Thu, 11 May 2023 15:59:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C60433858C54 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from r6.localdomain (82-217-174-174.cable.dynamic.v4.ziggo.nl [82.217.174.174]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 72E55313ACB3; Thu, 11 May 2023 17:59:57 +0200 (CEST) Received: by r6.localdomain (Postfix, from userid 1000) id 38587340154; Thu, 11 May 2023 17:59:57 +0200 (CEST) Message-ID: <66059a3000132413a3d7ebede540309256c85cfd.camel@klomp.org> Subject: Re: [PATCH 3/5] elflint: Fix invalid type of relocation info and other issues on mips From: Mark Wielaard To: ying.huang@oss.cipunited.com, elfutils-devel@sourceware.org Date: Thu, 11 May 2023 17:59:57 +0200 In-Reply-To: <20230411081141.1762395-4-ying.huang@oss.cipunited.com> References: <20230411081141.1762395-1-ying.huang@oss.cipunited.com> <20230411081141.1762395-4-ying.huang@oss.cipunited.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.1 (3.48.1-1.fc38) MIME-Version: 1.0 X-Spam-Status: No, score=-3035.0 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_BARRACUDACENTRAL,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: Hi, On Tue, 2023-04-11 at 16:12 +0800, Ying Huang wrote: > From: Ying Huang >=20 > 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(-) >=20 > 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 > diff --git a/backends/mips_symbol.c b/backends/mips_symbol.c > index e760d58d..8787fcee 100644 > --- a/backends/mips_symbol.c > +++ b/backends/mips_symbol.c > @@ -158,6 +158,39 @@ mips_section_type_name (int type, > return NULL; > } > =20 > +bool > +mips_check_reloc_target_type (Ebl *ebl __attribute__ ((unused)), Elf64_W= ord sh_type) > +{ > + return (sh_type =3D=3D SHT_MIPS_DWARF); > +} OK > +/* Check whether given symbol's st_value and st_size are OK despite fail= ing > + 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? > +/* 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. > /* Check whether machine flags are valid. */ > bool > mips_machine_flag_check (GElf_Word flags) > diff --git a/libebl/eblrelocvaliduse.c b/libebl/eblrelocvaliduse.c > index f0bed345..44b8d300 100644 > --- a/libebl/eblrelocvaliduse.c > +++ b/libebl/eblrelocvaliduse.c > @@ -32,10 +32,14 @@ > #endif > =20 > #include > - > +#include > =20 > bool > ebl_reloc_valid_use (Ebl *ebl, int reloc) > { > - return ebl !=3D NULL ? ebl->reloc_valid_use (ebl->elf, reloc) : false; > + int relocNew =3D reloc; > + GElf_Ehdr ehdr; > + if(ebl->elf->class =3D=3D ELFCLASS64 && gelf_getehdr(ebl->elf, &ehdr) = !=3D NULL && ehdr.e_machine =3D=3D EM_MIPS) > + relocNew =3D ELF64_MIPS_R_TYPE(reloc); > + return ebl !=3D NULL ? ebl->reloc_valid_use (ebl->elf, relocNew) : fal= se; > } This should be a mips reloc_valid_use hook. > 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 symbo= l outside range described in > } > =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? > 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 > + 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 > /* 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 si= ze\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_MI= PS_LOCAL > + || sh_flags =3D=3D SHF_MIPS_NAMES || sh_flags =3D=3D SHF_MIPS_NODUP= E) > + 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