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

["Wu, Hao A" <hao.a.wu@intel.com>]

Thanks and with the changes:
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: Dong, Eric
> Sent: Monday, May 07, 2018 1:33 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Subject: RE: [Patch 3/3] SecurityPkg/OpalPassword: Add support for pyrite 2.0
> devices.
> 
> 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