public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow.
@ 2018-07-31  5:15 Eric Dong
  2018-08-01  2:32 ` Dong, Eric
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Dong @ 2018-07-31  5:15 UTC (permalink / raw)
  To: edk2-devel; +Cc: Yao Jiewen, Wu Hao

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/OpalPasswordSupportNotify.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



^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow.
  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
  2018-08-01  2:44   ` Yao, Jiewen
  0 siblings, 1 reply; 3+ messages in thread
From: Dong, Eric @ 2018-08-01  2:32 UTC (permalink / raw)
  To: edk2-devel@lists.01.org; +Cc: Wu, Hao A, Yao, Jiewen

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow.
  2018-08-01  2:32 ` Dong, Eric
@ 2018-08-01  2:44   ` Yao, Jiewen
  0 siblings, 0 replies; 3+ messages in thread
From: Yao, Jiewen @ 2018-08-01  2:44 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Wu, Hao A

Reviewed-by: jiewen.yao@intel.com

> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, August 1, 2018 10:33 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib:
> Add check to avoid potential buffer overflow.
> 
> 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-08-01  2:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2018-08-01  2:44   ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox