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

2024-01-11 Thread Sami Mujawar 
Hi Gua,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

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









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 mailto:mbeat...@google.com> 
>

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

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




From: Gua Guo mailto:gua@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 mailto:mbeat...@google.com> 
>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org> >>
Cc: Sami Mujawar mailto:sami.muja...@arm.com> 
>>
Cc: Ray Ni mailto:ray...@intel.com> >>
Cc: John Mathew mailto:john.math...@intel.com> 
>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com> 
>>
Signed-off-by: Gua Guo 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

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

2024-01-11 Thread Sami Mujawar 
Hi Gua,

Please find my response inline marked [SAMI].

Regards,

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




From: Gua Guo mailto:gua@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 mailto:mbeat...@google.com> 
>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org> >>
Cc: Sami Mujawar mailto:sami.muja...@arm.com> 
>>
Cc: Ray Ni mailto:ray...@intel.com> >>
Cc: John Mathew mailto:john.math...@intel.com> 
>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com> 
>>
Signed-off-by: Gua Guo 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 (#113622): https://edk2.groups.io/g/devel/message/113622
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-01-11 Thread Guo, Gua 
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  
Sent: Thursday, January 11, 2024 10:06 PM
To: Guo, Gua ; devel@edk2.groups.io
Cc: Marc Beatove ; Ard Biesheuvel 
; Ni, Ray ; Mathews, John 
; Gerd Hoffmann ; nd 
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>> wrote:


From: Gua Guo mailto:gua@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 mailto:mbeat...@google.com>>
Reviewed-by: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Ray Ni mailto:ray...@intel.com>>
Cc: John Mathew mailto:john.math...@intel.com>>
Authored-by: Gerd Hoffmann mailto:kra...@redhat.com>>
Signed-off-by: Gua Guo 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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2024-01-11 Thread Sami Mujawar 
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>> wrote:


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

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

2024-01-11 Thread Guo, Gua 
From: Gua Guo 

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 
Reviewed-by: Ard Biesheuvel 
Cc: Sami Mujawar 
Cc: Ray Ni 
Cc: John Mathew 
Authored-by: Gerd Hoffmann 
Signed-off-by: Gua Guo 
---
 .../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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-