Re: [edk2-devel] [PATCH v4] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec

2024-04-06 Thread Yao, Jiewen
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

2024-04-06 Thread Dandan Bi
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

2024-04-04 Thread Felix Polyudov via groups.io
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

2024-04-02 Thread Tan, Ming
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