diff mbox series

[v2,1/4] efi_loader: capsule: Remove the check for capsule_authentication_enabled environment variable

Message ID 20210412150526.29822-2-sughosh.ganu@linaro.org
State Accepted
Commit 6a2e26b95f046a2973a95119910cbe2554c92b46
Headers show
Series Add support for embedding public key in platform's dtb | expand

Commit Message

Sughosh Ganu April 12, 2021, 3:05 p.m. UTC 
The current capsule authentication code checks if the environment
variable capsule_authentication_enabled is set, for authenticating the
capsule. This is in addition to the check for the config symbol
CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment
variable. The capsule will now be authenticated if the config symbol
is set.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

---

Changes since V1:
* As pointed out by Heinrich in the review, remove the extra check of
  the env variable 'capsule_authentication_enabled'for authenticating
  the capsule. The capsule authentication will now be done based on
  whether the corresponding config symbol is enabled.

 board/emulation/common/qemu_capsule.c | 6 ------
 lib/efi_loader/efi_firmware.c         | 5 ++---
 2 files changed, 2 insertions(+), 9 deletions(-)

-- 
2.17.1

Comments

Heinrich Schuchardt April 25, 2021, 7:15 a.m. UTC | #1 
On 4/12/21 5:05 PM, Sughosh Ganu wrote:
> The current capsule authentication code checks if the environment

> variable capsule_authentication_enabled is set, for authenticating the

> capsule. This is in addition to the check for the config symbol

> CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment

> variable. The capsule will now be authenticated if the config symbol

> is set.

>

> Signed-off-by: Sughosh Ganu<sughosh.ganu@linaro.org>


Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Heinrich Schuchardt May 5, 2021, 8:23 p.m. UTC | #2 
On 4/12/21 5:05 PM, Sughosh Ganu wrote:
> The current capsule authentication code checks if the environment

> variable capsule_authentication_enabled is set, for authenticating the

> capsule. This is in addition to the check for the config symbol

> CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment

> variable. The capsule will now be authenticated if the config symbol

> is set.

>

> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>


doc/board/emulation/qemu_capsule_update.rst mentions the environment
variable. So this file needs to be updated too.

Will you provide an extra patch or update this one?

Best regards

Heinrich

> ---

>

> Changes since V1:

> * As pointed out by Heinrich in the review, remove the extra check of

>    the env variable 'capsule_authentication_enabled'for authenticating

>    the capsule. The capsule authentication will now be done based on

>    whether the corresponding config symbol is enabled.

>

>   board/emulation/common/qemu_capsule.c | 6 ------

>   lib/efi_loader/efi_firmware.c         | 5 ++---

>   2 files changed, 2 insertions(+), 9 deletions(-)

>

> diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c

> index 5cb461d52b..6b8a87022a 100644

> --- a/board/emulation/common/qemu_capsule.c

> +++ b/board/emulation/common/qemu_capsule.c

> @@ -41,9 +41,3 @@ int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)

>

>   	return 0;

>   }

> -

> -bool efi_capsule_auth_enabled(void)

> -{

> -	return env_get("capsule_authentication_enabled") != NULL ?

> -		true : false;

> -}

> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c

> index 7a3cca2793..a1b88dbfc2 100644

> --- a/lib/efi_loader/efi_firmware.c

> +++ b/lib/efi_loader/efi_firmware.c

> @@ -190,7 +190,7 @@ static efi_status_t efi_get_dfu_info(

>   				IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;

>

>   		/* Check if the capsule authentication is enabled */

> -		if (env_get("capsule_authentication_enabled"))

> +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE))

>   			image_info[0].attributes_setting |=

>   				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;

>

