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

["Sami Mujawar" <sami.mujawar@arm.com>]

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