Re: [edk2-devel] [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec
Thanks.https://github.com/tianocore/edk2/pull/5533 > -Original Message- > From: Bi, Dandan > Sent: Sunday, April 7, 2024 10:07 AM > To: Tan, Ming ; devel@edk2.groups.io > Cc: Xu, Min M ; Yao, Jiewen ; > POLUDOV, FELIX > Subject: RE: [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according > to UEFI spec > > Reviewed-by: Dandan Bi > > -Original Message- > From: Tan, Ming > Sent: Tuesday, April 2, 2024 4:32 PM > To: devel@edk2.groups.io > Cc: Xu, Min M ; Yao, Jiewen ; Bi, > Dandan ; POLUDOV, FELIX > Subject: [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to > UEFI spec > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4713 > > In UEFI_Spec_2_10_Aug29.pdf page 1694 section 35.5.4 for > EFI_BROWSER_ACTION_FORM_OPEN: > NOTE: EFI_FORM_BROWSER2_PROTOCOL.BrowserCallback() cannot be used > with this browser action because question values have not been retrieved yet. > > So should not call HiiGetBrowserData() and HiiSetBrowserData() in FORM_OPEN > call back function. > > Now call SecureBootExtractConfigFromVariable() and update > IfrNvData->ListCount to save the change to EFI variable, then HII use > IfrNvData->EFI > variable to control the UI. > > Cc: Min Xu > Cc: Jiewen Yao > Cc: Dandan Bi > Cc: Felix Polyudov > Signed-off-by: Ming Tan > --- > PR: https://github.com/tianocore/edk2/pull/5411 > > V4: Fix a Cc issue of miss a space. > V3: According to Dandan Bi's feedback, does not call > SecureBootExtractConfigFromVariable() at last, but call it as needed. > And add more code for update IfrNvData->ListCount. > V2: Change code style to pass uncrustify check. > > .../SecureBootConfigImpl.c| 42 +++ > 1 file changed, 25 insertions(+), 17 deletions(-) > > diff --git > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm > pl.c > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm > pl.c > index 2c11129526..6d4560c39b 100644 > --- > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm > pl.c > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo > +++ nfigImpl.c > @@ -3366,6 +3366,8 @@ SecureBootExtractConfigFromVariable ( > ConfigData->FileEnrollType = UNKNOWN_FILE_TYPE; } + ConfigData- > >ListCount = Private->ListCount;+ // // If it is Physical Presence User, > >set the > PhysicalPresent to true. //@@ -4541,12 +4543,13 @@ SecureBootCallback ( >EFI_HII_POPUP_PROTOCOL *HiiPopup; EFI_HII_POPUP_SELECTION > UserSelection; - Status = EFI_SUCCESS;- SecureBootEnable = > NULL;- > SecureBootMode = NULL;- SetupMode = NULL;- File > = NULL;- > EnrollKeyErrorCode = None_Error;+ Status = EFI_SUCCESS;+ > SecureBootEnable = NULL;+ SecureBootMode = NULL;+ SetupMode > = NULL;+ File = NULL;+ EnrollKeyErrorCode = None_Error;+ > GetBrowserDataResult = FALSE;if ((This == NULL) || (Value == NULL) || > (ActionRequest == NULL)) { return EFI_INVALID_PARAMETER;@@ -4565,15 > +4568,12 @@ SecureBootCallback ( > return EFI_OUT_OF_RESOURCES; } - GetBrowserDataResult = > HiiGetBrowserData (, > mSecureBootStorageName, BufferSize, (UINT8 *)IfrNvData);- if (Action == > EFI_BROWSER_ACTION_FORM_OPEN) { if (QuestionId == > KEY_SECURE_BOOT_MODE) { // // Update secure boot strings when > opening this form //- Status = UpdateSecureBootString (Private);- > SecureBootExtractConfigFromVariable (Private, IfrNvData);+ Status > = > UpdateSecureBootString (Private); mIsEnterSecureBootForm = TRUE; } > else > { //@@ -4587,23 +4587,22 @@ SecureBootCallback ( >(QuestionId == KEY_SECURE_BOOT_DBT_OPTION)) > { CloseEnrolledFile (Private->FileContext);- } else if > (QuestionId == > KEY_SECURE_BOOT_DELETE_ALL_LIST) {-//-// Update ListCount > field in > varstore-// Button "Delete All Signature List" is-// enable > when ListCount > is greater than 0.-//-IfrNvData->ListCount = > Private->ListCount; } } > goto EXIT; } + GetBrowserDataResult = HiiGetBrowserData > (, mSecureBootStorageName, BufferSize, > (UINT8 *)IfrNvData);+ if (Action == EFI_BROWSER_ACTION_RETRIEVE) > { Status = EFI_UNSUPPORTED; if (QuestionId == KEY_SECURE_BOOT_MODE) > { if (mIsEnterSecureBootForm) {+if (GetBrowserDataResult) {+ > SecureBootExtractConfigFromVariable (Private, IfrNvData);+}+ > Value- > >u8 = SECURE_BOOT_MODE_STANDARD; Status= EFI_SUCCESS; }@@ - > 4764,6 +4763,8 @@ SecureBootCallback ( > L"Only Physical Presence User could delete PK in custom > mode!", > NULL );+} else {+ > SecureBootExtractConfigFromVariable > (Private, IfrNvData); } } }@@ -4827,6 +4828,7
Re: [edk2-devel] [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec
Reviewed-by: Dandan Bi -Original Message- From: Tan, Ming Sent: Tuesday, April 2, 2024 4:32 PM To: devel@edk2.groups.io Cc: Xu, Min M ; Yao, Jiewen ; Bi, Dandan ; POLUDOV, FELIX Subject: [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4713 In UEFI_Spec_2_10_Aug29.pdf page 1694 section 35.5.4 for EFI_BROWSER_ACTION_FORM_OPEN: NOTE: EFI_FORM_BROWSER2_PROTOCOL.BrowserCallback() cannot be used with this browser action because question values have not been retrieved yet. So should not call HiiGetBrowserData() and HiiSetBrowserData() in FORM_OPEN call back function. Now call SecureBootExtractConfigFromVariable() and update IfrNvData->ListCount to save the change to EFI variable, then HII use IfrNvData->EFI variable to control the UI. Cc: Min Xu Cc: Jiewen Yao Cc: Dandan Bi Cc: Felix Polyudov Signed-off-by: Ming Tan --- PR: https://github.com/tianocore/edk2/pull/5411 V4: Fix a Cc issue of miss a space. V3: According to Dandan Bi's feedback, does not call SecureBootExtractConfigFromVariable() at last, but call it as needed. And add more code for update IfrNvData->ListCount. V2: Change code style to pass uncrustify check. .../SecureBootConfigImpl.c| 42 +++ 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c index 2c11129526..6d4560c39b 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo +++ nfigImpl.c @@ -3366,6 +3366,8 @@ SecureBootExtractConfigFromVariable ( ConfigData->FileEnrollType = UNKNOWN_FILE_TYPE; } + ConfigData->ListCount = Private->ListCount;+ // // If it is Physical Presence User, set the PhysicalPresent to true. //@@ -4541,12 +4543,13 @@ SecureBootCallback ( EFI_HII_POPUP_PROTOCOL *HiiPopup; EFI_HII_POPUP_SELECTION UserSelection; - Status = EFI_SUCCESS;- SecureBootEnable = NULL;- SecureBootMode = NULL;- SetupMode = NULL;- File = NULL;- EnrollKeyErrorCode = None_Error;+ Status = EFI_SUCCESS;+ SecureBootEnable = NULL;+ SecureBootMode = NULL;+ SetupMode= NULL;+ File = NULL;+ EnrollKeyErrorCode = None_Error;+ GetBrowserDataResult = FALSE;if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) { return EFI_INVALID_PARAMETER;@@ -4565,15 +4568,12 @@ SecureBootCallback ( return EFI_OUT_OF_RESOURCES; } - GetBrowserDataResult = HiiGetBrowserData (, mSecureBootStorageName, BufferSize, (UINT8 *)IfrNvData);- if (Action == EFI_BROWSER_ACTION_FORM_OPEN) { if (QuestionId == KEY_SECURE_BOOT_MODE) { // // Update secure boot strings when opening this form //- Status = UpdateSecureBootString (Private);- SecureBootExtractConfigFromVariable (Private, IfrNvData);+ Status = UpdateSecureBootString (Private); mIsEnterSecureBootForm = TRUE; } else { //@@ -4587,23 +4587,22 @@ SecureBootCallback ( (QuestionId == KEY_SECURE_BOOT_DBT_OPTION)) { CloseEnrolledFile (Private->FileContext);- } else if (QuestionId == KEY_SECURE_BOOT_DELETE_ALL_LIST) {-//-// Update ListCount field in varstore-// Button "Delete All Signature List" is-// enable when ListCount is greater than 0.-//-IfrNvData->ListCount = Private->ListCount; } } goto EXIT; } + GetBrowserDataResult = HiiGetBrowserData (, mSecureBootStorageName, BufferSize, (UINT8 *)IfrNvData);+ if (Action == EFI_BROWSER_ACTION_RETRIEVE) { Status = EFI_UNSUPPORTED; if (QuestionId == KEY_SECURE_BOOT_MODE) { if (mIsEnterSecureBootForm) {+if (GetBrowserDataResult) {+ SecureBootExtractConfigFromVariable (Private, IfrNvData);+}+ Value->u8 = SECURE_BOOT_MODE_STANDARD; Status= EFI_SUCCESS; }@@ -4764,6 +4763,8 @@ SecureBootCallback ( L"Only Physical Presence User could delete PK in custom mode!", NULL );+} else {+ SecureBootExtractConfigFromVariable (Private, IfrNvData); } } }@@ -4827,6 +4828,7 @@ SecureBootCallback ( SECUREBOOT_DELETE_SIGNATURE_LIST_FORM, OPTION_SIGNATURE_LIST_QUESTION_ID );+IfrNvData->ListCount = Private->ListCount; break;//@@ -4851,6 +4853,7 @@ SecureBootCallback ( SECUREBOOT_DELETE_SIGNATURE_LIST_FORM, OPTION_SIGNATURE_LIST_QUESTION_ID );+IfrNvData->ListCount = Private->ListCount; break;//@@ -4875,6
Re: [edk2-devel] [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec
Reviewed-by: Felix Polyudov -Original Message- From: Ming Tan Sent: Tuesday, April 2, 2024 4:32 AM To: devel@edk2.groups.io Cc: Min Xu ; Jiewen Yao ; Dandan Bi ; Felix Polyudov Subject: [EXTERNAL] [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec **CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.** REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4713 In UEFI_Spec_2_10_Aug29.pdf page 1694 section 35.5.4 for EFI_BROWSER_ACTION_FORM_OPEN: NOTE: EFI_FORM_BROWSER2_PROTOCOL.BrowserCallback() cannot be used with this browser action because question values have not been retrieved yet. So should not call HiiGetBrowserData() and HiiSetBrowserData() in FORM_OPEN call back function. Now call SecureBootExtractConfigFromVariable() and update IfrNvData->ListCount to save the change to EFI variable, then HII use IfrNvData->EFI variable to control the UI. Cc: Min Xu Cc: Jiewen Yao Cc: Dandan Bi Cc: Felix Polyudov Signed-off-by: Ming Tan --- PR: https://github.com/tianocore/edk2/pull/5411 V4: Fix a Cc issue of miss a space. V3: According to Dandan Bi's feedback, does not call SecureBootExtractConfigFromVariable() at last, but call it as needed. And add more code for update IfrNvData->ListCount. V2: Change code style to pass uncrustify check. .../SecureBootConfigImpl.c| 42 +++ 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c index 2c11129526..6d4560c39b 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo +++ nfigImpl.c @@ -3366,6 +3366,8 @@ SecureBootExtractConfigFromVariable ( ConfigData->FileEnrollType = UNKNOWN_FILE_TYPE; } + ConfigData->ListCount = Private->ListCount; + // // If it is Physical Presence User, set the PhysicalPresent to true. // @@ -4541,12 +4543,13 @@ SecureBootCallback ( EFI_HII_POPUP_PROTOCOL *HiiPopup; EFI_HII_POPUP_SELECTION UserSelection; - Status = EFI_SUCCESS; - SecureBootEnable = NULL; - SecureBootMode = NULL; - SetupMode = NULL; - File = NULL; - EnrollKeyErrorCode = None_Error; + Status = EFI_SUCCESS; + SecureBootEnable = NULL; + SecureBootMode = NULL; + SetupMode= NULL; + File = NULL; + EnrollKeyErrorCode = None_Error; + GetBrowserDataResult = FALSE; if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) { return EFI_INVALID_PARAMETER; @@ -4565,15 +4568,12 @@ SecureBootCallback ( return EFI_OUT_OF_RESOURCES; } - GetBrowserDataResult = HiiGetBrowserData (, mSecureBootStorageName, BufferSize, (UINT8 *)IfrNvData); - if (Action == EFI_BROWSER_ACTION_FORM_OPEN) { if (QuestionId == KEY_SECURE_BOOT_MODE) { // // Update secure boot strings when opening this form // - Status = UpdateSecureBootString (Private); - SecureBootExtractConfigFromVariable (Private, IfrNvData); + Status = UpdateSecureBootString (Private); mIsEnterSecureBootForm = TRUE; } else { // @@ -4587,23 +4587,22 @@ SecureBootCallback ( (QuestionId == KEY_SECURE_BOOT_DBT_OPTION)) { CloseEnrolledFile (Private->FileContext); - } else if (QuestionId == KEY_SECURE_BOOT_DELETE_ALL_LIST) { -// -// Update ListCount field in varstore -// Button "Delete All Signature List" is -// enable when ListCount is greater than 0. -// -IfrNvData->ListCount = Private->ListCount; } } goto EXIT; } + GetBrowserDataResult = HiiGetBrowserData + (, mSecureBootStorageName, BufferSize, + (UINT8 *)IfrNvData); + if (Action == EFI_BROWSER_ACTION_RETRIEVE) { Status = EFI_UNSUPPORTED; if (QuestionId == KEY_SECURE_BOOT_MODE) { if (mIsEnterSecureBootForm) { +if (GetBrowserDataResult) { + SecureBootExtractConfigFromVariable (Private, IfrNvData); +} + Value->u8 = SECURE_BOOT_MODE_STANDARD; Status= EFI_SUCCESS; } @@ -4764,6 +4763,8 @@ SecureBootCallback ( L"Only Physical Presence User could delete PK in custom mode!", NULL ); +} else { + SecureBootExtractConfigFromVariable (Private, IfrNvData); } } } @@ -4827,6 +4828,7 @@ SecureBootCallback ( SECUREBOOT_DELETE_SIGNATURE_LIST_FORM, OPTION_SIGNATURE_LIST_QUESTION_ID ); +
[edk2-devel] [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4713 In UEFI_Spec_2_10_Aug29.pdf page 1694 section 35.5.4 for EFI_BROWSER_ACTION_FORM_OPEN: NOTE: EFI_FORM_BROWSER2_PROTOCOL.BrowserCallback() cannot be used with this browser action because question values have not been retrieved yet. So should not call HiiGetBrowserData() and HiiSetBrowserData() in FORM_OPEN call back function. Now call SecureBootExtractConfigFromVariable() and update IfrNvData->ListCount to save the change to EFI variable, then HII use EFI variable to control the UI. Cc: Min Xu Cc: Jiewen Yao Cc: Dandan Bi Cc: Felix Polyudov Signed-off-by: Ming Tan --- PR: https://github.com/tianocore/edk2/pull/5411 V4: Fix a Cc issue of miss a space. V3: According to Dandan Bi's feedback, does not call SecureBootExtractConfigFromVariable() at last, but call it as needed. And add more code for update IfrNvData->ListCount. V2: Change code style to pass uncrustify check. .../SecureBootConfigImpl.c| 42 +++ 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c index 2c11129526..6d4560c39b 100644 --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c @@ -3366,6 +3366,8 @@ SecureBootExtractConfigFromVariable ( ConfigData->FileEnrollType = UNKNOWN_FILE_TYPE; } + ConfigData->ListCount = Private->ListCount; + // // If it is Physical Presence User, set the PhysicalPresent to true. // @@ -4541,12 +4543,13 @@ SecureBootCallback ( EFI_HII_POPUP_PROTOCOL *HiiPopup; EFI_HII_POPUP_SELECTION UserSelection; - Status = EFI_SUCCESS; - SecureBootEnable = NULL; - SecureBootMode = NULL; - SetupMode = NULL; - File = NULL; - EnrollKeyErrorCode = None_Error; + Status = EFI_SUCCESS; + SecureBootEnable = NULL; + SecureBootMode = NULL; + SetupMode= NULL; + File = NULL; + EnrollKeyErrorCode = None_Error; + GetBrowserDataResult = FALSE; if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) { return EFI_INVALID_PARAMETER; @@ -4565,15 +4568,12 @@ SecureBootCallback ( return EFI_OUT_OF_RESOURCES; } - GetBrowserDataResult = HiiGetBrowserData (, mSecureBootStorageName, BufferSize, (UINT8 *)IfrNvData); - if (Action == EFI_BROWSER_ACTION_FORM_OPEN) { if (QuestionId == KEY_SECURE_BOOT_MODE) { // // Update secure boot strings when opening this form // - Status = UpdateSecureBootString (Private); - SecureBootExtractConfigFromVariable (Private, IfrNvData); + Status = UpdateSecureBootString (Private); mIsEnterSecureBootForm = TRUE; } else { // @@ -4587,23 +4587,22 @@ SecureBootCallback ( (QuestionId == KEY_SECURE_BOOT_DBT_OPTION)) { CloseEnrolledFile (Private->FileContext); - } else if (QuestionId == KEY_SECURE_BOOT_DELETE_ALL_LIST) { -// -// Update ListCount field in varstore -// Button "Delete All Signature List" is -// enable when ListCount is greater than 0. -// -IfrNvData->ListCount = Private->ListCount; } } goto EXIT; } + GetBrowserDataResult = HiiGetBrowserData (, mSecureBootStorageName, BufferSize, (UINT8 *)IfrNvData); + if (Action == EFI_BROWSER_ACTION_RETRIEVE) { Status = EFI_UNSUPPORTED; if (QuestionId == KEY_SECURE_BOOT_MODE) { if (mIsEnterSecureBootForm) { +if (GetBrowserDataResult) { + SecureBootExtractConfigFromVariable (Private, IfrNvData); +} + Value->u8 = SECURE_BOOT_MODE_STANDARD; Status= EFI_SUCCESS; } @@ -4764,6 +4763,8 @@ SecureBootCallback ( L"Only Physical Presence User could delete PK in custom mode!", NULL ); +} else { + SecureBootExtractConfigFromVariable (Private, IfrNvData); } } } @@ -4827,6 +4828,7 @@ SecureBootCallback ( SECUREBOOT_DELETE_SIGNATURE_LIST_FORM, OPTION_SIGNATURE_LIST_QUESTION_ID ); +IfrNvData->ListCount = Private->ListCount; break; // @@ -4851,6 +4853,7 @@ SecureBootCallback ( SECUREBOOT_DELETE_SIGNATURE_LIST_FORM, OPTION_SIGNATURE_LIST_QUESTION_ID ); +IfrNvData->ListCount = Private->ListCount; break; // @@ -4875,6 +4878,7 @@ SecureBootCallback ( SECUREBOOT_DELETE_SIGNATURE_LIST_FORM, OPTION_SIGNATURE_LIST_QUESTION_ID ); +IfrNvData->ListCount = Private->ListCount; break; case