On 01/29/19 19:50, Neo Hsueh wrote:
> Skip runtime relocation for PE images that provide invalid relocation
> infomation (ex: RelocDir->Size = 0) to fix a hang observed while booting
> Windows.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Neo Hsueh <hong-chihx.hs...@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Dandan Bi <dandan...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> ---
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c 
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 1bd079ad6a..3a46e626e4 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -1002,7 +1002,7 @@ PeCoffLoaderRelocateImage (
>                                                                              
> RelocDir->VirtualAddress + RelocDir->Size - 1,
>                                                                              
> TeStrippedOffset
>                                                                              
> );
> -    if (RelocBase == NULL || RelocBaseEnd == NULL || RelocBaseEnd < 
> RelocBase) {
> +    if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < 
> (UINTN) RelocBase) {
>        ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION;
>        return RETURN_LOAD_ERROR;
>      }
> @@ -1022,7 +1022,7 @@ PeCoffLoaderRelocateImage (
>      // Run the relocation information and apply the fixups
>      //
>      FixupData = ImageContext->FixupData;
> -    while (RelocBase < RelocBaseEnd) {
> +    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
>  
>        Reloc     = (UINT16 *) ((CHAR8 *) RelocBase + sizeof 
> (EFI_IMAGE_BASE_RELOCATION));
>        //
> @@ -1051,7 +1051,7 @@ PeCoffLoaderRelocateImage (
>        //
>        // Run this relocation record
>        //
> -      while (Reloc < RelocEnd) {
> +      while ((UINTN) Reloc < (UINTN) RelocEnd) {
>          Fixup = PeCoffLoaderImageAddress (ImageContext, 
> RelocBase->VirtualAddress + (*Reloc & 0xFFF), TeStrippedOffset);
>          if (Fixup == NULL) {
>            ImageContext->ImageError = IMAGE_ERROR_FAILED_RELOCATION;
> @@ -1741,11 +1741,19 @@ PeCoffLoaderRelocateImageForRuntime (
>    //
>    if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
>      RelocDir      = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC;
> -    RelocBase     = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress 
> (&ImageContext, RelocDir->VirtualAddress, 0);
> -    RelocBaseEnd  = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress 
> (&ImageContext,
> -                                                                            
> RelocDir->VirtualAddress + RelocDir->Size - 1,
> -                                                                            0
> -                                                                            
> );
> +    if ((RelocDir != NULL) && (RelocDir->Size > 0)) {
> +      RelocBase     = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress 
> (&ImageContext, RelocDir->VirtualAddress, 0);
> +      RelocBaseEnd  = (EFI_IMAGE_BASE_RELOCATION *) PeCoffLoaderImageAddress 
> (&ImageContext,
> +                                                                             
>  RelocDir->VirtualAddress + RelocDir->Size - 1,
> +                                                                             
>  0
> +                                                                             
>  );
> +    }

If the above check fails, then RelocBase and RelocBaseEnd will not be
assigned, and the expressions below will check the previous values of
those variables. What guarantees that those values are valid / not
indeterminate?

Thanks
Laszlo

> +    if (RelocBase == NULL || RelocBaseEnd == NULL || (UINTN) RelocBaseEnd < 
> (UINTN) RelocBase) {
> +      //
> +      // relocation block is not valid, just return
> +      //
> +      return;
> +    }
>    } else {
>      //
>      // Cannot find relocations, cannot continue to relocate the image, 
> ASSERT for this invalid image.
> @@ -1769,7 +1777,7 @@ PeCoffLoaderRelocateImageForRuntime (
>      //
>      FixupData = RelocationData;
>      RelocBaseOrig = RelocBase;
> -    while (RelocBase < RelocBaseEnd) {
> +    while ((UINTN) RelocBase < (UINTN) RelocBaseEnd) {
>        //
>        // Add check for RelocBase->SizeOfBlock field.
>        //
> @@ -1794,7 +1802,7 @@ PeCoffLoaderRelocateImageForRuntime (
>        //
>        // Run this relocation record
>        //
> -      while (Reloc < RelocEnd) {
> +      while ((UINTN) Reloc < (UINTN) RelocEnd) {
>  
>          Fixup = PeCoffLoaderImageAddress (&ImageContext, 
> RelocBase->VirtualAddress + (*Reloc & 0xFFF), 0);
>          if (Fixup == NULL) {
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to