Re: [edk2-devel] [PATCH v3 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob()

["Ni, Ray" <ray.ni@intel.com>]

It's strange to me that ARM's MM env still allows modifying HOBs.

Thanks,
Ray
> -----Original Message-----
> From: Guo, Gua <gua.guo@intel.com>
> Sent: Friday, January 12, 2024 10:25 AM
> To: devel@edk2.groups.io
> Cc: Guo, Gua <gua.guo@intel.com>; Marc Beatove <mbeatove@google.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Sami Mujawar
> <sami.mujawar@arm.com>; Ni, Ray <ray.ni@intel.com>; Mathews, John
> <john.mathews@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v3 2/4] StandaloneMmPkg/Hob: Integer Overflow in
> CreateHob()
> 
> From: Gua Guo <gua.guo@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166
> 
> Fix integer overflow in various CreateHob instances.
> Fixes: CVE-2022-36765
> 
> The CreateHob() function aligns the requested size to 8
> performing the following operation:
> ```
> HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
> ```
> 
> No checks are performed to ensure this value doesn't
> overflow, and could lead to CreateHob() returning a smaller
> HOB than requested, which could lead to OOB HOB accesses.
> 
> Reported-by: Marc Beatove <mbeatove@google.com>
> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: John Mathew <john.mathews@intel.com>
> Authored-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Gua Guo <gua.guo@intel.com>
> ---
>  .../Arm/StandaloneMmCoreHobLib.c              | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git
> a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneM
> mCoreHobLib.c
> b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneM
> mCoreHobLib.c
> index 1550e1babc..59473e28fe 100644
> ---
> a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneM
> mCoreHobLib.c
> +++
> b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneM
> mCoreHobLib.c
> @@ -34,6 +34,13 @@ CreateHob (
> 
> 
>    HandOffHob = GetHobList ();
> 
> 
> 
> +  //
> 
> +  // Check Length to avoid data overflow.
> 
> +  //
> 
> +  if (HobLength > MAX_UINT16 - 0x7) {
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
>    HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
> 
> 
> 
>    FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob-
> >EfiFreeMemoryBottom;
> 
> @@ -89,6 +96,10 @@ BuildModuleHob (
>      );
> 
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof
> (EFI_HOB_MEMORY_ALLOCATION_MODULE));
> 
> +  ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    CopyGuid (&(Hob->MemoryAllocationHeader.Name),
> &gEfiHobMemoryAllocModuleGuid);
> 
>    Hob->MemoryAllocationHeader.MemoryBaseAddress =
> MemoryAllocationModule;
> 
> @@ -129,6 +140,9 @@ BuildResourceDescriptorHob (
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, sizeof
> (EFI_HOB_RESOURCE_DESCRIPTOR));
> 
>    ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    Hob->ResourceType      = ResourceType;
> 
>    Hob->ResourceAttribute = ResourceAttribute;
> 
> @@ -167,6 +181,11 @@ BuildGuidHob (
>    ASSERT (DataLength <= (0xffff - sizeof (EFI_HOB_GUID_TYPE)));
> 
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_GUID_EXTENSION, (UINT16)(sizeof
> (EFI_HOB_GUID_TYPE) + DataLength));
> 
> +  ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return NULL;
> 
> +  }
> 
> +
> 
>    CopyGuid (&Hob->Name, Guid);
> 
>    return Hob + 1;
> 
>  }
> 
> @@ -226,6 +245,10 @@ BuildFvHob (
>    EFI_HOB_FIRMWARE_VOLUME  *Hob;
> 
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_FV, sizeof
> (EFI_HOB_FIRMWARE_VOLUME));
> 
> +  ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    Hob->BaseAddress = BaseAddress;
> 
>    Hob->Length      = Length;
> 
> @@ -255,6 +278,10 @@ BuildFv2Hob (
>    EFI_HOB_FIRMWARE_VOLUME2  *Hob;
> 
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_FV2, sizeof
> (EFI_HOB_FIRMWARE_VOLUME2));
> 
> +  ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    Hob->BaseAddress = BaseAddress;
> 
>    Hob->Length      = Length;
> 
> @@ -282,6 +309,10 @@ BuildCpuHob (
>    EFI_HOB_CPU  *Hob;
> 
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_CPU, sizeof (EFI_HOB_CPU));
> 
> +  ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    Hob->SizeOfMemorySpace = SizeOfMemorySpace;
> 
>    Hob->SizeOfIoSpace     = SizeOfIoSpace;
> 
> @@ -319,6 +350,10 @@ BuildMemoryAllocationHob (
>      );
> 
> 
> 
>    Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof
> (EFI_HOB_MEMORY_ALLOCATION));
> 
> +  ASSERT (Hob != NULL);
> 
> +  if (Hob == NULL) {
> 
> +    return;
> 
> +  }
> 
> 
> 
>    ZeroMem (&(Hob->AllocDescriptor.Name), sizeof (EFI_GUID));
> 
>    Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress;
> 
> --
> 2.39.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113699): https://edk2.groups.io/g/devel/message/113699
Mute This Topic: https://groups.io/mt/103675962/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-