On 25 September 2015 at 10:23, Andrew Fish <[email protected]> wrote: > > On Sep 24, 2015, at 6:03 PM, Ard Biesheuvel <[email protected]> > wrote: > > (adding Leif and Andrew, who merged this code originally) > > > Does this pattern exist in the BaseTools version for the > PeCoffLoaderRelocateImageEx() path? >
Yes, it does, and it suffers from the same problem, as it seems. However, it seems unlikely that the BaseTools version of this code ever gets used, since it is strictly for doing the second relocation on runtime images when SetVirtualAddressMap () is called. If FixupData is not allocated, it is just ignored. > I think we just copied the pattern from the IPF. > Yes, and I also wonder whether the IPF specific relocation of instruction immediate fields makes sense, since I don't see how the program would be poking different immediate values into those instructions at runtime, which means that the second relocation needs to be performed unconditionally. However, the IPF version does advance *FixupData when advancing to the next relocation, so there this particular bug does not exist. -- Ard. > On 23 September 2015 at 21:31, Ard Biesheuvel <[email protected]> > wrote: > > The base relocation type EFI_IMAGE_REL_BASED_ARM_MOV32T patches an > absolute address into the immediate fields of an adjacent movt/movw > instruction pair. > > As the instructions are not writable by the program itself, there is > no need to keep track of the fixup data, since we can reapply the > relocation for runtime unconditionally. So remove the fixup handling > for this relocation type. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > MdePkg/Library/BasePeCoffLib/Arm/PeCoffLoaderEx.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/MdePkg/Library/BasePeCoffLib/Arm/PeCoffLoaderEx.c > b/MdePkg/Library/BasePeCoffLib/Arm/PeCoffLoaderEx.c > index d6bf42738d2b..03a532c60514 100644 > --- a/MdePkg/Library/BasePeCoffLib/Arm/PeCoffLoaderEx.c > +++ b/MdePkg/Library/BasePeCoffLib/Arm/PeCoffLoaderEx.c > @@ -158,13 +158,6 @@ PeCoffLoaderRelocateImageEx ( > case EFI_IMAGE_REL_BASED_ARM_MOV32T: > FixupVal = ThumbMovwMovtImmediateAddress (Fixup16) + (UINT32)Adjust; > ThumbMovwMovtImmediatePatch (Fixup16, FixupVal); > - > - if (*FixupData != NULL) { > - *FixupData = ALIGN_POINTER(*FixupData, sizeof(UINT64)); > - // Fixup16 is not aligned so we must copy it. Thumb instructions are > streams of 16 bytes. > - CopyMem (*FixupData, Fixup16, sizeof (UINT64)); > - *FixupData = *FixupData + sizeof(UINT64); > - } > break; > > case EFI_IMAGE_REL_BASED_ARM_MOV32A: > @@ -229,11 +222,8 @@ PeHotRelocateImageEx ( > switch ((*Reloc) >> 12) { > > case EFI_IMAGE_REL_BASED_ARM_MOV32T: > - *FixupData = ALIGN_POINTER (*FixupData, sizeof (UINT64)); > - if (*(UINT64 *) (*FixupData) == ReadUnaligned64 ((UINT64 *)Fixup16)) { > - FixupVal = ThumbMovwMovtImmediateAddress (Fixup16) + (UINT32)Adjust; > - ThumbMovwMovtImmediatePatch (Fixup16, FixupVal); > - } > + FixupVal = ThumbMovwMovtImmediateAddress (Fixup16) + (UINT32)Adjust; > + ThumbMovwMovtImmediatePatch (Fixup16, FixupVal); > break; > > case EFI_IMAGE_REL_BASED_ARM_MOV32A: > > > To elaborate a bit, this patch actually fixes a bug on ARM: since > PeHotRelocateImageEx () fails to advance the FixupData pointer, it > will think the targets of these relocations have been updated by the > program since the first relocation occurred, and leave them alone > instead of relocating them to the OS VA mapping. > > If people prefer, I could also fix this by adding the increment of > *FixupData in the correct place, but since the whole check is a bit > pointless, I preferred to just rip it out. > > -- > Ard. > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
