SecurityPkg OpalPasswordDxe:Fix wrong BufferSize input to UnicodeSPrint

[edk2] [PATCH V2] SecurityPkg OpalPasswordDxe:Fix wrong BufferSize input to UnicodeSPrint

Posted by Star Zeng 6 years, 1 month ago

Current code uses string length as BufferSize input to UnicodeSPrint,
it is wrong and makes the pop up string trimmed. The BufferSize input
to UnicodeSPrint should be the size, in bytes, of the output buffer.

This is to use sizeof (mPopUpString) as the BufferSize input to
UnicodeSPrint, it also updates array size of mPopUpString from 256 to
100 that is enough, otherwise the pop up string may be too long.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
index 1b55bbe4ecb8..6344deb86750 100644
--- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
+++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
@@ -27,7 +27,7 @@ EFI_GUID mOpalDeviceNvmeGuid = OPAL_DEVICE_NVME_GUID;
 BOOLEAN                 mOpalEndOfDxe = FALSE;
 OPAL_REQUEST_VARIABLE   *mOpalRequestVariable = NULL;
 UINTN                   mOpalRequestVariableSize = 0;
-CHAR16                  mPopUpString[256];
+CHAR16                  mPopUpString[100];
 
 typedef struct {
   UINT32                   Address;
@@ -908,9 +908,9 @@ OpalDriverPopUpPasswordInput (
 }
 
 /**
-  Check if disk is locked, show popup window and ask for password if it is.
+  Get pop up string.
 
-  @param[in] Dev            The device which need to be unlocked.
+  @param[in] Dev            The OPAL device.
   @param[in] RequestString  Request string.
 
 **/
@@ -920,15 +920,10 @@ OpalGetPopUpString (
   IN CHAR16             *RequestString
   )
 {
-  UINTN                 StrLength;
-
-  StrLength = StrLen (RequestString) + 1 + MAX (StrLen (Dev->Name16), StrLen (L"Disk"));
-  ASSERT (StrLength < sizeof (mPopUpString) / sizeof (CHAR16));
-
   if (Dev->Name16 == NULL) {
-    UnicodeSPrint (mPopUpString, StrLength + 1, L"%s Disk", RequestString);
+    UnicodeSPrint (mPopUpString, sizeof (mPopUpString), L"%s Disk", RequestString);
   } else {
-    UnicodeSPrint (mPopUpString, StrLength + 1, L"%s %s", RequestString, Dev->Name16);
+    UnicodeSPrint (mPopUpString, sizeof (mPopUpString), L"%s %s", RequestString, Dev->Name16);
   }
 
   return mPopUpString;
-- 
2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH V2] SecurityPkg OpalPasswordDxe:Fix wrong BufferSize input to UnicodeSPrint

Posted by Yao, Jiewen 6 years, 1 month ago

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, March 15, 2018 1:53 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
> Subject: [PATCH V2] SecurityPkg OpalPasswordDxe:Fix wrong BufferSize input to
> UnicodeSPrint
> 
> Current code uses string length as BufferSize input to UnicodeSPrint,
> it is wrong and makes the pop up string trimmed. The BufferSize input
> to UnicodeSPrint should be the size, in bytes, of the output buffer.
> 
> This is to use sizeof (mPopUpString) as the BufferSize input to
> UnicodeSPrint, it also updates array size of mPopUpString from 256 to
> 100 that is enough, otherwise the pop up string may be too long.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> index 1b55bbe4ecb8..6344deb86750 100644
> --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> @@ -27,7 +27,7 @@ EFI_GUID mOpalDeviceNvmeGuid =
> OPAL_DEVICE_NVME_GUID;
>  BOOLEAN                 mOpalEndOfDxe = FALSE;
>  OPAL_REQUEST_VARIABLE   *mOpalRequestVariable = NULL;
>  UINTN                   mOpalRequestVariableSize = 0;
> -CHAR16                  mPopUpString[256];
> +CHAR16                  mPopUpString[100];
> 
>  typedef struct {
>    UINT32                   Address;
> @@ -908,9 +908,9 @@ OpalDriverPopUpPasswordInput (
>  }
> 
>  /**
> -  Check if disk is locked, show popup window and ask for password if it is.
> +  Get pop up string.
> 
> -  @param[in] Dev            The device which need to be unlocked.
> +  @param[in] Dev            The OPAL device.
>    @param[in] RequestString  Request string.
> 
>  **/
> @@ -920,15 +920,10 @@ OpalGetPopUpString (
>    IN CHAR16             *RequestString
>    )
>  {
> -  UINTN                 StrLength;
> -
> -  StrLength = StrLen (RequestString) + 1 + MAX (StrLen (Dev->Name16), StrLen
> (L"Disk"));
> -  ASSERT (StrLength < sizeof (mPopUpString) / sizeof (CHAR16));
> -
>    if (Dev->Name16 == NULL) {
> -    UnicodeSPrint (mPopUpString, StrLength + 1, L"%s Disk", RequestString);
> +    UnicodeSPrint (mPopUpString, sizeof (mPopUpString), L"%s Disk",
> RequestString);
>    } else {
> -    UnicodeSPrint (mPopUpString, StrLength + 1, L"%s %s", RequestString,
> Dev->Name16);
> +    UnicodeSPrint (mPopUpString, sizeof (mPopUpString), L"%s %s",
> RequestString, Dev->Name16);
>    }
> 
>    return mPopUpString;
> --
> 2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel