* [PATCH EDK2 v1 0/1] MdeModulePkg/PiSmmCore:Avoid overflow risk
@ 2022-08-05 7:20 wenyi,xie
2022-08-05 7:20 ` [PATCH EDK2 v1 1/1] " wenyi,xie
0 siblings, 1 reply; 4+ messages in thread
From: wenyi,xie @ 2022-08-05 7:20 UTC (permalink / raw)
To: devel, jian.j.wang, gaoliming, eric.dong, ray.ni; +Cc: songdongkuang, xiewenyi2
Main Changes :
1.Add check to avoid overflow.
Wenyi Xie (1):
MdeModulePkg/PiSmmCore:Avoid overflow risk
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 22 +++++++++++++-------
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 5 +++++
2 files changed, 19 insertions(+), 8 deletions(-)
--
2.20.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk
2022-08-05 7:20 [PATCH EDK2 v1 0/1] MdeModulePkg/PiSmmCore:Avoid overflow risk wenyi,xie
@ 2022-08-05 7:20 ` wenyi,xie
2022-08-15 1:12 ` 回复: [edk2-devel] " gaoliming
0 siblings, 1 reply; 4+ messages in thread
From: wenyi,xie @ 2022-08-05 7:20 UTC (permalink / raw)
To: devel, jian.j.wang, gaoliming, eric.dong, ray.ni; +Cc: songdongkuang, xiewenyi2
As the CommunicationBuffer plus BufferSize may overflow, check the
value first before using.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 22 +++++++++++++-------
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 5 +++++
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index 9e5c6cbe33dd..fcf8c61d7f1b 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -613,23 +613,28 @@ SmmEndOfS3ResumeHandler (
@retval FALSE Buffer doesn't overlap.
**/
-BOOLEAN
+EFI_STATUS
InternalIsBufferOverlapped (
IN UINT8 *Buff1,
IN UINTN Size1,
IN UINT8 *Buff2,
- IN UINTN Size2
+ IN UINTN Size2,
+ IN BOOLEAN *IsOverlapped
)
{
+ *IsOverlapped = TRUE;
+ if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN - Size2)) {
+ return EFI_INVALID_PARAMETER;
+ }
//
// If buff1's end is less than the start of buff2, then it's ok.
// Also, if buff1's start is beyond buff2's end, then it's ok.
//
if (((Buff1 + Size1) <= Buff2) || (Buff1 >= (Buff2 + Size2))) {
- return FALSE;
+ *IsOverlapped = FALSE;
}
- return TRUE;
+ return EFI_SUCCESS;
}
/**
@@ -693,17 +698,18 @@ SmmEntryPoint (
//
// Synchronous SMI for SMM Core or request from Communicate protocol
//
- IsOverlapped = InternalIsBufferOverlapped (
+ Status = InternalIsBufferOverlapped (
(UINT8 *)CommunicationBuffer,
BufferSize,
(UINT8 *)gSmmCorePrivate,
- sizeof (*gSmmCorePrivate)
+ sizeof (*gSmmCorePrivate),
+ &IsOverlapped
);
- if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || IsOverlapped) {
+ if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer, BufferSize) || EFI_ERROR(Status) || IsOverlapped) {
//
// If CommunicationBuffer is not in valid address scope,
// or there is overlap between gSmmCorePrivate and CommunicationBuffer,
- // return EFI_INVALID_PARAMETER
+ // return EFI_ACCESS_DENIED
//
gSmmCorePrivate->CommunicationBuffer = NULL;
gSmmCorePrivate->ReturnStatus = EFI_ACCESS_DENIED;
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 4f00cebaf5ed..bd13cf97ec93 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -525,6 +525,11 @@ SmmCommunicationCommunicate (
CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommBuffer;
+ if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)) {
+ DEBUG ((DEBUG_ERROR, "MessageLength is invalid!\n"));
+ return EFI_INVALID_PARAMETER;
+ }
+
if (CommSize == NULL) {
TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength;
} else {
--
2.20.1.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* 回复: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk
2022-08-05 7:20 ` [PATCH EDK2 v1 1/1] " wenyi,xie
@ 2022-08-15 1:12 ` gaoliming
2022-08-15 1:44 ` wenyi,xie
0 siblings, 1 reply; 4+ messages in thread
From: gaoliming @ 2022-08-15 1:12 UTC (permalink / raw)
To: devel, xiewenyi2, jian.j.wang, eric.dong, ray.ni; +Cc: songdongkuang
Wenyi:
I add my comments below.
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 wenyi,xie via
> groups.io
> 发送时间: 2022年8月5日 15:21
> 收件人: devel@edk2.groups.io; jian.j.wang@intel.com;
> gaoliming@byosoft.com.cn; eric.dong@intel.com; ray.ni@intel.com
> 抄送: songdongkuang@huawei.com; xiewenyi2@huawei.com
> 主题: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid
> overflow risk
>
> As the CommunicationBuffer plus BufferSize may overflow, check the
> value first before using.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 22 +++++++++++++-------
> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 5 +++++
> 2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index 9e5c6cbe33dd..fcf8c61d7f1b 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -613,23 +613,28 @@ SmmEndOfS3ResumeHandler (
> @retval FALSE Buffer doesn't overlap.
>
> **/
> -BOOLEAN
> +EFI_STATUS
> InternalIsBufferOverlapped (
> IN UINT8 *Buff1,
> IN UINTN Size1,
> IN UINT8 *Buff2,
> - IN UINTN Size2
> + IN UINTN Size2,
> + IN BOOLEAN *IsOverlapped
> )
> {
> + *IsOverlapped = TRUE;
> + if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN -
> Size2)) {
> + return EFI_INVALID_PARAMETER;
> + }
> //
> // If buff1's end is less than the start of buff2, then it's ok.
> // Also, if buff1's start is beyond buff2's end, then it's ok.
> //
> if (((Buff1 + Size1) <= Buff2) || (Buff1 >= (Buff2 + Size2))) {
> - return FALSE;
> + *IsOverlapped = FALSE;
> }
>
> - return TRUE;
> + return EFI_SUCCESS;
> }
>
If Buff1 + Size1 overflows MAX_UINTN, it can be also regarded as overlap,
return TRUE.
If you think overflow is not overlap, please also update function
InternalIsBufferOverlapped name and description to avoid confuse.
Thanks
Liming
> /**
> @@ -693,17 +698,18 @@ SmmEntryPoint (
> //
> // Synchronous SMI for SMM Core or request from Communicate
> protocol
> //
> - IsOverlapped = InternalIsBufferOverlapped (
> + Status = InternalIsBufferOverlapped (
> (UINT8 *)CommunicationBuffer,
> BufferSize,
> (UINT8 *)gSmmCorePrivate,
> - sizeof (*gSmmCorePrivate)
> + sizeof (*gSmmCorePrivate),
> + &IsOverlapped
> );
> - if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
> BufferSize) || IsOverlapped) {
> + if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
> BufferSize) || EFI_ERROR(Status) || IsOverlapped) {
> //
> // If CommunicationBuffer is not in valid address scope,
> // or there is overlap between gSmmCorePrivate and
> CommunicationBuffer,
> - // return EFI_INVALID_PARAMETER
> + // return EFI_ACCESS_DENIED
> //
> gSmmCorePrivate->CommunicationBuffer = NULL;
> gSmmCorePrivate->ReturnStatus = EFI_ACCESS_DENIED;
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 4f00cebaf5ed..bd13cf97ec93 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -525,6 +525,11 @@ SmmCommunicationCommunicate (
>
> CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER
> *)CommBuffer;
>
> + if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data)) {
> + DEBUG ((DEBUG_ERROR, "MessageLength is invalid!\n"));
> + return EFI_INVALID_PARAMETER;
> + }
> +
> if (CommSize == NULL) {
> TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER,
> Data) + CommunicateHeader->MessageLength;
> } else {
> --
> 2.20.1.windows.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: 回复: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk
2022-08-15 1:12 ` 回复: [edk2-devel] " gaoliming
@ 2022-08-15 1:44 ` wenyi,xie
0 siblings, 0 replies; 4+ messages in thread
From: wenyi,xie @ 2022-08-15 1:44 UTC (permalink / raw)
To: gaoliming, devel, jian.j.wang, eric.dong, ray.ni; +Cc: songdongkuang
On 2022/8/15 9:12, gaoliming wrote:
> Wenyi:
> I add my comments below.
Thank you for your comments. I agree with you that overflow can be treated as overlap. I will update the patch and send it later.
Thanks
Wenyi
>
>> -----邮件原件-----
>> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 wenyi,xie via
>> groups.io
>> 发送时间: 2022年8月5日 15:21
>> 收件人: devel@edk2.groups.io; jian.j.wang@intel.com;
>> gaoliming@byosoft.com.cn; eric.dong@intel.com; ray.ni@intel.com
>> 抄送: songdongkuang@huawei.com; xiewenyi2@huawei.com
>> 主题: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid
>> overflow risk
>>
>> As the CommunicationBuffer plus BufferSize may overflow, check the
>> value first before using.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
>> ---
>> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 22 +++++++++++++-------
>> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 5 +++++
>> 2 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> index 9e5c6cbe33dd..fcf8c61d7f1b 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> @@ -613,23 +613,28 @@ SmmEndOfS3ResumeHandler (
>> @retval FALSE Buffer doesn't overlap.
>>
>> **/
>> -BOOLEAN
>> +EFI_STATUS
>> InternalIsBufferOverlapped (
>> IN UINT8 *Buff1,
>> IN UINTN Size1,
>> IN UINT8 *Buff2,
>> - IN UINTN Size2
>> + IN UINTN Size2,
>> + IN BOOLEAN *IsOverlapped
>> )
>> {
>> + *IsOverlapped = TRUE;
>> + if (((UINTN)Buff1 > MAX_UINTN - Size1) || ((UINTN)Buff2 > MAX_UINTN -
>> Size2)) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> //
>> // If buff1's end is less than the start of buff2, then it's ok.
>> // Also, if buff1's start is beyond buff2's end, then it's ok.
>> //
>> if (((Buff1 + Size1) <= Buff2) || (Buff1 >= (Buff2 + Size2))) {
>> - return FALSE;
>> + *IsOverlapped = FALSE;
>> }
>>
>> - return TRUE;
>> + return EFI_SUCCESS;
>> }
>>
> If Buff1 + Size1 overflows MAX_UINTN, it can be also regarded as overlap,
> return TRUE.
>
> If you think overflow is not overlap, please also update function
> InternalIsBufferOverlapped name and description to avoid confuse.
>
> Thanks
> Liming
>
>> /**
>> @@ -693,17 +698,18 @@ SmmEntryPoint (
>> //
>> // Synchronous SMI for SMM Core or request from Communicate
>> protocol
>> //
>> - IsOverlapped = InternalIsBufferOverlapped (
>> + Status = InternalIsBufferOverlapped (
>> (UINT8 *)CommunicationBuffer,
>> BufferSize,
>> (UINT8 *)gSmmCorePrivate,
>> - sizeof (*gSmmCorePrivate)
>> + sizeof (*gSmmCorePrivate),
>> + &IsOverlapped
>> );
>> - if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
>> BufferSize) || IsOverlapped) {
>> + if (!SmmIsBufferOutsideSmmValid ((UINTN)CommunicationBuffer,
>> BufferSize) || EFI_ERROR(Status) || IsOverlapped) {
>> //
>> // If CommunicationBuffer is not in valid address scope,
>> // or there is overlap between gSmmCorePrivate and
>> CommunicationBuffer,
>> - // return EFI_INVALID_PARAMETER
>> + // return EFI_ACCESS_DENIED
>> //
>> gSmmCorePrivate->CommunicationBuffer = NULL;
>> gSmmCorePrivate->ReturnStatus = EFI_ACCESS_DENIED;
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> index 4f00cebaf5ed..bd13cf97ec93 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> @@ -525,6 +525,11 @@ SmmCommunicationCommunicate (
>>
>> CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER
>> *)CommBuffer;
>>
>> + if (CommunicateHeader->MessageLength > MAX_UINTN - OFFSET_OF
>> (EFI_SMM_COMMUNICATE_HEADER, Data)) {
>> + DEBUG ((DEBUG_ERROR, "MessageLength is invalid!\n"));
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> if (CommSize == NULL) {
>> TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER,
>> Data) + CommunicateHeader->MessageLength;
>> } else {
>> --
>> 2.20.1.windows.1
>>
>>
>>
>>
>>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-15 1:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-05 7:20 [PATCH EDK2 v1 0/1] MdeModulePkg/PiSmmCore:Avoid overflow risk wenyi,xie
2022-08-05 7:20 ` [PATCH EDK2 v1 1/1] " wenyi,xie
2022-08-15 1:12 ` 回复: [edk2-devel] " gaoliming
2022-08-15 1:44 ` wenyi,xie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox