From 59f024c76ee57c2bec84794536302fc770cd6ec2 Mon Sep 17 00:00:00 2001 From: Gua Guo Date: Thu, 11 Jan 2024 13:01:19 +0800 Subject: [PATCH] UefiPayloadPkg/Hob: Integer Overflow in CreateHob() 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 Cc: Guo Dong Cc: Sean Rhodes Cc: James Lu Reviewed-by: Gua Guo Cc: John Mathew Authored-by: Gerd Hoffmann Signed-off-by: Gua Guo --- .../Library/PayloadEntryHobLib/Hob.c | 43 +++++++++++++++++++ .../FitUniversalPayloadEntry.c | 8 ++-- .../UefiPayloadEntry/UniversalPayloadEntry.c | 8 ++-- 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c index 2c3acbbc195e..51c2e28d7ddd 100644 --- a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c +++ b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c @@ -110,6 +110,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; @@ -160,6 +167,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; @@ -330,6 +340,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; @@ -378,6 +392,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; } @@ -441,6 +460,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; @@ -472,6 +495,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; @@ -513,6 +540,10 @@ BuildFv3Hob ( EFI_HOB_FIRMWARE_VOLUME3 *Hob; Hob = CreateHob (EFI_HOB_TYPE_FV3, sizeof (EFI_HOB_FIRMWARE_VOLUME3)); + ASSERT (Hob != NULL); + if (Hob == NULL) { + return; + } Hob->BaseAddress = BaseAddress; Hob->Length = Length; @@ -546,6 +577,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; @@ -583,6 +618,10 @@ BuildStackHob ( ); Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof (EFI_HOB_MEMORY_ALLOCATION_STACK)); + ASSERT (Hob != NULL); + if (Hob == NULL) { + return; + } CopyGuid (&(Hob->AllocDescriptor.Name), &gEfiHobMemoryAllocStackGuid); Hob->AllocDescriptor.MemoryBaseAddress = BaseAddress; @@ -664,6 +703,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; diff --git a/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c index d2e7df4fbe3c..eb0b325369a0 100644 --- a/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c +++ b/UefiPayloadPkg/UefiPayloadEntry/FitUniversalPayloadEntry.c @@ -207,10 +207,12 @@ AddNewHob ( } NewHob.Header = CreateHob (Hob->Header->HobType, Hob->Header->HobLength); - - if (NewHob.Header != NULL) { - CopyMem (NewHob.Header + 1, Hob->Header + 1, Hob->Header->HobLength - sizeof (EFI_HOB_GENERIC_HEADER)); + ASSERT (NewHob.Header != NULL); + if (NewHob.Header == NULL) { + return; } + + CopyMem (NewHob.Header + 1, Hob->Header + 1, Hob->Header->HobLength - sizeof (EFI_HOB_GENERIC_HEADER)); } /** diff --git a/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c index f8939efe70db..f37c00fad774 100644 --- a/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c +++ b/UefiPayloadPkg/UefiPayloadEntry/UniversalPayloadEntry.c @@ -111,10 +111,12 @@ AddNewHob ( } NewHob.Header = CreateHob (Hob->Header->HobType, Hob->Header->HobLength); - - if (NewHob.Header != NULL) { - CopyMem (NewHob.Header + 1, Hob->Header + 1, Hob->Header->HobLength - sizeof (EFI_HOB_GENERIC_HEADER)); + ASSERT (NewHob.Header != NULL); + if (NewHob.Header == NULL) { + return; } + + CopyMem (NewHob.Header + 1, Hob->Header + 1, Hob->Header->HobLength - sizeof (EFI_HOB_GENERIC_HEADER)); } /**