* [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