Reviewed-by: Liming Gao <[email protected]> >-----Original Message----- >From: Bi, Dandan >Sent: Monday, January 14, 2019 11:02 PM >To: [email protected] >Cc: Kinney, Michael D <[email protected]>; Gao, Liming ><[email protected]> >Subject: [patch v2] MdePkg/BasePeCoffLib: Add more check for relocation >data > >REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1426 > >V2: >(1) Add NULL pointer check for the input parameters >(2) Add check for the "Adjust" value before applying fix ups. > >In function PeCoffLoaderRelocateImageForRuntime, it doesn't >do much check when do relocation. For API level consideration, >it's not safe enough. >So this patch is to replace the same code logic with function >PeCoffLoaderImageAddress which will cover more validation. > >Cc: Michael D Kinney <[email protected]> >Cc: Liming Gao <[email protected]> >Contributed-under: TianoCore Contribution Agreement 1.1 >Signed-off-by: Dandan Bi <[email protected]> >--- > MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 165 +++++++++++++--------- > 1 file changed, 95 insertions(+), 70 deletions(-) > >diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >index c57816a808..d9c94b89bd 100644 >--- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >+++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c >@@ -13,11 +13,11 @@ > This library will also do some additional check for PE header fields. > > PeCoffLoaderGetPeHeader() routine will do basic check for PE/COFF header. > PeCoffLoaderGetImageInfo() routine will do basic check for whole PE/COFF >image. > >- Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR> >+ Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD >License > which accompanies this distribution. The full text of the license may be >found at > http://opensource.org/licenses/bsd-license.php. >@@ -1669,25 +1669,34 @@ PeCoffLoaderRelocateImageForRuntime ( > UINT32 NumberOfRvaAndSizes; > EFI_IMAGE_DATA_DIRECTORY *DataDirectory; > EFI_IMAGE_DATA_DIRECTORY *RelocDir; > EFI_IMAGE_BASE_RELOCATION *RelocBase; > EFI_IMAGE_BASE_RELOCATION *RelocBaseEnd; >+ EFI_IMAGE_BASE_RELOCATION *RelocBaseOrig; > UINT16 *Reloc; > UINT16 *RelocEnd; > CHAR8 *Fixup; > CHAR8 *FixupBase; > UINT16 *Fixup16; > UINT32 *Fixup32; > UINT64 *Fixup64; > CHAR8 *FixupData; > UINTN Adjust; > RETURN_STATUS Status; >+ PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; >+ >+ if (RelocationData == NULL || ImageBase == 0x0 || VirtImageBase == 0x0) { >+ return; >+ } > > OldBase = (CHAR8 *)((UINTN)ImageBase); > NewBase = (CHAR8 *)((UINTN)VirtImageBase); > Adjust = (UINTN) NewBase - (UINTN) OldBase; > >+ ImageContext.ImageAddress = ImageBase; >+ ImageContext.ImageSize = ImageSize; >+ > // > // Find the image's relocate dir info > // > DosHdr = (EFI_IMAGE_DOS_HEADER *)OldBase; > if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) { >@@ -1730,12 +1739,15 @@ PeCoffLoaderRelocateImageForRuntime ( > // is present in the image. You have to check the NumberOfRvaAndSizes in > // the optional header to verify a desired directory entry is there. > // > if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) { > RelocDir = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC; >- RelocBase = (EFI_IMAGE_BASE_RELOCATION *)(UINTN)(ImageBase + >RelocDir->VirtualAddress); >- RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *)(UINTN)(ImageBase + >RelocDir->VirtualAddress + RelocDir->Size); >+ RelocBase = (EFI_IMAGE_BASE_RELOCATION *) >PeCoffLoaderImageAddress (&ImageContext, RelocDir->VirtualAddress, 0); >+ RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *) >PeCoffLoaderImageAddress (&ImageContext, >+ >RelocDir->VirtualAddress + RelocDir- >>Size, >+ 0 >+ ); > } else { > // > // Cannot find relocations, cannot continue to relocate the image, ASSERT >for this invalid image. > // > ASSERT (FALSE); >@@ -1745,100 +1757,113 @@ PeCoffLoaderRelocateImageForRuntime ( > // > // ASSERT for the invalid image when RelocBase and RelocBaseEnd are both >NULL. > // > ASSERT (RelocBase != NULL && RelocBaseEnd != NULL); > >- // >- // Run the whole relocation block. And re-fixup data that has not been >- // modified. The FixupData is used to see if the image has been modified >- // since it was relocated. This is so data sections that have been updated >- // by code will not be fixed up, since that would set them back to >- // defaults. >- // >- FixupData = RelocationData; >- while (RelocBase < RelocBaseEnd) { >+ if (Adjust != 0) { > // >- // Add check for RelocBase->SizeOfBlock field. >+ // Run the whole relocation block. And re-fixup data that has not been >+ // modified. The FixupData is used to see if the image has been modified >+ // since it was relocated. This is so data sections that have been updated >+ // by code will not be fixed up, since that would set them back to >+ // defaults. > // >- if ((RelocBase->SizeOfBlock == 0) || (RelocBase->SizeOfBlock > RelocDir- >>Size)) { >+ FixupData = RelocationData; >+ RelocBaseOrig = RelocBase; >+ while (RelocBase < RelocBaseEnd) { > // >- // Data invalid, cannot continue to relocate the image, just return. >+ // Add check for RelocBase->SizeOfBlock field. > // >- return; >- } >- >- Reloc = (UINT16 *) ((UINT8 *) RelocBase + sizeof >(EFI_IMAGE_BASE_RELOCATION)); >- RelocEnd = (UINT16 *) ((UINT8 *) RelocBase + RelocBase->SizeOfBlock); >- FixupBase = (CHAR8 *) ((UINTN)ImageBase) + RelocBase->VirtualAddress; >+ if ((RelocBase->SizeOfBlock == 0) || (RelocBase->SizeOfBlock > RelocDir- >>Size)) { >+ // >+ // Data invalid, cannot continue to relocate the image, just return. >+ // >+ return; >+ } > >- // >- // Run this relocation record >- // >- while (Reloc < RelocEnd) { >+ Reloc = (UINT16 *) ((UINT8 *) RelocBase + sizeof >(EFI_IMAGE_BASE_RELOCATION)); >+ RelocEnd = (UINT16 *) ((UINT8 *) RelocBase + RelocBase->SizeOfBlock); >+ if ((UINTN)RelocEnd > (UINTN)RelocBaseOrig + RelocDir->Size) { >+ return; >+ } > >- Fixup = FixupBase + (*Reloc & 0xFFF); >- switch ((*Reloc) >> 12) { >+ FixupBase = PeCoffLoaderImageAddress (&ImageContext, RelocBase- >>VirtualAddress, 0); >+ if (FixupBase == NULL) { >+ return; >+ } > >- case EFI_IMAGE_REL_BASED_ABSOLUTE: >- break; >+ // >+ // Run this relocation record >+ // >+ while (Reloc < RelocEnd) { > >- case EFI_IMAGE_REL_BASED_HIGH: >- Fixup16 = (UINT16 *) Fixup; >- if (*(UINT16 *) FixupData == *Fixup16) { >- *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16))); >+ Fixup = PeCoffLoaderImageAddress (&ImageContext, RelocBase- >>VirtualAddress + (*Reloc & 0xFFF), 0); >+ if (Fixup == NULL) { >+ return; > } >+ switch ((*Reloc) >> 12) { > >- FixupData = FixupData + sizeof (UINT16); >- break; >+ case EFI_IMAGE_REL_BASED_ABSOLUTE: >+ break; > >- case EFI_IMAGE_REL_BASED_LOW: >- Fixup16 = (UINT16 *) Fixup; >- if (*(UINT16 *) FixupData == *Fixup16) { >- *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) Adjust & 0xffff)); >- } >+ case EFI_IMAGE_REL_BASED_HIGH: >+ Fixup16 = (UINT16 *) Fixup; >+ if (*(UINT16 *) FixupData == *Fixup16) { >+ *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> >16))); >+ } > >- FixupData = FixupData + sizeof (UINT16); >- break; >+ FixupData = FixupData + sizeof (UINT16); >+ break; > >- case EFI_IMAGE_REL_BASED_HIGHLOW: >- Fixup32 = (UINT32 *) Fixup; >- FixupData = ALIGN_POINTER (FixupData, sizeof (UINT32)); >- if (*(UINT32 *) FixupData == *Fixup32) { >- *Fixup32 = *Fixup32 + (UINT32) Adjust; >- } >+ case EFI_IMAGE_REL_BASED_LOW: >+ Fixup16 = (UINT16 *) Fixup; >+ if (*(UINT16 *) FixupData == *Fixup16) { >+ *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) Adjust & 0xffff)); >+ } > >- FixupData = FixupData + sizeof (UINT32); >- break; >+ FixupData = FixupData + sizeof (UINT16); >+ break; > >- case EFI_IMAGE_REL_BASED_DIR64: >- Fixup64 = (UINT64 *)Fixup; >- FixupData = ALIGN_POINTER (FixupData, sizeof (UINT64)); >- if (*(UINT64 *) FixupData == *Fixup64) { >- *Fixup64 = *Fixup64 + (UINT64)Adjust; >- } >+ case EFI_IMAGE_REL_BASED_HIGHLOW: >+ Fixup32 = (UINT32 *) Fixup; >+ FixupData = ALIGN_POINTER (FixupData, sizeof (UINT32)); >+ if (*(UINT32 *) FixupData == *Fixup32) { >+ *Fixup32 = *Fixup32 + (UINT32) Adjust; >+ } > >- FixupData = FixupData + sizeof (UINT64); >- break; >+ FixupData = FixupData + sizeof (UINT32); >+ break; > >- default: >+ case EFI_IMAGE_REL_BASED_DIR64: >+ Fixup64 = (UINT64 *)Fixup; >+ FixupData = ALIGN_POINTER (FixupData, sizeof (UINT64)); >+ if (*(UINT64 *) FixupData == *Fixup64) { >+ *Fixup64 = *Fixup64 + (UINT64)Adjust; >+ } >+ >+ FixupData = FixupData + sizeof (UINT64); >+ break; >+ >+ default: >+ // >+ // Only Itanium requires ConvertPeImage_Ex >+ // >+ Status = PeHotRelocateImageEx (Reloc, Fixup, &FixupData, Adjust); >+ if (RETURN_ERROR (Status)) { >+ return ; >+ } >+ } > // >- // Only Itanium requires ConvertPeImage_Ex >+ // Next relocation record > // >- Status = PeHotRelocateImageEx (Reloc, Fixup, &FixupData, Adjust); >- if (RETURN_ERROR (Status)) { >- return ; >- } >+ Reloc += 1; > } > // >- // Next relocation record >+ // next reloc block > // >- Reloc += 1; >+ RelocBase = (EFI_IMAGE_BASE_RELOCATION *) RelocEnd; > } >- // >- // next reloc block >- // >- RelocBase = (EFI_IMAGE_BASE_RELOCATION *) RelocEnd; > } > } > > > /** >-- >2.18.0.windows.1
_______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