> @@ -421,8 +421,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(

>   		return EFI_EXIT(EFI_INVALID_PARAMETER);

>

>   	/* Authenticate the capsule if authentication enabled */

> -	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&

> -	    env_get("capsule_authentication_enabled")) {

> +	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)) {

>   		capsule_payload = NULL;

>   		capsule_payload_size = 0;

>   		status = efi_capsule_authenticate(image, image_size,

>
AKASHI Takahiro May 7, 2021, 8:42 a.m. UTC | #3 
On Mon, Apr 12, 2021 at 08:35:23PM +0530, Sughosh Ganu wrote:
> The current capsule authentication code checks if the environment

> variable capsule_authentication_enabled is set, for authenticating the

> capsule. This is in addition to the check for the config symbol

> CONFIG_EFI_CAPSULE_AUTHENTICATE. Remove the check for the environment

> variable. The capsule will now be authenticated if the config symbol

> is set.

> 

> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

> ---

> 

> Changes since V1:

> * As pointed out by Heinrich in the review, remove the extra check of

>   the env variable 'capsule_authentication_enabled'for authenticating

>   the capsule. The capsule authentication will now be done based on

>   whether the corresponding config symbol is enabled.

> 

>  board/emulation/common/qemu_capsule.c | 6 ------

>  lib/efi_loader/efi_firmware.c         | 5 ++---

>  2 files changed, 2 insertions(+), 9 deletions(-)

> 

> diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c

> index 5cb461d52b..6b8a87022a 100644

> --- a/board/emulation/common/qemu_capsule.c

> +++ b/board/emulation/common/qemu_capsule.c

> @@ -41,9 +41,3 @@ int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)

>  

>  	return 0;

>  }

> -

> -bool efi_capsule_auth_enabled(void)

> -{

> -	return env_get("capsule_authentication_enabled") != NULL ?

> -		true : false;

> -}

> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c

> index 7a3cca2793..a1b88dbfc2 100644

> --- a/lib/efi_loader/efi_firmware.c

> +++ b/lib/efi_loader/efi_firmware.c

> @@ -190,7 +190,7 @@ static efi_status_t efi_get_dfu_info(

>  				IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;

>  

>  		/* Check if the capsule authentication is enabled */

> -		if (env_get("capsule_authentication_enabled"))

> +		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE))

>  			image_info[0].attributes_setting |=

>  				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;

>  

> @@ -421,8 +421,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(

>  		return EFI_EXIT(EFI_INVALID_PARAMETER);

>  

>  	/* Authenticate the capsule if authentication enabled */

> -	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&

> -	    env_get("capsule_authentication_enabled")) {

> +	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)) {


This change is not enough;
1. When a *signed* capsule file is applied on U-Boot with
EFI_CAPSULE_AUTHENTICATE disabled, the "authenticode" data
must be excluded from the data to write.
In other words, the offset and the size in a capsule file, 
image & image_size, must also be updated before writing even
if the authentication is not performed.

Otherwise, wrong data will be stored.

2. UEFI specification requires that the authentication must be
performed only if IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED is
set on the image (image type or ESRT?).
We must always check with the attribute.

-Takahiro Akashi

>  		capsule_payload = NULL;

>  		capsule_payload_size = 0;

>  		status = efi_capsule_authenticate(image, image_size,

> -- 

> 2.17.1

>
diff mbox series

Patch

diff --git a/board/emulation/common/qemu_capsule.c b/board/emulation/common/qemu_capsule.c
index 5cb461d52b..6b8a87022a 100644
--- a/board/emulation/common/qemu_capsule.c
+++ b/board/emulation/common/qemu_capsule.c
@@ -41,9 +41,3 @@  int efi_get_public_key_data(void **pkey, efi_uintn_t *pkey_len)
 
 	return 0;
 }
-
-bool efi_capsule_auth_enabled(void)
-{
-	return env_get("capsule_authentication_enabled") != NULL ?
-		true : false;
-}
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 7a3cca2793..a1b88dbfc2 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -190,7 +190,7 @@  static efi_status_t efi_get_dfu_info(
 				IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
 
 		/* Check if the capsule authentication is enabled */
-		if (env_get("capsule_authentication_enabled"))
+		if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE))
 			image_info[0].attributes_setting |=
 				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
 
@@ -421,8 +421,7 @@  efi_status_t EFIAPI efi_firmware_raw_set_image(
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
 	/* Authenticate the capsule if authentication enabled */
-	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
-	    env_get("capsule_authentication_enabled")) {
+	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE)) {
 		capsule_payload = NULL;
 		capsule_payload_size = 0;
 		status = efi_capsule_authenticate(image, image_size,