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

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

[Guo, Gua]

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/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
index 1550e1babc..59473e28fe 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.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 (#113641): https://edk2.groups.io/g/devel/message/113641
Mute This Topic: https://groups.io/mt/103675962/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

On Fri, Jan 12, 2024 at 08:56:02AM +0000, Ni, Ray wrote:
> It's strange to me that ARM's MM env still allows modifying HOBs.

Yes.

But fixing that is beyond the scope of this patch, which just
fixes the integer overflow in CreateHob().

Can we please move forward and get the remaining CreateHob()
fixes merged?

thanks & take care,
  Gerd



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

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

[Sami Mujawar]

Hi All,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 24/01/2024, 12:41, "Gerd Hoffmann" <kraxel@redhat.com <mailto:kraxel@redhat.com>> wrote:


On Fri, Jan 12, 2024 at 08:56:02AM +0000, Ni, Ray wrote:
> It's strange to me that ARM's MM env still allows modifying HOBs.
[SAMI] We are investigating the possibility of removing the HOB modification code for Arm. However, this may involve changes at multiple places in the software stack.
Therefore, it may not be possible to immediately remove that support. 
[/SAMI]

Yes.


But fixing that is beyond the scope of this patch, which just
[SAMI] I agree, it is beyond the scope of this patch.

fixes the integer overflow in CreateHob().

Can we please move forward and get the remaining CreateHob()
fixes merged?


thanks & take care,
Gerd







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

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

[Ni, Ray]

Agree.
Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, January 24, 2024 8:41 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: Guo, Gua <gua.guo@intel.com>; devel@edk2.groups.io; Marc Beatove
> <mbeatove@google.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Sami Mujawar <sami.mujawar@arm.com>; Mathews, John
> <john.mathews@intel.com>
> Subject: Re: RE: [PATCH v3 2/4] StandaloneMmPkg/Hob: Integer Overflow in
> CreateHob()
> 
> On Fri, Jan 12, 2024 at 08:56:02AM +0000, Ni, Ray wrote:
> > It's strange to me that ARM's MM env still allows modifying HOBs.
> 
> Yes.
> 
> But fixing that is beyond the scope of this patch, which just
> fixes the integer overflow in CreateHob().
> 
> Can we please move forward and get the remaining CreateHob()
> fixes merged?
> 
> thanks & take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114359): https://edk2.groups.io/g/devel/message/114359
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]
-=-=-=-=-=-=-=-=-=-=-=-