* [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob()
@ 2024-01-11 9:14 Guo, Gua
2024-01-11 9:14 ` [edk2-devel] [PATCH v2 1/4] UefiPayloadPkg/Hob: " Guo, Gua
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Guo, Gua @ 2024-01-11 9:14 UTC (permalink / raw)
To: devel; +Cc: gua.guo, Ard Biesheuvel, Gerd Hoffmann, John Mathew,
Vincent Zimmer
From: Gua Guo <gua.guo@intel.com>
V2
1. UefiPayloadPkg/Hob: Integer : Add Reviewed-by and Authored-by
2. StandaloneMmPkg/Hob: Integer Overflow in : Add Reviewed-by and Authored-by
3. EmbeddedPkg/Hob: Integer Overflow in CreateHob() : Add Reviewed-by and Authored-by
4. MdeModulePkg/Hob: Integer Overflow in CreateHob() : Add Authored-by
V1
1. UefiPayloadPkg/Hob: Integer
2. StandaloneMmPkg/Hob: Integer Overflow in
3. EmbeddedPkg/Hob: Integer Overflow in CreateHob()
4. MdeModulePkg/Hob: Integer Overflow in CreateHob()
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: John Mathew <john.mathews@intel.com>
Cc: Vincent Zimmer <vincent.zimmer@intel.com>
Gua Guo (4):
UefiPayloadPkg/Hob: Integer Overflow in CreateHob()
StandaloneMmPkg/Hob: Integer Overflow in CreateHob()
EmbeddedPkg/Hob: Integer Overflow in CreateHob()
MdeModulePkg/Hob: Integer Overflow in CreateHob()
EmbeddedPkg/Library/PrePiHobLib/Hob.c | 7 +++++++
MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +-
.../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++
UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c | 7 +++++++
4 files changed, 22 insertions(+), 1 deletion(-)
--
2.39.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113607): https://edk2.groups.io/g/devel/message/113607
Mute This Topic: https://groups.io/mt/103658961/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread* [edk2-devel] [PATCH v2 1/4] UefiPayloadPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 9:14 [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() Guo, Gua @ 2024-01-11 9:14 ` Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: " Guo, Gua ` (3 subsequent siblings) 4 siblings, 0 replies; 12+ messages in thread From: Guo, Gua @ 2024-01-11 9:14 UTC (permalink / raw) To: devel Cc: gua.guo, Marc Beatove, Guo Dong, Sean Rhodes, James Lu, John Mathew, Gerd Hoffmann 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> Cc: Guo Dong <guo.dong@intel.com> Cc: Sean Rhodes <sean@starlabs.systems> Cc: James Lu <james.lu@intel.com> Reviewed-by: Gua Guo <gua.guo@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> --- UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c b/UefiPayloadPkg/Library/PayloadEntryHobLib/Hob.c index 2c3acbbc19..39f07d1964 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; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113609): https://edk2.groups.io/g/devel/message/113609 Mute This Topic: https://groups.io/mt/103658963/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 9:14 [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 1/4] UefiPayloadPkg/Hob: " Guo, Gua @ 2024-01-11 9:14 ` Guo, Gua 2024-01-11 14:06 ` Sami Mujawar 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 3/4] EmbeddedPkg/Hob: " Guo, Gua ` (2 subsequent siblings) 4 siblings, 1 reply; 12+ messages in thread From: Guo, Gua @ 2024-01-11 9:14 UTC (permalink / raw) To: devel Cc: gua.guo, Marc Beatove, Ard Biesheuvel, Sami Mujawar, Ray Ni, John Mathew, Gerd Hoffmann 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> --- .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c index 1550e1babc..bb8426dc0a 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; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113606): https://edk2.groups.io/g/devel/message/113606 Mute This Topic: https://groups.io/mt/103658960/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: " Guo, Gua @ 2024-01-11 14:06 ` Sami Mujawar 2024-01-11 14:18 ` Guo, Gua 0 siblings, 1 reply; 12+ messages in thread From: Sami Mujawar @ 2024-01-11 14:06 UTC (permalink / raw) To: gua.guo@intel.com, devel@edk2.groups.io Cc: Marc Beatove, Ard Biesheuvel, Ray Ni, John Mathew, Gerd Hoffmann, nd Hi Gua, Thank you for this patch. Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 09:15, "gua.guo@intel.com <mailto:gua.guo@intel.com>" <gua.guo@intel.com <mailto:gua.guo@intel.com>> wrote: From: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 <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 <mailto:mbeatove@google.com>> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>> Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>> Cc: John Mathew <john.mathews@intel.com <mailto:john.mathews@intel.com>> Authored-by: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>> Signed-off-by: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com>> --- .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c index 1550e1babc..bb8426dc0a 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; [SAMI] Although this fix is correct, I think it shifts the problem somewhere else. If the above condition occurs, a NULL is returned. A quick scan reveals that the calling functions do not check the returned value before use. e.g. https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170 There are multiple such places where the calling functions do not check the pointer returned by CreateHob(). I believe a similar situation can happen for the other patches in this series. [/SAMI] + } + HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113619): https://edk2.groups.io/g/devel/message/113619 Mute This Topic: https://groups.io/mt/103658960/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 14:06 ` Sami Mujawar @ 2024-01-11 14:18 ` Guo, Gua 2024-01-11 15:02 ` Sami Mujawar 0 siblings, 1 reply; 12+ messages in thread From: Guo, Gua @ 2024-01-11 14:18 UTC (permalink / raw) To: Sami Mujawar, devel@edk2.groups.io Cc: Marc Beatove, Ard Biesheuvel, Ni, Ray, Mathews, John, Gerd Hoffmann, nd You mean we need to add below error handle after all callers ? Hob = CreateHob (...) ASSERT (Hob != NULL); <---------------- Here Thanks, Gua -----Original Message----- From: Sami Mujawar <Sami.Mujawar@arm.com> Sent: Thursday, January 11, 2024 10:06 PM To: Guo, Gua <gua.guo@intel.com>; devel@edk2.groups.io Cc: Marc Beatove <mbeatove@google.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Ni, Ray <ray.ni@intel.com>; Mathews, John <john.mathews@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; nd <nd@arm.com> Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() Hi Gua, Thank you for this patch. Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 09:15, "gua.guo@intel.com <mailto:gua.guo@intel.com>" <gua.guo@intel.com <mailto:gua.guo@intel.com>> wrote: From: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 <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 <mailto:mbeatove@google.com>> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>> Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>> Cc: John Mathew <john.mathews@intel.com <mailto:john.mathews@intel.com>> Authored-by: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>> Signed-off-by: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com>> --- .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c index 1550e1babc..bb8426dc0a 100644 --- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c +++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor +++ eHobLib.c @@ -34,6 +34,13 @@ CreateHob ( HandOffHob = GetHobList (); + // + // Check Length to avoid data overflow. + // + if (HobLength > MAX_UINT16 - 0x7) { + return NULL; [SAMI] Although this fix is correct, I think it shifts the problem somewhere else. If the above condition occurs, a NULL is returned. A quick scan reveals that the calling functions do not check the returned value before use. e.g. https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170 There are multiple such places where the calling functions do not check the pointer returned by CreateHob(). I believe a similar situation can happen for the other patches in this series. [/SAMI] + } + HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113620): https://edk2.groups.io/g/devel/message/113620 Mute This Topic: https://groups.io/mt/103658960/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 14:18 ` Guo, Gua @ 2024-01-11 15:02 ` Sami Mujawar 2024-01-11 15:14 ` Guo, Gua 0 siblings, 1 reply; 12+ messages in thread From: Sami Mujawar @ 2024-01-11 15:02 UTC (permalink / raw) To: Guo, Gua, devel@edk2.groups.io Cc: Marc Beatove, Ard Biesheuvel, Ni, Ray, Mathews, John, Gerd Hoffmann, nd Hi Gua, Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 14:19, "Guo, Gua" <gua.guo@intel.com <mailto:gua.guo@intel.com>> wrote: You mean we need to add below error handle after all callers ? Hob = CreateHob (...) ASSERT (Hob != NULL); <---------------- Here [SAMI] That would certainly help catch issues in the debug builds. But the problem with asserts is, they vanish in release builds. I think we should consider adding appropriate error handling in the calling functions to make sure that they do not result in a crash. [/SAMI] Thanks, Gua -----Original Message----- From: Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>> Sent: Thursday, January 11, 2024 10:06 PM To: Guo, Gua <gua.guo@intel.com <mailto:gua.guo@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> Cc: Marc Beatove <mbeatove@google.com <mailto:mbeatove@google.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Mathews, John <john.mathews@intel.com <mailto:john.mathews@intel.com>>; Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>; nd <nd@arm.com <mailto:nd@arm.com>> Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() Hi Gua, Thank you for this patch. Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 09:15, "gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>" <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> wrote: From: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <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 <mailto:mbeatove@google.com> <mailto:mbeatove@google.com <mailto:mbeatove@google.com>>> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>> Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com> <mailto:sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>> Cc: John Mathew <john.mathews@intel.com <mailto:john.mathews@intel.com> <mailto:john.mathews@intel.com <mailto:john.mathews@intel.com>>> Authored-by: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com> <mailto:kraxel@redhat.com <mailto:kraxel@redhat.com>>> Signed-off-by: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> --- .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c index 1550e1babc..bb8426dc0a 100644 --- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c +++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor +++ eHobLib.c @@ -34,6 +34,13 @@ CreateHob ( HandOffHob = GetHobList (); + // + // Check Length to avoid data overflow. + // + if (HobLength > MAX_UINT16 - 0x7) { + return NULL; [SAMI] Although this fix is correct, I think it shifts the problem somewhere else. If the above condition occurs, a NULL is returned. A quick scan reveals that the calling functions do not check the returned value before use. e.g. https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170 <https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170> There are multiple such places where the calling functions do not check the pointer returned by CreateHob(). I believe a similar situation can happen for the other patches in this series. [/SAMI] + } + HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113622): https://edk2.groups.io/g/devel/message/113622 Mute This Topic: https://groups.io/mt/103658960/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 15:02 ` Sami Mujawar @ 2024-01-11 15:14 ` Guo, Gua 2024-01-11 16:11 ` Sami Mujawar 0 siblings, 1 reply; 12+ messages in thread From: Guo, Gua @ 2024-01-11 15:14 UTC (permalink / raw) To: Sami Mujawar, devel@edk2.groups.io Cc: Marc Beatove, Ard Biesheuvel, Ni, Ray, Mathews, John, Gerd Hoffmann, nd Maybe I can add error handle but they will have several case need to do if it's fine. It maybe increasing some BIOS size. Error Handle Error Handle Error Handle Error Handle A ----------------> B -----------------> C ------------------> CreateHob ----------------> return NULL All caller chain may need to add it if we really want to prevent it on release. Thanks, Gua -----Original Message----- From: Sami Mujawar <Sami.Mujawar@arm.com> Sent: Thursday, January 11, 2024 11:02 PM To: Guo, Gua <gua.guo@intel.com>; devel@edk2.groups.io Cc: Marc Beatove <mbeatove@google.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Ni, Ray <ray.ni@intel.com>; Mathews, John <john.mathews@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; nd <nd@arm.com> Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() Hi Gua, Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 14:19, "Guo, Gua" <gua.guo@intel.com <mailto:gua.guo@intel.com>> wrote: You mean we need to add below error handle after all callers ? Hob = CreateHob (...) ASSERT (Hob != NULL); <---------------- Here [SAMI] That would certainly help catch issues in the debug builds. But the problem with asserts is, they vanish in release builds. I think we should consider adding appropriate error handling in the calling functions to make sure that they do not result in a crash. [/SAMI] Thanks, Gua -----Original Message----- From: Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>> Sent: Thursday, January 11, 2024 10:06 PM To: Guo, Gua <gua.guo@intel.com <mailto:gua.guo@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> Cc: Marc Beatove <mbeatove@google.com <mailto:mbeatove@google.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Mathews, John <john.mathews@intel.com <mailto:john.mathews@intel.com>>; Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>; nd <nd@arm.com <mailto:nd@arm.com>> Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() Hi Gua, Thank you for this patch. Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 09:15, "gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>" <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> wrote: From: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <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 <mailto:mbeatove@google.com> <mailto:mbeatove@google.com <mailto:mbeatove@google.com>>> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>> Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com> <mailto:sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>> Cc: John Mathew <john.mathews@intel.com <mailto:john.mathews@intel.com> <mailto:john.mathews@intel.com <mailto:john.mathews@intel.com>>> Authored-by: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com> <mailto:kraxel@redhat.com <mailto:kraxel@redhat.com>>> Signed-off-by: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> --- .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c index 1550e1babc..bb8426dc0a 100644 --- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c +++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor +++ eHobLib.c @@ -34,6 +34,13 @@ CreateHob ( HandOffHob = GetHobList (); + // + // Check Length to avoid data overflow. + // + if (HobLength > MAX_UINT16 - 0x7) { + return NULL; [SAMI] Although this fix is correct, I think it shifts the problem somewhere else. If the above condition occurs, a NULL is returned. A quick scan reveals that the calling functions do not check the returned value before use. e.g. https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170 <https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170> There are multiple such places where the calling functions do not check the pointer returned by CreateHob(). I believe a similar situation can happen for the other patches in this series. [/SAMI] + } + HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113623): https://edk2.groups.io/g/devel/message/113623 Mute This Topic: https://groups.io/mt/103658960/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 15:14 ` Guo, Gua @ 2024-01-11 16:11 ` Sami Mujawar 0 siblings, 0 replies; 12+ messages in thread From: Sami Mujawar @ 2024-01-11 16:11 UTC (permalink / raw) To: Guo, Gua, devel@edk2.groups.io Cc: Marc Beatove, Ard Biesheuvel, Ni, Ray, Mathews, John, Gerd Hoffmann, nd, afish@apple.com, lersek@redhat.com, quic_llindhol@quicinc.com, michael.d.kinney@intel.com Hi Gua, Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 15:19, "Guo, Gua" <gua.guo@intel.com <mailto:gua.guo@intel.com>> wrote: Maybe I can add error handle but they will have several case need to do if it's fine. It maybe increasing some BIOS size. Error Handle Error Handle Error Handle Error Handle A ----------------> B -----------------> C ------------------> CreateHob ----------------> return NULL All caller chain may need to add it if we really want to prevent it on release. [SAMI] As I understand, in most cases such an error may not be recoverable. In such cases, would it make sense for the ASSERT() implementation to call a Panic() in release builds? Adding a Panic() API has been discussed before at https://edk2.groups.io/g/devel/message/86354. [/SAMI] Thanks, Gua -----Original Message----- From: Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>> Sent: Thursday, January 11, 2024 11:02 PM To: Guo, Gua <gua.guo@intel.com <mailto:gua.guo@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> Cc: Marc Beatove <mbeatove@google.com <mailto:mbeatove@google.com>>; Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Mathews, John <john.mathews@intel.com <mailto:john.mathews@intel.com>>; Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>; nd <nd@arm.com <mailto:nd@arm.com>> Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() Hi Gua, Please find my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 14:19, "Guo, Gua" <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>> wrote: You mean we need to add below error handle after all callers ? Hob = CreateHob (...) ASSERT (Hob != NULL); <---------------- Here [SAMI] That would certainly help catch issues in the debug builds. But the problem with asserts is, they vanish in release builds. I think we should consider adding appropriate error handling in the calling functions to make sure that they do not result in a crash. [/SAMI] Thanks, Gua -----Original Message----- From: Sami Mujawar <Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com> <mailto:Sami.Mujawar@arm.com <mailto:Sami.Mujawar@arm.com>>> Sent: Thursday, January 11, 2024 10:06 PM To: Guo, Gua <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>> Cc: Marc Beatove <mbeatove@google.com <mailto:mbeatove@google.com> <mailto:mbeatove@google.com <mailto:mbeatove@google.com>>>; Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>; Mathews, John <john.mathews@intel.com <mailto:john.mathews@intel.com> <mailto:john.mathews@intel.com <mailto:john.mathews@intel.com>>>; Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com> <mailto:kraxel@redhat.com <mailto:kraxel@redhat.com>>>; nd <nd@arm.com <mailto:nd@arm.com> <mailto:nd@arm.com <mailto:nd@arm.com>>> Subject: Re: [PATCH v2 2/4] StandaloneMmPkg/Hob: Integer Overflow in CreateHob() Hi Gua, Thank you for this patch. Please see my response inline marked [SAMI]. Regards, Sami Mujawar On 11/01/2024, 09:15, "gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>>" <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>>> wrote: From: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4166 <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166>> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166>> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166>> <https://bugzilla.tianocore.org/show_bug.cgi?id=4166&gt;>> 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 <mailto:mbeatove@google.com> <mailto:mbeatove@google.com <mailto:mbeatove@google.com>> <mailto:mbeatove@google.com <mailto:mbeatove@google.com> <mailto:mbeatove@google.com <mailto:mbeatove@google.com>>>> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org> <mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>>>> Cc: Sami Mujawar <sami.mujawar@arm.com <mailto:sami.mujawar@arm.com> <mailto:sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>> <mailto:sami.mujawar@arm.com <mailto:sami.mujawar@arm.com> <mailto:sami.mujawar@arm.com <mailto:sami.mujawar@arm.com>>>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>> Cc: John Mathew <john.mathews@intel.com <mailto:john.mathews@intel.com> <mailto:john.mathews@intel.com <mailto:john.mathews@intel.com>> <mailto:john.mathews@intel.com <mailto:john.mathews@intel.com> <mailto:john.mathews@intel.com <mailto:john.mathews@intel.com>>>> Authored-by: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com> <mailto:kraxel@redhat.com <mailto:kraxel@redhat.com>> <mailto:kraxel@redhat.com <mailto:kraxel@redhat.com> <mailto:kraxel@redhat.com <mailto:kraxel@redhat.com>>>> Signed-off-by: Gua Guo <gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com> <mailto:gua.guo@intel.com <mailto:gua.guo@intel.com>>>> --- .../StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c index 1550e1babc..bb8426dc0a 100644 --- a/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c +++ b/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCor +++ eHobLib.c @@ -34,6 +34,13 @@ CreateHob ( HandOffHob = GetHobList (); + // + // Check Length to avoid data overflow. + // + if (HobLength > MAX_UINT16 - 0x7) { + return NULL; [SAMI] Although this fix is correct, I think it shifts the problem somewhere else. If the above condition occurs, a NULL is returned. A quick scan reveals that the calling functions do not check the returned value before use. e.g. https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170 <https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170> <https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170> <https://github.com/tianocore/edk2/blob/master/StandaloneMmPkg/Library/StandaloneMmCoreHobLib/Arm/StandaloneMmCoreHobLib.c#L167-L170>> There are multiple such places where the calling functions do not check the pointer returned by CreateHob(). I believe a similar situation can happen for the other patches in this series. [/SAMI] + } + HobLength = (UINT16)((HobLength + 0x7) & (~0x7)); FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113625): https://edk2.groups.io/g/devel/message/113625 Mute This Topic: https://groups.io/mt/103658960/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [edk2-devel] [PATCH v2 3/4] EmbeddedPkg/Hob: Integer Overflow in CreateHob() 2024-01-11 9:14 [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 1/4] UefiPayloadPkg/Hob: " Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: " Guo, Gua @ 2024-01-11 9:14 ` Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: " Guo, Gua 2024-01-11 10:28 ` [edk2-devel] [PATCH v2 0/4] Bz4166: " Gerd Hoffmann 4 siblings, 0 replies; 12+ messages in thread From: Guo, Gua @ 2024-01-11 9:14 UTC (permalink / raw) To: devel Cc: gua.guo, Marc Beatove, Leif Lindholm, Ard Biesheuvel, Abner Chang, John Mathew, Gerd Hoffmann 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> Cc: Leif Lindholm <quic_llindhol@quicinc.com> Reviewed-by: Ard Biesheuvel <ardb+tianocore@kernel.org> Cc: Abner Chang <abner.chang@amd.com> Cc: John Mathew <john.mathews@intel.com> Authored-by: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Gua Guo <gua.guo@intel.com> --- EmbeddedPkg/Library/PrePiHobLib/Hob.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/EmbeddedPkg/Library/PrePiHobLib/Hob.c b/EmbeddedPkg/Library/PrePiHobLib/Hob.c index 8eb175aa96..1fe3ea93e4 100644 --- a/EmbeddedPkg/Library/PrePiHobLib/Hob.c +++ b/EmbeddedPkg/Library/PrePiHobLib/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; -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113608): https://edk2.groups.io/g/devel/message/113608 Mute This Topic: https://groups.io/mt/103658962/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob() 2024-01-11 9:14 [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() Guo, Gua ` (2 preceding siblings ...) 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 3/4] EmbeddedPkg/Hob: " Guo, Gua @ 2024-01-11 9:14 ` Guo, Gua 2024-01-11 13:31 ` Guo, Gua 2024-01-11 10:28 ` [edk2-devel] [PATCH v2 0/4] Bz4166: " Gerd Hoffmann 4 siblings, 1 reply; 12+ messages in thread From: Guo, Gua @ 2024-01-11 9:14 UTC (permalink / raw) To: devel; +Cc: gua.guo, Marc Beatove, Liming Gao, John Mathew, Gerd Hoffmann 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> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: John Mathew <john.mathews@intel.com> Authored-by: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Gua Guo <gua.guo@intel.com> --- MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Core/Pei/Hob/Hob.c b/MdeModulePkg/Core/Pei/Hob/Hob.c index c4882a23cd..985da50995 100644 --- a/MdeModulePkg/Core/Pei/Hob/Hob.c +++ b/MdeModulePkg/Core/Pei/Hob/Hob.c @@ -85,7 +85,7 @@ PeiCreateHob ( // // Check Length to avoid data overflow. // - if (0x10000 - Length <= 0x7) { + if (MAX_UINT16 - Length < 0x7) { return EFI_INVALID_PARAMETER; } -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113610): https://edk2.groups.io/g/devel/message/113610 Mute This Topic: https://groups.io/mt/103658964/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: Integer Overflow in CreateHob() 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: " Guo, Gua @ 2024-01-11 13:31 ` Guo, Gua 0 siblings, 0 replies; 12+ messages in thread From: Guo, Gua @ 2024-01-11 13:31 UTC (permalink / raw) To: devel@edk2.groups.io, Gao, Liming Cc: Marc Beatove, Mathews, John, Gerd Hoffmann Hi @Gao, Liming I may need to get your help to check this change when you're available. If it's fine for you from MdeModulePkg. I think we can merge this PR. https://github.com/tianocore/edk2/pull/5252 Thanks, Gua -----Original Message----- From: Guo, Gua <gua.guo@intel.com> Sent: Thursday, January 11, 2024 5:15 PM To: devel@edk2.groups.io Cc: Guo, Gua <gua.guo@intel.com>; Marc Beatove <mbeatove@google.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Mathews, John <john.mathews@intel.com>; Gerd Hoffmann <kraxel@redhat.com> Subject: [PATCH v2 4/4] MdeModulePkg/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> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: John Mathew <john.mathews@intel.com> Authored-by: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Gua Guo <gua.guo@intel.com> --- MdeModulePkg/Core/Pei/Hob/Hob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Core/Pei/Hob/Hob.c b/MdeModulePkg/Core/Pei/Hob/Hob.c index c4882a23cd..985da50995 100644 --- a/MdeModulePkg/Core/Pei/Hob/Hob.c +++ b/MdeModulePkg/Core/Pei/Hob/Hob.c @@ -85,7 +85,7 @@ PeiCreateHob ( // // Check Length to avoid data overflow. //- if (0x10000 - Length <= 0x7) {+ if (MAX_UINT16 - Length < 0x7) { return EFI_INVALID_PARAMETER; } -- 2.39.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113617): https://edk2.groups.io/g/devel/message/113617 Mute This Topic: https://groups.io/mt/103658964/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() 2024-01-11 9:14 [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() Guo, Gua ` (3 preceding siblings ...) 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: " Guo, Gua @ 2024-01-11 10:28 ` Gerd Hoffmann 4 siblings, 0 replies; 12+ messages in thread From: Gerd Hoffmann @ 2024-01-11 10:28 UTC (permalink / raw) To: gua.guo; +Cc: devel, Ard Biesheuvel, John Mathew, Vincent Zimmer On Thu, Jan 11, 2024 at 05:14:35PM +0800, gua.guo@intel.com wrote: > From: Gua Guo <gua.guo@intel.com> > > Gua Guo (4): > UefiPayloadPkg/Hob: Integer Overflow in CreateHob() > StandaloneMmPkg/Hob: Integer Overflow in CreateHob() > EmbeddedPkg/Hob: Integer Overflow in CreateHob() > MdeModulePkg/Hob: Integer Overflow in CreateHob() Series: Acked-by: Gerd Hoffmann <kraxel@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113614): https://edk2.groups.io/g/devel/message/113614 Mute This Topic: https://groups.io/mt/103658961/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-01-11 16:11 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-11 9:14 [edk2-devel] [PATCH v2 0/4] Bz4166: Integer Overflow in CreateHob() Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 1/4] UefiPayloadPkg/Hob: " Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 2/4] StandaloneMmPkg/Hob: " Guo, Gua 2024-01-11 14:06 ` Sami Mujawar 2024-01-11 14:18 ` Guo, Gua 2024-01-11 15:02 ` Sami Mujawar 2024-01-11 15:14 ` Guo, Gua 2024-01-11 16:11 ` Sami Mujawar 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 3/4] EmbeddedPkg/Hob: " Guo, Gua 2024-01-11 9:14 ` [edk2-devel] [PATCH v2 4/4] MdeModulePkg/Hob: " Guo, Gua 2024-01-11 13:31 ` Guo, Gua 2024-01-11 10:28 ` [edk2-devel] [PATCH v2 0/4] Bz4166: " Gerd Hoffmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox