Re: [Patch 3/3] SecurityPkg/OpalPassword: Add support for pyrite 2.0 devices.

["Dong, Eric" <eric.dong@intel.com>]

Hi Hao,

Thanks for your comments, I will update it when I check in the code.

Thanks,
Eric

-----Original Message-----
From: Wu, Hao A 
Sent: Monday, May 7, 2018 11:08 AM
To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
Subject: RE: [Patch 3/3] SecurityPkg/OpalPassword: Add support for pyrite 2.0 devices.

Some comments below:

> -----Original Message-----
> From: Dong, Eric
> Sent: Thursday, May 03, 2018 11:17 AM
> To: edk2-devel@lists.01.org; Wu, Hao A
> Subject: [Patch 3/3] SecurityPkg/OpalPassword: Add support for pyrite 
> 2.0 devices.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c     | 60 ++++++++++++++--
>  SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h     |  9 +++
>  SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c        | 84
> ++++++++++++++++++++++
>  .../Tcg/Opal/OpalPassword/OpalPasswordPei.c        |  1 +
>  4 files changed, 147 insertions(+), 7 deletions(-)
> 
> diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> index 4133e503e2..91ab372f0c 100644
> --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.c
> @@ -105,10 +105,9 @@ OpalSupportGetAvailableActions(
>    }
> 
>    //
> -  // Psid revert is available for any device with media encryption 
> support
> -  // Revert is allowed for any device with media encryption support, 
> however it requires
> +  // Psid revert is available for any device with media encryption 
> + support or
> pyrite 2.0 type support.
>    //
> -  if (SupportedAttributes->MediaEncryption) {
> +  if (SupportedAttributes->PyriteSscV2 || SupportedAttributes-
> >MediaEncryption) {
> 
>      //
>      // Only allow psid revert if media encryption is enabled.

For the comments here:
    //
    // Only allow psid revert if media encryption is enabled.
    // Otherwise, someone who steals a disk can psid revert the disk and the user Data is still
    // intact and accessible
    //
I think we'd better update it as well.

> @@ -657,6 +656,8 @@ OpalEndOfDxeEventNotify (
>    @param[in]  Dev           The device which need Psid to process Psid Revert
>                              OPAL request.
>    @param[in]  PopUpString   Pop up string.
> +  @param[in]  PopUpString2  Pop up string in line 2.
> +
>    @param[out] PressEsc      Whether user escape function through Press ESC.
> 
>    @retval Psid string if success. NULL if failed.
> @@ -666,6 +667,7 @@ CHAR8 *
>  OpalDriverPopUpPsidInput (
>    IN OPAL_DRIVER_DEVICE     *Dev,
>    IN CHAR16                 *PopUpString,
> +  IN CHAR16                 *PopUpString2,
>    OUT BOOLEAN               *PressEsc
>    )
>  {
> @@ -689,6 +691,7 @@ OpalDriverPopUpPsidInput (
>        EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
>        &InputKey,
>        PopUpString,
> +      PopUpString2,
>        L"---------------------",
>        Mask,
>        NULL
> @@ -1369,6 +1372,8 @@ ProcessOpalRequestPsidRevert (
>    EFI_INPUT_KEY         Key;
>    TCG_RESULT            Ret;
>    CHAR16                *PopUpString;
> +  CHAR16                *PopUpString2;
> +  UINTN                 BufferSize;
> 
>    if (Dev == NULL) {
>      return;
> @@ -1378,6 +1383,20 @@ ProcessOpalRequestPsidRevert (
> 
>    PopUpString = OpalGetPopUpString (Dev, RequestString);
> 
> +  if (Dev->OpalDisk.EstimateTimeCost > MAX_ACCEPTABLE_REVERTING_TIME)
> {
> +    BufferSize = StrSize (L"Warning: Revert action will take about 
> + ######
> seconds, DO NOT power off system during the revert action!");

The 'BufferSize' seems not big enough to me.
Per my understanding, the possible max timeout value can be:
65534 * 2 * 60 = 7864080 seconds

So using '######' to keep space for 6 digits maybe not enough. Some characters might get truncated.

> +    PopUpString2 = AllocateZeroPool (BufferSize);
> +    ASSERT (PopUpString2 != NULL);
> +    UnicodeSPrint (
> +        PopUpString2,
> +        BufferSize,
> +        L"WARNING: Revert action will take about %d seconds, DO NOT 
> + power
> off system during the revert action!",
> +        Dev->OpalDisk.EstimateTimeCost
> +      );
> +  } else {
> +    PopUpString2 = NULL;
> +  }
> +
>    Count = 0;
> 
>    ZeroMem(&Session, sizeof(Session)); @@ -1386,7 +1405,7 @@ 
> ProcessOpalRequestPsidRevert (
>    Session.OpalBaseComId = Dev->OpalDisk.OpalBaseComId;
> 
>    while (Count < MAX_PSID_TRY_COUNT) {
> -    Psid = OpalDriverPopUpPsidInput (Dev, PopUpString, &PressEsc);
> +    Psid = OpalDriverPopUpPsidInput (Dev, PopUpString, PopUpString2,
> &PressEsc);
>      if (PressEsc) {
>          do {
>            CreatePopUp (
> @@ -1400,7 +1419,7 @@ ProcessOpalRequestPsidRevert (
> 
>          if (Key.UnicodeChar == CHAR_CARRIAGE_RETURN) {
>            gST->ConOut->ClearScreen(gST->ConOut);
> -          return;
> +          goto Done;
>          } else {
>            //
>            // Let user input Psid again.
> @@ -1456,6 +1475,11 @@ ProcessOpalRequestPsidRevert (
>      } while (Key.UnicodeChar != CHAR_CARRIAGE_RETURN);
>      gST->ConOut->ClearScreen(gST->ConOut);
>    }
> +
> +Done:
> +  if (PopUpString2 != NULL) {
> +    FreePool (PopUpString2);
> +  }
>  }
> 
>  /**
> @@ -1482,6 +1506,8 @@ ProcessOpalRequestRevert (
>    TCG_RESULT            Ret;
>    BOOLEAN               PasswordFailed;
>    CHAR16                *PopUpString;
> +  CHAR16                *PopUpString2;
> +  UINTN                 BufferSize;
> 
>    if (Dev == NULL) {
>      return;
> @@ -1491,6 +1517,20 @@ ProcessOpalRequestRevert (
> 
>    PopUpString = OpalGetPopUpString (Dev, RequestString);
> 
> +  if (Dev->OpalDisk.EstimateTimeCost > MAX_ACCEPTABLE_REVERTING_TIME)
> {
> +    BufferSize = StrSize (L"Warning: Revert action will take about 
> + ######
> seconds, DO NOT power off system during the revert action!");

Similar comments as above.

Using '######' to keep space for 6 digits maybe not enough. Some characters might get truncated.

> +    PopUpString2 = AllocateZeroPool (BufferSize);
> +    ASSERT (PopUpString2 != NULL);
> +    UnicodeSPrint (
> +        PopUpString2,
> +        BufferSize,
> +        L"WARNING: Revert action will take about %d seconds, DO NOT 
> + power
> off system during the revert action!",
> +        Dev->OpalDisk.EstimateTimeCost
> +      );
> +  } else {
> +    PopUpString2 = NULL;
> +  }
> +
>    Count = 0;
> 
>    ZeroMem(&Session, sizeof(Session)); @@ -1499,7 +1539,7 @@ 
> ProcessOpalRequestRevert (
>    Session.OpalBaseComId = Dev->OpalDisk.OpalBaseComId;
> 
>    while (Count < MAX_PASSWORD_TRY_COUNT) {
> -    Password = OpalDriverPopUpPasswordInput (Dev, PopUpString, NULL,
> &PressEsc);
> +    Password = OpalDriverPopUpPasswordInput (Dev, PopUpString,
> PopUpString2, &PressEsc);
>      if (PressEsc) {
>          do {
>            CreatePopUp (
> @@ -1513,7 +1553,7 @@ ProcessOpalRequestRevert (
> 
>          if (Key.UnicodeChar == CHAR_CARRIAGE_RETURN) {
>            gST->ConOut->ClearScreen(gST->ConOut);
> -          return;
> +          goto Done;
>          } else {
>            //
>            // Let user input password again.
> @@ -1596,6 +1636,11 @@ ProcessOpalRequestRevert (
>      } while (Key.UnicodeChar != CHAR_CARRIAGE_RETURN);
>      gST->ConOut->ClearScreen(gST->ConOut);
>    }
> +
> +Done:
> +  if (PopUpString2 != NULL) {
> +    FreePool (PopUpString2);
> +  }
>  }
> 
>  /**
> @@ -2337,6 +2382,7 @@ ReadyToBootCallback (
>          Session.MediaId = Itr->OpalDisk.MediaId;
>          Session.OpalBaseComId = Itr->OpalDisk.OpalBaseComId;
> 
> +        DEBUG ((DEBUG_INFO, "OpalPassword: ReadyToBoot point, send
> BlockSid command to device!\n"));
>          Result = OpalBlockSid (&Session, TRUE);  // HardwareReset 
> must always be TRUE
>          if (Result != TcgResultSuccess) {
>            DEBUG ((DEBUG_ERROR, "OpalBlockSid fail\n")); diff --git 
> a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h
> b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h
> index 2154523e93..2fe7ada29b 100644
> --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h
> +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalDriver.h
> @@ -75,6 +75,13 @@ extern EFI_COMPONENT_NAME2_PROTOCOL 
> gOpalComponentName2;
>  #define PSID_CHARACTER_LENGTH   0x20
>  #define MAX_PSID_TRY_COUNT      5
> 
> +//
> +// The max timeout value assume the user can wait for the revert action.
> +// If the revert time value bigger than this one, driver needs to 
> +popup a // dialog to let user confirm the revert action.
> +//
> +#define MAX_ACCEPTABLE_REVERTING_TIME    10

It would be better to state that the unit of this macro is second.

> +
>  #pragma pack(1)
> 
>  //
> @@ -140,6 +147,8 @@ typedef struct {
>    TCG_LOCKING_FEATURE_DESCRIPTOR                  LockingFeature;         //
> Locking Feature Descriptor retrieved from performing a Level 0 Discovery
>    UINT8                                           PasswordLength;
>    UINT8                                           Password[OPAL_MAX_PASSWORD_SIZE];
> +
> +  UINT32                                          EstimateTimeCost;
>  } OPAL_DISK;
> 
>  //
> diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
> b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
> index e4972227b6..479f413c07 100644
> --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
> +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalHii.c
> @@ -13,6 +13,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> EITHER EXPRESS OR IMPLIED.
>  **/
> 
>  #include "OpalHii.h"
> +//
> +// Character definitions
> +//
> +#define UPPER_LOWER_CASE_OFFSET 0x20
> 
>  //
>  // This is the generated IFR binary Data for each formset defined in VFR.
> @@ -520,6 +524,59 @@ GetDiskNameStringId(
>    return 0;
>  }
> 
> +/**
> +  Confirm whether user truly want to do the revert action.
> +
> +  @param     OpalDisk            The device which need to do the revert action.
> +
> +  @retval  EFI_SUCCESS           Confirmed user want to do the revert action.
> +**/
> +EFI_STATUS
> +HiiConfirmRevertAction (
> +  IN OPAL_DISK                  *OpalDisk
> +
> +  )
> +{
> +  CHAR16                        Unicode[512];
> +  EFI_INPUT_KEY                 Key;
> +  CHAR16                        ApproveResponse;
> +  CHAR16                        RejectResponse;
> +
> +  //
> +  // When the estimate cost time bigger than
> MAX_ACCEPTABLE_REVERTING_TIME, pop up dialog to let user confirm
> +  // the revert action.
> +  //
> +  if (OpalDisk->EstimateTimeCost < MAX_ACCEPTABLE_REVERTING_TIME) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  ApproveResponse = L'Y';
> +  RejectResponse  = L'N';
> +
> +  UnicodeSPrint(Unicode, StrSize(L"WARNING: Revert device needs about
> ###### seconds"), L"WARNING: Revert device needs about %d seconds",
> OpalDisk->EstimateTimeCost);

Again, using '######' to keep space for 6 digits maybe not enough. Some characters might get truncated.

Best Regards,
Hao Wu

> +
> +  do {
> +    CreatePopUp(
> +        EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> +        &Key,
> +        Unicode,
> +        L" System should not be powered off until revert completion ",
> +        L" ",
> +        L" Press 'Y/y' to continue, press 'N/n' to cancal ",
> +        NULL
> +    );
> +  } while (
> +      ((Key.UnicodeChar | UPPER_LOWER_CASE_OFFSET) != 
> + (ApproveResponse
> | UPPER_LOWER_CASE_OFFSET)) &&
> +      ((Key.UnicodeChar | UPPER_LOWER_CASE_OFFSET) != (RejectResponse 
> + |
> UPPER_LOWER_CASE_OFFSET))
> +    );
> +
> +  if ((Key.UnicodeChar | UPPER_LOWER_CASE_OFFSET) == (RejectResponse 
> + |
> UPPER_LOWER_CASE_OFFSET)) {
> +    return EFI_ABORTED;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    This function processes the results of changes in configuration.
> 
> @@ -588,6 +645,17 @@ DriverCallback(
>      switch (HiiKeyId) {
>        case HII_KEY_ID_GOTO_DISK_INFO:
>          return HiiSelectDisk((UINT8)HiiKey.KeyBits.Index);
> +
> +      case HII_KEY_ID_REVERT:
> +      case HII_KEY_ID_PSID_REVERT:
> +        OpalDisk = HiiGetOpalDiskCB(gHiiConfiguration.SelectedDiskIndex);
> +        if (OpalDisk != NULL) {
> +          return HiiConfirmRevertAction (OpalDisk);
> +        } else {
> +          ASSERT (FALSE);
> +          return EFI_SUCCESS;
> +        }
> +
>      }
>    } else if (Action == EFI_BROWSER_ACTION_CHANGED) {
>      switch (HiiKeyId) {
> @@ -1112,6 +1180,8 @@ OpalDiskInitialize (  {
>    TCG_RESULT                  TcgResult;
>    OPAL_SESSION                Session;
> +  UINT8                       ActiveDataRemovalMechanism;
> +  UINT32                      RemovalMechanishLists[ResearvedMechanism];
> 
>    ZeroMem(&Dev->OpalDisk, sizeof(OPAL_DISK));
>    Dev->OpalDisk.Sscp = Dev->Sscp;
> @@ -1133,6 +1203,20 @@ OpalDiskInitialize (
>      return EFI_DEVICE_ERROR;
>    }
> 
> +  if (Dev->OpalDisk.SupportedAttributes.DataRemoval) {
> +    TcgResult = OpalUtilGetDataRemovalMechanismLists (&Session,
> RemovalMechanishLists);
> +    if (TcgResult != TcgResultSuccess) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    TcgResult = OpalUtilGetActiveDataRemovalMechanism (&Session, Dev-
> >OpalDisk.Msid, Dev->OpalDisk.MsidLength, 
> >&ActiveDataRemovalMechanism);
> +    if (TcgResult != TcgResultSuccess) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    Dev->OpalDisk.EstimateTimeCost =
> RemovalMechanishLists[ActiveDataRemovalMechanism];
> +  }
> +
>    return OpalDiskUpdateStatus (&Dev->OpalDisk);  }
> 
> diff --git a/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.c
> b/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.c
> index b4b2d4b3f0..edb47ca8bc 100644
> --- a/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.c
> +++ b/SecurityPkg/Tcg/Opal/OpalPassword/OpalPasswordPei.c
> @@ -635,6 +635,7 @@ UnlockOpalPassword (
>      BlockSIDEnabled = FALSE;
>    }
>    if (BlockSIDEnabled && BlockSidSupport) {
> +    DEBUG ((DEBUG_INFO, "OpalPassword: S3 phase send BlockSid command
> to device!\n"));
>      ZeroMem(&Session, sizeof (Session));
>      Session.Sscp = &OpalDev->Sscp;
>      Session.MediaId = 0;
> --
> 2.15.0.windows.1