Thanks and with the changes: Reviewed-by: Hao Wu <[email protected]>
Best Regards, Hao Wu > -----Original Message----- > From: Dong, Eric > Sent: Monday, May 07, 2018 1:33 PM > To: Wu, Hao A; [email protected] > 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 <[email protected]>; [email protected] > 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: [email protected]; 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 <[email protected]> > > --- > > 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
