From: "Dong, Eric" <eric.dong@intel.com>
To: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Wu, Hao A" <hao.a.wu@intel.com>, "Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow.
Date: Wed, 1 Aug 2018 02:32:31 +0000 [thread overview]
Message-ID: <ED077930C258884BBCB450DB737E66224AC5CF45@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <20180731051556.15980-1-eric.dong@intel.com>
Hi Hao & Jiewen,
This patch has been verified by the original reporter, also pass regression test done by myself.
Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Eric
> Dong
> Sent: Tuesday, July 31, 2018 1:16 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add
> check to avoid potential buffer overflow.
>
> Current code not check the CommunicationBuffer size before use it. Attacker
> can read beyond the end of the (untrusted) commbuffer into controlled
> memory. Attacker can get access outside of valid SMM memory regions. This
> patch add check before use it.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> Cc: Wu Hao <hao.a.wu@intel.com>
> ---
> .../Include/Library/OpalPasswordSupportLib.h | 3 +-
> .../OpalPasswordSupportLib.c | 55 +++++++++++++++-------
> .../OpalPasswordSupportNotify.h | 2 +-
> .../Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h | 6 +--
> 4 files changed, 42 insertions(+), 24 deletions(-)
>
> diff --git a/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> b/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> index e616c763f0..ad45e9e3b7 100644
> --- a/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> +++ b/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> @@ -19,6 +19,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> #include <Protocol/DevicePath.h>
> #include <Library/TcgStorageOpalLib.h>
>
> +#define OPAL_PASSWORD_MAX_LENGTH 32
>
> #pragma pack(1)
>
> @@ -76,7 +77,7 @@ typedef struct {
> typedef struct {
> LIST_ENTRY Link;
>
> - UINT8 Password[32];
> + UINT8 Password[OPAL_PASSWORD_MAX_LENGTH];
> UINT8 PasswordLength;
>
> EFI_DEVICE_PATH_PROTOCOL OpalDevicePath;
> diff --git
> a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c
> b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c
> index 837582359e..e377e9ca79 100644
> --- a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c
> +++ b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.
> +++ c
> @@ -14,8 +14,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>
> #include "OpalPasswordSupportNotify.h"
>
> -#define OPAL_PASSWORD_MAX_LENGTH 32
> -
> LIST_ENTRY mDeviceList = INITIALIZE_LIST_HEAD_VARIABLE
> (mDeviceList);
> BOOLEAN gInSmm = FALSE;
> EFI_GUID gOpalPasswordNotifyProtocolGuid =
> OPAL_PASSWORD_NOTIFY_PROTOCOL_GUID;
> @@ -663,34 +661,53 @@ SmmOpalPasswordHandler (
> EFI_STATUS Status;
> OPAL_SMM_COMMUNICATE_HEADER *SmmFunctionHeader;
> UINTN TempCommBufferSize;
> - UINT8 *NewPassword;
> - UINT8 PasswordLength;
> - EFI_DEVICE_PATH_PROTOCOL *DevicePath;
> + UINTN RemainedDevicePathSize;
> + OPAL_COMM_DEVICE_LIST *DeviceBuffer;
>
> if (CommBuffer == NULL || CommBufferSize == NULL) {
> return EFI_SUCCESS;
> }
>
> + Status = EFI_SUCCESS;
> + DeviceBuffer = NULL;
> +
> TempCommBufferSize = *CommBufferSize;
> if (TempCommBufferSize < OFFSET_OF
> (OPAL_SMM_COMMUNICATE_HEADER, Data)) {
> return EFI_SUCCESS;
> }
> -
> - Status = EFI_SUCCESS;
> - SmmFunctionHeader = (OPAL_SMM_COMMUNICATE_HEADER
> *)CommBuffer;
> -
> - DevicePath = &((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader-
> >Data))->OpalDevicePath;
> - PasswordLength = ((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader-
> >Data))->PasswordLength;
> - NewPassword = ((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader-
> >Data))->Password;
> + SmmFunctionHeader = (OPAL_SMM_COMMUNICATE_HEADER
> *)CommBuffer;
>
> switch (SmmFunctionHeader->Function) {
> case SMM_FUNCTION_SET_OPAL_PASSWORD:
> - if (OpalPasswordIsFullZero (NewPassword) || PasswordLength == 0) {
> - Status = EFI_INVALID_PARAMETER;
> - goto EXIT;
> - }
> + if (TempCommBufferSize <= OFFSET_OF
> (OPAL_SMM_COMMUNICATE_HEADER, Data) + OFFSET_OF
> (OPAL_COMM_DEVICE_LIST, OpalDevicePath)) {
> + return EFI_SUCCESS;
> + }
> +
> + DeviceBuffer = AllocateCopyPool (TempCommBufferSize - OFFSET_OF
> (OPAL_SMM_COMMUNICATE_HEADER, Data), SmmFunctionHeader->Data);
> + if (DeviceBuffer == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto EXIT;
> + }
>
> - Status = OpalSavePasswordToSmm (DevicePath, NewPassword,
> PasswordLength);
> + //
> + // Validate the DevicePath.
> + //
> + RemainedDevicePathSize = TempCommBufferSize - OFFSET_OF
> (OPAL_SMM_COMMUNICATE_HEADER, Data)
> + - OFFSET_OF (OPAL_COMM_DEVICE_LIST,
> OpalDevicePath);
> + if (!IsDevicePathValid(&DeviceBuffer->OpalDevicePath,
> RemainedDevicePathSize) ||
> + (RemainedDevicePathSize != GetDevicePathSize (&DeviceBuffer-
> >OpalDevicePath))) {
> + Status = EFI_SUCCESS;
> + goto EXIT;
> + }
> +
> + if (OpalPasswordIsFullZero (DeviceBuffer->Password) ||
> + DeviceBuffer->PasswordLength == 0 ||
> + DeviceBuffer->PasswordLength > OPAL_PASSWORD_MAX_LENGTH) {
> + Status = EFI_INVALID_PARAMETER;
> + goto EXIT;
> + }
> +
> + Status = OpalSavePasswordToSmm (&DeviceBuffer->OpalDevicePath,
> + DeviceBuffer->Password, DeviceBuffer->PasswordLength);
> break;
>
> default:
> @@ -701,6 +718,10 @@ SmmOpalPasswordHandler (
> EXIT:
> SmmFunctionHeader->ReturnStatus = Status;
>
> + if (DeviceBuffer != NULL) {
> + FreePool (DeviceBuffer);
> + }
> +
> //
> // Return EFI_SUCCESS cause only one handler can be trigged.
> // so return EFI_WARN_INTERRUPT_SOURCE_PENDING to make all handler
> can be trigged.
> diff --git
> a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h
> b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h
> index f0ad3a1136..d543faed5d 100644
> ---
> a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h
> +++
> b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNoti
> +++ fy.h
> @@ -41,7 +41,7 @@ typedef struct {
> } OPAL_SMM_COMMUNICATE_HEADER;
>
> typedef struct {
> - UINT8 Password[32];
> + UINT8 Password[OPAL_PASSWORD_MAX_LENGTH];
> UINT8 PasswordLength;
>
> EFI_DEVICE_PATH_PROTOCOL OpalDevicePath;
> diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> b/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> index ce88786fab..bc559f0bd1 100644
> --- a/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> +++ b/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> @@ -64,10 +64,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> // The payload Length of HDD related ATA commands //
> #define HDD_PAYLOAD 512
> -//
> -// According to ATA spec, the max Length of hdd password is 32 bytes -//
> -#define OPAL_PASSWORD_MAX_LENGTH 32
>
> extern VOID *mBuffer;
>
> @@ -124,7 +120,7 @@ typedef struct {
>
> UINT32 NvmeNamespaceId;
>
> - UINT8 Password[32];
> + UINT8 Password[OPAL_PASSWORD_MAX_LENGTH];
> UINT8 PasswordLength;
>
> UINT32 Length;
> --
> 2.15.0.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2018-08-01 2:32 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-31 5:15 [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow Eric Dong
2018-08-01 2:32 ` Dong, Eric [this message]
2018-08-01 2:44 ` Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ED077930C258884BBCB450DB737E66224AC5CF45@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox