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

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

[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() to save the change to EFI
variable, then HII use EFI variable to control the UI.

Cc: Min Xu <min.m.xu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Ming Tan <ming.tan@intel.com>
---
  V2: Change code style to pass uncrustify check.

 .../SecureBootConfigImpl.c                    | 37 ++++++++++---------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index 2c11129526..e2e61d1e07 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 (&gSecureBootConfigFormSetGuid, 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 (&gSecureBootConfigFormSetGuid, 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;
       }
@@ -5179,6 +5178,10 @@ SecureBootCallback (
     }
   }
 
+  if (GetBrowserDataResult) {
+    SecureBootExtractConfigFromVariable (Private, IfrNvData);
+  }
+
 EXIT:
 
   if (!EFI_ERROR (Status) && GetBrowserDataResult) {
-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115998): https://edk2.groups.io/g/devel/message/115998
Mute This Topic: https://groups.io/mt/104596915/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Yao, Jiewen]

Thanks for the update.

First, would you please clarify which test you have done for this patch set.
Have you tested all previous function to ensure it still works?

Second, would you please clarify if there is any compatibility issue to follow the new UEFI 2.10?
For example, what if the core HII is still UEFI 2.9? would that still work?

Third, because I am not HII expert, I would like to have HII expert to comment the HII/Browser related change.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Tan, Ming <ming.tan@intel.com>
> Sent: Tuesday, February 27, 2024 10:59 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v2] 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() to save the change to EFI
> variable, then HII use EFI variable to control the UI.
> 
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Ming Tan <ming.tan@intel.com>
> ---
>   V2: Change code style to pass uncrustify check.
> 
>  .../SecureBootConfigImpl.c                    | 37 ++++++++++---------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> pl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> pl.c
> index 2c11129526..e2e61d1e07 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> pl.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> pl.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
> (&gSecureBootConfigFormSetGuid, 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
> (&gSecureBootConfigFormSetGuid, 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;
> 
>        }
> 
> @@ -5179,6 +5178,10 @@ SecureBootCallback (
>      }
> 
>    }
> 
> 
> 
> +  if (GetBrowserDataResult) {
> 
> +    SecureBootExtractConfigFromVariable (Private, IfrNvData);
> 
> +  }
> 
> +
> 
>  EXIT:
> 
> 
> 
>    if (!EFI_ERROR (Status) && GetBrowserDataResult) {
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116090): https://edk2.groups.io/g/devel/message/116090
Mute This Topic: https://groups.io/mt/104596915/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Tan, Ming]

Jiewen:
  This patch is only for UEFI spec mantis 1908 change in SecureBootConfigDxe.
  This spec change just ask some drivers do some modification, it does not ask the HII core to be modified, so this spec change will not cause compatibility issue.

  For this patch, it only touch the UI setting, did not touch the modification of pk, kek, db, dbx, dbt EFI variable.
  I did the following unit test in EmulatorPkg WinHost.exe and Intel AlderLake RVP:
  1. In PK Options, Enroll PK, check the "Attempt Secure Boot" is not gray and enabled.
  2. In RVP, Set "Attempt Secure Boot" to enable and disable, check it does work, and device can boot to Windows.
  3. In PK Options, delete PK, check the "Attempt Secure Boot" is gray and disabled.
  4. In DBX Options, Enroll Signature, check it does work, and the "Delete All Signature List" is not gray.
  5. In DBX Options, Delete all signature, check it does work, and the "Delete All Signature List" is gray after all signatures are deleted.
  6. In KEK Options, Enroll KEK, check it does work.
  7. In KEK Options, Delete KEK, check it does work.
  8. In DB Options, Enroll Signature, check it does work.
  9. In DB Options, Delete Signature, check it does work.
  10. In DBT Options, Enroll Signature, check it does work.
  11. In DBT Options, Delete Signature, check it does work.

  @Pingle, Sneha S Would you like to help to verify the secure boot functions again in another Intel RVP?
  @Felix Polyudov Would you like to help to verify this patch with AMI browser? Since AMI submitted this UEFI spec change, if verify pass, would you like to add a "Reviewed-by' for this patch?
  @Bi, Dandan is HII expert and the reviewer of edk2 HII and UI modules, would you like to review this patch? And add a "Reviewed-by" for this patch?


  And I create a PR for this patch: https://github.com/tianocore/edk2/pull/5411, all CI checking are passed.

  Thank you.
  Tan Ming.


-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com> 
Sent: Wednesday, February 28, 2024 1:56 PM
To: Tan, Ming <ming.tan@intel.com>; devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>
Subject: RE: [PATCH v2] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec

Thanks for the update.

First, would you please clarify which test you have done for this patch set.
Have you tested all previous function to ensure it still works?

Second, would you please clarify if there is any compatibility issue to follow the new UEFI 2.10?
For example, what if the core HII is still UEFI 2.9? would that still work?

Third, because I am not HII expert, I would like to have HII expert to comment the HII/Browser related change.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Tan, Ming <ming.tan@intel.com>
> Sent: Tuesday, February 27, 2024 10:59 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v2] 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() to save the change to 
> EFI variable, then HII use EFI variable to control the UI.
> 
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Ming Tan <ming.tan@intel.com>
> ---
>   V2: Change code style to pass uncrustify check.
> 
>  .../SecureBootConfigImpl.c                    | 37 ++++++++++---------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.c
> index 2c11129526..e2e61d1e07 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.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 
> (&gSecureBootConfigFormSetGuid, 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
> (&gSecureBootConfigFormSetGuid, 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;
> 
>        }
> 
> @@ -5179,6 +5178,10 @@ SecureBootCallback (
>      }
> 
>    }
> 
> 
> 
> +  if (GetBrowserDataResult) {
> 
> +    SecureBootExtractConfigFromVariable (Private, IfrNvData);
> 
> +  }
> 
> +
> 
>  EXIT:
> 
> 
> 
>    if (!EFI_ERROR (Status) && GetBrowserDataResult) {
> 
> --
> 2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116132): https://edk2.groups.io/g/devel/message/116132
Mute This Topic: https://groups.io/mt/104596915/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Felix Polyudov via groups.io]

Reviewed-by: Felix Polyudov <felixp@...>

-----Original Message-----
From: Tan, Ming <ming.tan@intel.com>
Sent: Wednesday, February 28, 2024 10:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Pingle, Sneha S <sneha.s.pingle@intel.com>; Felix Polyudov <Felixp@ami.com>; Bi, Dandan <dandan.bi@intel.com>
Cc: Xu, Min M <min.m.xu@intel.com>
Subject: [EXTERNAL] RE: [PATCH v2] 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.**

Jiewen:
  This patch is only for UEFI spec mantis 1908 change in SecureBootConfigDxe.
  This spec change just ask some drivers do some modification, it does not ask the HII core to be modified, so this spec change will not cause compatibility issue.

  For this patch, it only touch the UI setting, did not touch the modification of pk, kek, db, dbx, dbt EFI variable.
  I did the following unit test in EmulatorPkg WinHost.exe and Intel AlderLake RVP:
  1. In PK Options, Enroll PK, check the "Attempt Secure Boot" is not gray and enabled.
  2. In RVP, Set "Attempt Secure Boot" to enable and disable, check it does work, and device can boot to Windows.
  3. In PK Options, delete PK, check the "Attempt Secure Boot" is gray and disabled.
  4. In DBX Options, Enroll Signature, check it does work, and the "Delete All Signature List" is not gray.
  5. In DBX Options, Delete all signature, check it does work, and the "Delete All Signature List" is gray after all signatures are deleted.
  6. In KEK Options, Enroll KEK, check it does work.
  7. In KEK Options, Delete KEK, check it does work.
  8. In DB Options, Enroll Signature, check it does work.
  9. In DB Options, Delete Signature, check it does work.
  10. In DBT Options, Enroll Signature, check it does work.
  11. In DBT Options, Delete Signature, check it does work.

  @Pingle, Sneha S Would you like to help to verify the secure boot functions again in another Intel RVP?
  @Felix Polyudov Would you like to help to verify this patch with AMI browser? Since AMI submitted this UEFI spec change, if verify pass, would you like to add a "Reviewed-by' for this patch?
  @Bi, Dandan is HII expert and the reviewer of edk2 HII and UI modules, would you like to review this patch? And add a "Reviewed-by" for this patch?


  And I create a PR for this patch: https://github.com/tianocore/edk2/pull/5411, all CI checking are passed.

  Thank you.
  Tan Ming.


-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Wednesday, February 28, 2024 1:56 PM
To: Tan, Ming <ming.tan@intel.com>; devel@edk2.groups.io
Cc: Xu, Min M <min.m.xu@intel.com>
Subject: RE: [PATCH v2] SecurityPkg/SecureBootConfigDxe: Update UI according to UEFI spec

Thanks for the update.

First, would you please clarify which test you have done for this patch set.
Have you tested all previous function to ensure it still works?

Second, would you please clarify if there is any compatibility issue to follow the new UEFI 2.10?
For example, what if the core HII is still UEFI 2.9? would that still work?

Third, because I am not HII expert, I would like to have HII expert to comment the HII/Browser related change.

Thank you
Yao, Jiewen

> -----Original Message-----
> From: Tan, Ming <ming.tan@intel.com>
> Sent: Tuesday, February 27, 2024 10:59 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v2] 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() to save the change to
> EFI variable, then HII use EFI variable to control the UI.
>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Ming Tan <ming.tan@intel.com>
> ---
>   V2: Change code style to pass uncrustify check.
>
>  .../SecureBootConfigImpl.c                    | 37 ++++++++++---------
>  1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.c
> index 2c11129526..e2e61d1e07 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igIm
> pl.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
> (&gSecureBootConfigFormSetGuid, 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
> (&gSecureBootConfigFormSetGuid, 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;
>
>        }
>
> @@ -5179,6 +5178,10 @@ SecureBootCallback (
>      }
>
>    }
>
>
>
> +  if (GetBrowserDataResult) {
>
> +    SecureBootExtractConfigFromVariable (Private, IfrNvData);
>
> +  }
>
> +
>
>  EXIT:
>
>
>
>    if (!EFI_ERROR (Status) && GetBrowserDataResult) {
>
> --
> 2.31.1.windows.1

-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116180): https://edk2.groups.io/g/devel/message/116180
Mute This Topic: https://groups.io/mt/104596915/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-