From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 16E0920961060 for ; Sun, 6 May 2018 22:33:28 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 May 2018 22:33:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,372,1520924400"; d="scan'208";a="43702583" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 06 May 2018 22:33:27 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 6 May 2018 22:33:27 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 6 May 2018 22:33:27 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.40]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.240]) with mapi id 14.03.0319.002; Mon, 7 May 2018 13:33:12 +0800 From: "Dong, Eric" To: "Wu, Hao A" , "edk2-devel@lists.01.org" Thread-Topic: [Patch 3/3] SecurityPkg/OpalPassword: Add support for pyrite 2.0 devices. Thread-Index: AQHT5bCo7kIFdejdhE+y+OTMojp+oaQjvkHg Date: Mon, 7 May 2018 05:33:12 +0000 Message-ID: References: <20180503031702.11296-1-eric.dong@intel.com> <20180503031702.11296-4-eric.dong@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 3/3] SecurityPkg/OpalPassword: Add support for pyrite 2.0 devices. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 May 2018 05:33:29 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Hao, Thanks for your comments, I will update it when I check in the code. Thanks, Eric -----Original Message----- From: Wu, Hao A=20 Sent: Monday, May 7, 2018 11:08 AM To: Dong, Eric ; 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=20 > 2.0 devices. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > 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(-) >=20 > 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( > } >=20 > // > - // Psid revert is available for any device with media encryption=20 > support > - // Revert is allowed for any device with media encryption support,=20 > however it requires > + // Psid revert is available for any device with media encryption=20 > + support or > pyrite 2.0 type support. > // > - if (SupportedAttributes->MediaEncryption) { > + if (SupportedAttributes->PyriteSscV2 || SupportedAttributes- > >MediaEncryption) { >=20 > // > // 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 th= e 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 R= evert > 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 E= SC. >=20 > @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; >=20 > if (Dev =3D=3D NULL) { > return; > @@ -1378,6 +1383,20 @@ ProcessOpalRequestPsidRevert ( >=20 > PopUpString =3D OpalGetPopUpString (Dev, RequestString); >=20 > + if (Dev->OpalDisk.EstimateTimeCost > MAX_ACCEPTABLE_REVERTING_TIME) > { > + BufferSize =3D StrSize (L"Warning: Revert action will take about=20 > + ###### > 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 =3D 7864080 seconds So using '######' to keep space for 6 digits maybe not enough. Some charact= ers might get truncated. > + PopUpString2 =3D AllocateZeroPool (BufferSize); > + ASSERT (PopUpString2 !=3D NULL); > + UnicodeSPrint ( > + PopUpString2, > + BufferSize, > + L"WARNING: Revert action will take about %d seconds, DO NOT=20 > + power > off system during the revert action!", > + Dev->OpalDisk.EstimateTimeCost > + ); > + } else { > + PopUpString2 =3D NULL; > + } > + > Count =3D 0; >=20 > ZeroMem(&Session, sizeof(Session)); @@ -1386,7 +1405,7 @@=20 > ProcessOpalRequestPsidRevert ( > Session.OpalBaseComId =3D Dev->OpalDisk.OpalBaseComId; >=20 > while (Count < MAX_PSID_TRY_COUNT) { > - Psid =3D OpalDriverPopUpPsidInput (Dev, PopUpString, &PressEsc); > + Psid =3D OpalDriverPopUpPsidInput (Dev, PopUpString, PopUpString2, > &PressEsc); > if (PressEsc) { > do { > CreatePopUp ( > @@ -1400,7 +1419,7 @@ ProcessOpalRequestPsidRevert ( >=20 > if (Key.UnicodeChar =3D=3D 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 !=3D CHAR_CARRIAGE_RETURN); > gST->ConOut->ClearScreen(gST->ConOut); > } > + > +Done: > + if (PopUpString2 !=3D NULL) { > + FreePool (PopUpString2); > + } > } >=20 > /** > @@ -1482,6 +1506,8 @@ ProcessOpalRequestRevert ( > TCG_RESULT Ret; > BOOLEAN PasswordFailed; > CHAR16 *PopUpString; > + CHAR16 *PopUpString2; > + UINTN BufferSize; >=20 > if (Dev =3D=3D NULL) { > return; > @@ -1491,6 +1517,20 @@ ProcessOpalRequestRevert ( >=20 > PopUpString =3D OpalGetPopUpString (Dev, RequestString); >=20 > + if (Dev->OpalDisk.EstimateTimeCost > MAX_ACCEPTABLE_REVERTING_TIME) > { > + BufferSize =3D StrSize (L"Warning: Revert action will take about=20 > + ###### > 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 =3D AllocateZeroPool (BufferSize); > + ASSERT (PopUpString2 !=3D NULL); > + UnicodeSPrint ( > + PopUpString2, > + BufferSize, > + L"WARNING: Revert action will take about %d seconds, DO NOT=20 > + power > off system during the revert action!", > + Dev->OpalDisk.EstimateTimeCost > + ); > + } else { > + PopUpString2 =3D NULL; > + } > + > Count =3D 0; >=20 > ZeroMem(&Session, sizeof(Session)); @@ -1499,7 +1539,7 @@=20 > ProcessOpalRequestRevert ( > Session.OpalBaseComId =3D Dev->OpalDisk.OpalBaseComId; >=20 > while (Count < MAX_PASSWORD_TRY_COUNT) { > - Password =3D OpalDriverPopUpPasswordInput (Dev, PopUpString, NULL, > &PressEsc); > + Password =3D OpalDriverPopUpPasswordInput (Dev, PopUpString, > PopUpString2, &PressEsc); > if (PressEsc) { > do { > CreatePopUp ( > @@ -1513,7 +1553,7 @@ ProcessOpalRequestRevert ( >=20 > if (Key.UnicodeChar =3D=3D 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 !=3D CHAR_CARRIAGE_RETURN); > gST->ConOut->ClearScreen(gST->ConOut); > } > + > +Done: > + if (PopUpString2 !=3D NULL) { > + FreePool (PopUpString2); > + } > } >=20 > /** > @@ -2337,6 +2382,7 @@ ReadyToBootCallback ( > Session.MediaId =3D Itr->OpalDisk.MediaId; > Session.OpalBaseComId =3D Itr->OpalDisk.OpalBaseComId; >=20 > + DEBUG ((DEBUG_INFO, "OpalPassword: ReadyToBoot point, send > BlockSid command to device!\n")); > Result =3D OpalBlockSid (&Session, TRUE); // HardwareReset=20 > must always be TRUE > if (Result !=3D TcgResultSuccess) { > DEBUG ((DEBUG_ERROR, "OpalBlockSid fail\n")); diff --git=20 > 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=20 > gOpalComponentName2; > #define PSID_CHARACTER_LENGTH 0x20 > #define MAX_PSID_TRY_COUNT 5 >=20 > +// > +// 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=20 > +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) >=20 > // > @@ -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_PASS= WORD_SIZE]; > + > + UINT32 EstimateTimeCost; > } OPAL_DISK; >=20 > // > 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,=20 > EITHER EXPRESS OR IMPLIED. > **/ >=20 > #include "OpalHii.h" > +// > +// Character definitions > +// > +#define UPPER_LOWER_CASE_OFFSET 0x20 >=20 > // > // This is the generated IFR binary Data for each formset defined in VFR= . > @@ -520,6 +524,59 @@ GetDiskNameStringId( > return 0; > } >=20 > +/** > + 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 ac= tion. > +**/ > +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 =3D L'Y'; > + RejectResponse =3D 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 cha= racters 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) !=3D=20 > + (ApproveResponse > | UPPER_LOWER_CASE_OFFSET)) && > + ((Key.UnicodeChar | UPPER_LOWER_CASE_OFFSET) !=3D (RejectResponse= =20 > + | > UPPER_LOWER_CASE_OFFSET)) > + ); > + > + if ((Key.UnicodeChar | UPPER_LOWER_CASE_OFFSET) =3D=3D (RejectResponse= =20 > + | > UPPER_LOWER_CASE_OFFSET)) { > + return EFI_ABORTED; > + } > + > + return EFI_SUCCESS; > +} > + > /** > This function processes the results of changes in configuration. >=20 > @@ -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 =3D HiiGetOpalDiskCB(gHiiConfiguration.SelectedDiskInde= x); > + if (OpalDisk !=3D NULL) { > + return HiiConfirmRevertAction (OpalDisk); > + } else { > + ASSERT (FALSE); > + return EFI_SUCCESS; > + } > + > } > } else if (Action =3D=3D EFI_BROWSER_ACTION_CHANGED) { > switch (HiiKeyId) { > @@ -1112,6 +1180,8 @@ OpalDiskInitialize ( { > TCG_RESULT TcgResult; > OPAL_SESSION Session; > + UINT8 ActiveDataRemovalMechanism; > + UINT32 RemovalMechanishLists[ResearvedMechanism]; >=20 > ZeroMem(&Dev->OpalDisk, sizeof(OPAL_DISK)); > Dev->OpalDisk.Sscp =3D Dev->Sscp; > @@ -1133,6 +1203,20 @@ OpalDiskInitialize ( > return EFI_DEVICE_ERROR; > } >=20 > + if (Dev->OpalDisk.SupportedAttributes.DataRemoval) { > + TcgResult =3D OpalUtilGetDataRemovalMechanismLists (&Session, > RemovalMechanishLists); > + if (TcgResult !=3D TcgResultSuccess) { > + return EFI_DEVICE_ERROR; > + } > + > + TcgResult =3D OpalUtilGetActiveDataRemovalMechanism (&Session, Dev- > >OpalDisk.Msid, Dev->OpalDisk.MsidLength,=20 > >&ActiveDataRemovalMechanism); > + if (TcgResult !=3D TcgResultSuccess) { > + return EFI_DEVICE_ERROR; > + } > + > + Dev->OpalDisk.EstimateTimeCost =3D > RemovalMechanishLists[ActiveDataRemovalMechanism]; > + } > + > return OpalDiskUpdateStatus (&Dev->OpalDisk); } >=20 > 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 =3D FALSE; > } > if (BlockSIDEnabled && BlockSidSupport) { > + DEBUG ((DEBUG_INFO, "OpalPassword: S3 phase send BlockSid command > to device!\n")); > ZeroMem(&Session, sizeof (Session)); > Session.Sscp =3D &OpalDev->Sscp; > Session.MediaId =3D 0; > -- > 2.15.0.windows.1