Hi Gua, Please find my response inline marked [SAMI].
Regards, Sami Mujawar On 11/01/2024, 15:19, "Guo, Gua" <gua....@intel.com <mailto:gua....@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.muja...@arm.com <mailto:sami.muja...@arm.com>> Sent: Thursday, January 11, 2024 11:02 PM To: Guo, Gua <gua....@intel.com <mailto:gua....@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> Cc: Marc Beatove <mbeat...@google.com <mailto:mbeat...@google.com>>; Ard Biesheuvel <ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org>>; Ni, Ray <ray...@intel.com <mailto:ray...@intel.com>>; Mathews, John <john.math...@intel.com <mailto:john.math...@intel.com>>; Gerd Hoffmann <kra...@redhat.com <mailto:kra...@redhat.com>>; nd <n...@arm.com <mailto:n...@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....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@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.muja...@arm.com <mailto:sami.muja...@arm.com> <mailto:sami.muja...@arm.com <mailto:sami.muja...@arm.com>>> Sent: Thursday, January 11, 2024 10:06 PM To: Guo, Gua <gua....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@intel.com>>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io> <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>> Cc: Marc Beatove <mbeat...@google.com <mailto:mbeat...@google.com> <mailto:mbeat...@google.com <mailto:mbeat...@google.com>>>; Ard Biesheuvel <ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org> <mailto:ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org>>>; Ni, Ray <ray...@intel.com <mailto:ray...@intel.com> <mailto:ray...@intel.com <mailto:ray...@intel.com>>>; Mathews, John <john.math...@intel.com <mailto:john.math...@intel.com> <mailto:john.math...@intel.com <mailto:john.math...@intel.com>>>; Gerd Hoffmann <kra...@redhat.com <mailto:kra...@redhat.com> <mailto:kra...@redhat.com <mailto:kra...@redhat.com>>>; nd <n...@arm.com <mailto:n...@arm.com> <mailto:n...@arm.com <mailto: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> <mailto:gua....@intel.com <mailto:gua....@intel.com>> <mailto:gua....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@intel.com>>>" <gua....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@intel.com>> <mailto:gua....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@intel.com>>>> wrote: From: Gua Guo <gua....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@intel.com>> <mailto:gua....@intel.com <mailto:gua....@intel.com> <mailto: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> <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 <mbeat...@google.com <mailto:mbeat...@google.com> <mailto:mbeat...@google.com <mailto:mbeat...@google.com>> <mailto:mbeat...@google.com <mailto:mbeat...@google.com> <mailto:mbeat...@google.com <mailto:mbeat...@google.com>>>> Reviewed-by: Ard Biesheuvel <ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org> <mailto:ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org>> <mailto:ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org> <mailto:ardb+tianoc...@kernel.org <mailto:ardb+tianoc...@kernel.org>>>> Cc: Sami Mujawar <sami.muja...@arm.com <mailto:sami.muja...@arm.com> <mailto:sami.muja...@arm.com <mailto:sami.muja...@arm.com>> <mailto:sami.muja...@arm.com <mailto:sami.muja...@arm.com> <mailto:sami.muja...@arm.com <mailto:sami.muja...@arm.com>>>> Cc: Ray Ni <ray...@intel.com <mailto:ray...@intel.com> <mailto:ray...@intel.com <mailto:ray...@intel.com>> <mailto:ray...@intel.com <mailto:ray...@intel.com> <mailto:ray...@intel.com <mailto:ray...@intel.com>>>> Cc: John Mathew <john.math...@intel.com <mailto:john.math...@intel.com> <mailto:john.math...@intel.com <mailto:john.math...@intel.com>> <mailto:john.math...@intel.com <mailto:john.math...@intel.com> <mailto:john.math...@intel.com <mailto:john.math...@intel.com>>>> Authored-by: Gerd Hoffmann <kra...@redhat.com <mailto:kra...@redhat.com> <mailto:kra...@redhat.com <mailto:kra...@redhat.com>> <mailto:kra...@redhat.com <mailto:kra...@redhat.com> <mailto:kra...@redhat.com <mailto:kra...@redhat.com>>>> Signed-off-by: Gua Guo <gua....@intel.com <mailto:gua....@intel.com> <mailto:gua....@intel.com <mailto:gua....@intel.com>> <mailto:gua....@intel.com <mailto:gua....@intel.com> <mailto: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 <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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
