From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x236.google.com (mail-it0-x236.google.com [IPv6:2607:f8b0:4001:c0b::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C0B9C1A1DFB for ; Sat, 30 Jul 2016 07:11:18 -0700 (PDT) Received: by mail-it0-x236.google.com with SMTP id f6so131707239ith.0 for ; Sat, 30 Jul 2016 07:11:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=B1y8fVn0hS5LkiEWtJvwmZVQm/tcYkAgUFwFavxv24E=; b=kdZl67n8NHD26tPzkdryOQa56nY6W9HOq+LsMjfV3EKw98RqWrhL2xd26ah5Wglro7 ZQ2D4QSBWsvnQIWQFaChU6KyR2bilBLlxHaej+KLRZR81xaI9UN20GKwhecHAoQ/qlEE 6U23Dp9OrXJQfGYN0AiuqtAuhICG+kDFAFTjE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=B1y8fVn0hS5LkiEWtJvwmZVQm/tcYkAgUFwFavxv24E=; b=S6wJ6x+jgkixtwaZR9wp0E4YWj3mfdUr5H5lbwEODjGD8I9VuPXzAD9K56cHrl388V jgjPyymhCRWo8P4uaznwuZY1JA2cOJLEV+sUl82Nj9WZ2WFWxDtENUYcXO4510Gfr48p w7v+6bhaVTH9mLPbbwy0+Ai4EpAFek82E2IwgL1/pM4xpIOr3pYdeHBGxlJDUGA7yX1g MoE+fhN5kfxM/EMir+lR+qySaS6KlWymLOwG+TLmSQO9cyWrzRve/5VoWOmjSIDN7Oq7 gSCd9rlJKhmYzE+U3rQf0eSG9xB885tP8Jjx4EYb8+ykDzRg0Vlhf33lWjwglquY6HLi gPPg== X-Gm-Message-State: AEkoouvjT2oqJnXpv1VSDjMhJyw7TUa3PXgPogyV8DGJ4hGlTYlIqz1VQWSIx9I/2N3B/rZrjn7pa1R4Z4RDyaPT X-Received: by 10.36.86.134 with SMTP id o128mr53036944itb.5.1469887877926; Sat, 30 Jul 2016 07:11:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Sat, 30 Jul 2016 07:11:17 -0700 (PDT) In-Reply-To: <06C8AB66E78EE34A949939824ABE2B3103381245@shsmsx102.ccr.corp.intel.com> References: <1467967364-11556-1-git-send-email-steven.shi@intel.com> <1467967364-11556-3-git-send-email-steven.shi@intel.com> <06C8AB66E78EE34A949939824ABE2B3103381245@shsmsx102.ccr.corp.intel.com> From: Ard Biesheuvel Date: Sat, 30 Jul 2016 16:11:17 +0200 Message-ID: To: "Shi, Steven" Cc: edk2-devel-01 , "Gao, Liming" , "afish@apple.com" , "Justen, Jordan L" , "Kinney, Michael D" Subject: Re: [PATCH v2 2/7] BaseTools-GenFw:Add new x86_64 Elf relocation types for PIC/PIE code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 30 Jul 2016 14:11:19 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 30 July 2016 at 16:09, Shi, Steven wrote: >> > diff --git a/BaseTools/Source/C/GenFw/Elf64Convert.c >> b/BaseTools/Source/C/GenFw/Elf64Convert.c >> > index 7c838f3..fc241ab 100755 >> > --- a/BaseTools/Source/C/GenFw/Elf64Convert.c >> > +++ b/BaseTools/Source/C/GenFw/Elf64Convert.c >> > @@ -755,6 +755,11 @@ WriteSections64 ( >> > // Determine how to handle each relocation type based on the >> machine type. >> > // >> > if (mEhdr->e_machine =3D=3D EM_X86_64) { >> > + // >> > + // See Relocation Types details semantics in Table 4.9 of >> > + // SystemV x86_64 abi Draft Version 0.99.8 >> > + // https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI= - >> secure.pdf >> > + // >> > switch (ELF_R_TYPE(Rel->r_info)) { >> > case R_X86_64_NONE: >> > break; >> > @@ -763,24 +768,24 @@ WriteSections64 ( >> > // Absolute relocation. >> > // >> > VerboseMsg ("R_X86_64_64"); >> > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%016LX", >> > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%016LX", >> > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> >> No change? > [Steven]: remove the unnecessary blank at end of line, which is required = by coding style > >> >> > *(UINT64 *)Targ); >> > *(UINT64 *)Targ =3D *(UINT64 *)Targ - SymShdr->sh_addr + >> mCoffSectionsOffset[Sym->st_shndx]; >> > VerboseMsg ("Relocation: 0x%016LX", *(UINT64*)Targ); >> > break; >> > case R_X86_64_32: >> > VerboseMsg ("R_X86_64_32"); >> > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> >> and here > [Steven]: remove the unnecessary blank at end of line, which is required = by coding style > >> >> > *(UINT32 *)Targ); >> > *(UINT32 *)Targ =3D (UINT32)((UINT64)(*(UINT32 *)Targ) - = SymShdr- >> >sh_addr + mCoffSectionsOffset[Sym->st_shndx]); >> > VerboseMsg ("Relocation: 0x%08X", *(UINT32*)Targ); >> > break; >> > case R_X86_64_32S: >> > VerboseMsg ("R_X86_64_32S"); >> > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> >> and here > [Steven]: remove the unnecessary blank at end of line, which is required = by coding style > >> >> > *(UINT32 *)Targ); >> > *(INT32 *)Targ =3D (INT32)((INT64)(*(INT32 *)Targ) - SymS= hdr- >> >sh_addr + mCoffSectionsOffset[Sym->st_shndx]); >> > VerboseMsg ("Relocation: 0x%08X", *(UINT32*)Targ); >> > @@ -790,14 +795,45 @@ WriteSections64 ( >> > // Relative relocation: Symbol - Ip + Addend >> > // >> > VerboseMsg ("R_X86_64_PC32"); >> > - VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > - (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> >> and here > [Steven]: remove the unnecessary blank at end of line, which is required = by coding style > OK. Could we do that in a separate patch? >> >> > *(UINT32 *)Targ); >> > *(UINT32 *)Targ =3D (UINT32) (*(UINT32 *)Targ >> > + (mCoffSectionsOffset[Sym->st_shndx] - SymShdr->sh_add= r) >> > - (SecOffset - SecShdr->sh_addr)); >> > VerboseMsg ("Relocation: 0x%08X", *(UINT32 *)Targ); >> > break; >> > + case R_X86_64_PLT32: >> > + // >> > + // Relative relocation: L + A - P >> > + // >> > + VerboseMsg ("R_X86_64_PLT32"); >> > + VerboseMsg ("Offset: 0x%08X, Addend: 0x%08X", >> > + (UINT32)(SecOffset + (Rel->r_offset - SecShdr->sh_addr)= ), >> > + *(UINT32 *)Targ); >> > + *(UINT32 *)Targ =3D (UINT32) (*(UINT32 *)Targ >> > + + (mCoffSectionsOffset[Sym->st_shndx] - SymShdr->sh_add= r) >> > + - (SecOffset - SecShdr->sh_addr)); >> > + VerboseMsg ("Relocation: 0x%08X", *(UINT32 *)Targ); >> > + break; >> > + case R_X86_64_GOTPCREL: >> > + // >> > + // Relative relocation: G + GOT + A - P >> > + // >> > + VerboseMsg ("R_X86_64_GOTPCREL"); >> > + break; >> > + case R_X86_64_GOTPCRELX: >> > + // >> > + // Relative relocation: G + GOT + A - P >> > + // >> > + VerboseMsg ("R_X86_64_GOTPCRELX"); >> > + break; >> > + case R_X86_64_REX_GOTPCRELX: >> > + // >> > + // Relative relocation: G + GOT + A - P >> > + // >> > + VerboseMsg ("R_X86_64_REX_GOTPCRELX"); >> > + break; >> > default: >> > Error (NULL, 0, 3000, "Invalid", "%s unsupported ELF EM_X= 86_64 >> relocation 0x%x.", mInImageName, (unsigned) ELF_R_TYPE(Rel->r_info)); >> > } >> > @@ -887,6 +923,12 @@ WriteRelocations64 ( >> > UINT32 Index; >> > EFI_IMAGE_OPTIONAL_HEADER_UNION *NtHdr; >> > EFI_IMAGE_DATA_DIRECTORY *Dir; >> > + UINT64 GoTPcRelPtrOffset =3D 0; >> > + UINT64 *RipDisplacementPtr; >> > + UINT64 *ElfFileGoTPcRelPtr; >> > + UINT64 *CoffFileGoTPcRelPtr; >> > + Elf_Shdr *shdr; >> > + UINT32 i; >> > >> > for (Index =3D 0; Index < mEhdr->e_shnum; Index++) { >> > Elf_Shdr *RelShdr =3D GetShdrByIndex(Index); >> > @@ -902,6 +944,53 @@ WriteRelocations64 ( >> > switch (ELF_R_TYPE(Rel->r_info)) { >> > case R_X86_64_NONE: >> > case R_X86_64_PC32: >> > + case R_X86_64_PLT32: >> > + break; >> > + case R_X86_64_GOTPCREL: >> > + case R_X86_64_GOTPCRELX: >> > + case R_X86_64_REX_GOTPCRELX: >> > + // >> > + // link script force .got and .got.* in .text section, = so GoTPcRel >> pointer must be in .text section >> > + // but its value might point to .text or .data section >> > + // >> > + RipDisplacementPtr =3D (UINT64 *)((UINT8*)mEhdr + SecS= hdr- >> >sh_offset + Rel->r_offset - SecShdr->sh_addr); >> > + GoTPcRelPtrOffset =3D Rel->r_offset + 4 + >> (INT32)(*RipDisplacementPtr) - SecShdr->sh_addr; >> > + ElfFileGoTPcRelPtr =3D (UINT64 *)((UINT8*)mEhdr + SecS= hdr- >> >sh_offset + GoTPcRelPtrOffset); >> > + CoffFileGoTPcRelPtr =3D (UINT64 *)(mCoffFile + >> mCoffSectionsOffset[RelShdr->sh_info] + GoTPcRelPtrOffset); >> > + // >> > + // Check which section the GoTPcRel pointer point to, a= nd >> calculate Elf to Coff sections displacement accordingly >> > + // >> > + shdr =3D NULL; >> > + for (i =3D 0; i < mEhdr->e_shnum; i++) { >> > + shdr =3D GetShdrByIndex(i); >> > + if ((*ElfFileGoTPcRelPtr >=3D shdr->sh_addr) && >> > + (*ElfFileGoTPcRelPtr < shdr->sh_addr + shdr->sh_s= ize)) { >> > + break; >> > + } >> > + } >> > + // >> > + // Fix the Elf to Coff sections displacement >> > + // >> > + if(IsDataShdr(shdr)) { >> > + *CoffFileGoTPcRelPtr =3D *CoffFileGoTPcRelPtr + mData= Offset - >> shdr->sh_addr; >> > + }else if (IsTextShdr(SecShdr)){ >> > + *CoffFileGoTPcRelPtr =3D *CoffFileGoTPcRelPtr + mText= Offset - >> shdr->sh_addr; >> > + }else { >> > + // >> > + // link script force to merge .rodata .rodata.*, .got= and .got.* >> in .text section, >> > + // not support GoTPcRel point to section other than .= data or .text >> > + // >> > + Error (NULL, 0, 3000, "Invalid", "Not support relocat= e >> R_X86_64_GOTPCREL address in section other than .data or .text"); >> > + assert (FALSE); >> > + } >> > + >> > + VerboseMsg ("R_X86_64_GOTPCREL to >> EFI_IMAGE_REL_BASED_DIR64 Offset: 0x%08X", >> > + mCoffSectionsOffset[RelShdr->sh_info] + GoTPcRelPtrOf= fset); >> > + CoffAddFixup( >> > + (UINT32)(UINTN)((UINT64) mCoffSectionsOffset[RelShdr- >> >sh_info] + GoTPcRelPtrOffset), >> > + EFI_IMAGE_REL_BASED_DIR64); >> >> Is this necessary? I would expect the GOT entry itself to be already >> covered by a R_X86_64_64 relocation, so I don't think there is a need >> to emit a EFI_IMAGE_REL_BASED_DIR64 PE/COFF reloc here. > [Steven]: Do you ask whether it is still necessary to support the GOTPCRE= L/GOTPCRELX/REX_GOTPCRELX new relocation types? I think they are nice to ha= ve now, since new type support has no impact to old ones, we could just lea= ve the code there for reference. No. The question is whether it is necessary to emit the EFI_IMAGE_REL_BASED_DIR64 fixup reloc here. The relocation place is the instruction, not the GOT entry, and I would expect the GOT entry to be covered by a R_X86_64_64 relocation already