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.muja...@arm.com> 
Sent: Thursday, January 11, 2024 10:06 PM
To: Guo, Gua <gua....@intel.com>; devel@edk2.groups.io
Cc: Marc Beatove <mbeat...@google.com>; Ard Biesheuvel 
<ardb+tianoc...@kernel.org>; Ni, Ray <ray...@intel.com>; Mathews, John 
<john.math...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; nd <n...@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....@intel.com <mailto:gua....@intel.com>" 
<gua....@intel.com <mailto:gua....@intel.com>> wrote:


From: Gua Guo <gua....@intel.com <mailto:gua....@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 <mbeat...@google.com <mailto:mbeat...@google.com>>
Reviewed-by: Ard Biesheuvel <ardb+tianoc...@kernel.org 
<mailto:ardb+tianoc...@kernel.org>>
Cc: Sami Mujawar <sami.muja...@arm.com <mailto:sami.muja...@arm.com>>
Cc: Ray Ni <ray...@intel.com <mailto:ray...@intel.com>>
Cc: John Mathew <john.math...@intel.com <mailto:john.math...@intel.com>>
Authored-by: Gerd Hoffmann <kra...@redhat.com <mailto:kra...@redhat.com>>
Signed-off-by: Gua Guo <gua....@intel.com <mailto:gua....@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to