回复: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/PiSmmCore:Avoid overflow risk
gaoliming via groups.io
gaoliming=byosoft.com.cn at groups.io
Mon Aug 15 01:12:22 UTC 2022
Wenyi:
I add my comments below.
> -----邮件原件-----
> 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 wenyi,xie via
> groups.io
> 发送时间: 2022年8月5日 15:21
> 收件人: devel at edk2.groups.io; jian.j.wang at intel.com;
> gaoliming at byosoft.com.cn; eric.dong at intel.com; ray.ni at intel.com
> 抄送: songdongkuang at huawei.com; xiewenyi2 at 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 at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Signed-off-by: Wenyi Xie <xiewenyi2 at 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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92406): https://edk2.groups.io/g/devel/message/92406
Mute This Topic: https://groups.io/mt/93027797/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list