[PATCH 0/2] EFI Capsule update explicitly sets dfu_alt_info

[PATCH 0/2] EFI Capsule update explicitly sets dfu_alt_info

[Jonathan Humphreys]

For capsule update, explicitly set the dfu_alt_info environment variable
before the DFU operation, and then restore it to the original value.
Previously, the dfu_alt_info environment variable was set with the
set_dfu_alt_info() function.

The problem with setting the capsule update's dfu_alt_info setting in
set_dfu_alt_info() is that set_dfu_alt_info() lacks the context of what DFU
operation is being performed (eg, capsule update, DFU boot, listing the
alt_info, etc) so the capsule update setting was overwriting the setting
for other DFU operations.

Jonathan Humphreys (2):
  efi_firmware: set EFI capsule dfu_alt_info env explicitly
  board: remove capsule update support in set_dfu_alt_info()

 board/beagle/beagleboneai64/beagleboneai64.c  |  8 -----
 board/beagle/beagleplay/beagleplay.c          |  8 -----
 .../aml-a311d-cc/aml-a311d-cc.c               |  2 --
 .../aml-s805x-ac/aml-s805x-ac.c               |  2 --
 .../aml-s905d3-cc/aml-s905d3-cc.c             |  2 --
 board/phytec/common/k3/board.c                |  8 -----
 board/ti/am62px/evm.c                         |  8 -----
 board/ti/am62x/evm.c                          |  8 -----
 board/ti/am64x/evm.c                          |  8 -----
 board/ti/j721e/evm.c                          |  8 -----
 board/ti/j784s4/evm.c                         |  8 -----
 lib/efi_loader/Kconfig                        |  2 --
 lib/efi_loader/efi_firmware.c                 | 34 ++++++++++++++++---
 13 files changed, 29 insertions(+), 77 deletions(-)

-- 
2.34.1

[PATCH 1/2] efi_firmware: set EFI capsule dfu_alt_info env explicitly

[Jonathan Humphreys]

The current implementation of EFI capsule update uses set_dfu_alt_info() to
set the dfu_alt_info environment variable with the settings it requires.
However, set_dfu_alt_info() is doing this for all DFU operations, even
those unrelated to capsule update.

Thus other uses of DFU, such as DFU boot which sets its own value for the
dfu_alt_info environment variable, will have that setting overwritten with
the capsule update setting. Similarly, any user defined value for the
dfu_alt_info environment variable would get overwritten when any DFU
operation was performed, including simply performing a "dfu 0 list"
command.

The solution is stop using the set_dfu_alt_info() mechanism to set the
dfu_alt_info environment variable and instead explicitly set it to the
capsule update's setting just before performing the capsule update's DFU
operation, and then restore the environment variable back to its original
value.

This patch implements the explicit setting and restoring of the
dfu_alt_info environment variable as part of the EFI capsule update
operation.

The fix is fully implemented in a subsequent patch that removes the capsule
update dfu_alt_info support in set_dfu_alt_info().

Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
---
 lib/efi_loader/efi_firmware.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 5a754c9cd03..d8b6d34ccab 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
 	efi_status_t (*progress)(efi_uintn_t completion),
 	u16 **abort_reason)
 {
+	int ret;
 	efi_status_t status;
 	struct fmp_state state = { 0 };
+	char *orig_dfu_env;
 
 	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
 		  image_size, vendor_code, progress, abort_reason);
@@ -663,9 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
 	if (status != EFI_SUCCESS)
 		return EFI_EXIT(status);
 
+	orig_dfu_env = env_get("dfu_alt_info");
+	ret = env_set("dfu_alt_info", update_info.dfu_string);
+	if (ret) {
+		pr_err("unable to set env variable \"dfu_alt_info\"!\n");
+		return -EINVAL;
+	}
+
 	if (fit_update(image))
 		return EFI_EXIT(EFI_DEVICE_ERROR);
 
+	ret = env_set("dfu_alt_info", orig_dfu_env);
+	if (ret) {
+		pr_err("unable to set env variable \"dfu_alt_info\"!\n");
+		return -EINVAL;
+	}
+
 	efi_firmware_set_fmp_state_var(&state, image_index);
 
 	return EFI_EXIT(EFI_SUCCESS);
@@ -717,6 +732,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 	u8 dfu_alt_num;
 	efi_status_t status;
 	struct fmp_state state = { 0 };
+	char *orig_dfu_env;
 
 	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
 		  image_size, vendor_code, progress, abort_reason);
@@ -747,10 +763,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 		}
 	}
 
+	orig_dfu_env = env_get("dfu_alt_info");
+	ret = env_set("dfu_alt_info", update_info.dfu_string);
+	if (ret) {
+		pr_err("unable to set env variable \"dfu_alt_info\"!\n");
+		return -EINVAL;
+	}
+
 	if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
 			     NULL, NULL))
 		return EFI_EXIT(EFI_DEVICE_ERROR);
 
+	ret = env_set("dfu_alt_info", orig_dfu_env);
+	if (ret) {
+		pr_err("unable to set env variable \"dfu_alt_info\"!\n");
+		return -EINVAL;
+	}
+
 	efi_firmware_set_fmp_state_var(&state, image_index);
 
 	return EFI_EXIT(EFI_SUCCESS);
-- 
2.34.1

[PATCH 2/2] board: remove capsule update support in set_dfu_alt_info()

[Jonathan Humphreys]

Now that capsule update sets the dfu_alt_info environment variable
explicitly, there is no need to support it in the set_dfu_alt_info()
function. Decouple SET_DFU_ALT_INFO from EFI_CAPSULE_FIRMWARE_FIT and
EFI_CAPSULE_FIRMWARE_RAW. For many boards, this was the only use of
set_dfu_alt_info() so remove the function entirely.

Fixes commit a9e6f01a941f ("efi: Define set_dfu_alt_info() for boards with UEFI capsule update enabled")

Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
---
 board/beagle/beagleboneai64/beagleboneai64.c       | 8 --------
 board/beagle/beagleplay/beagleplay.c               | 8 --------
 board/libre-computer/aml-a311d-cc/aml-a311d-cc.c   | 2 --
 board/libre-computer/aml-s805x-ac/aml-s805x-ac.c   | 2 --
 board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c | 2 --
 board/phytec/common/k3/board.c                     | 8 --------
 board/ti/am62px/evm.c                              | 8 --------
 board/ti/am62x/evm.c                               | 8 --------
 board/ti/am64x/evm.c                               | 8 --------
 board/ti/j721e/evm.c                               | 8 --------
 board/ti/j784s4/evm.c                              | 8 --------
 lib/efi_loader/Kconfig                             | 2 --
 lib/efi_loader/efi_firmware.c                      | 5 -----
 13 files changed, 77 deletions(-)

diff --git a/board/beagle/beagleboneai64/beagleboneai64.c b/board/beagle/beagleboneai64/beagleboneai64.c
index e8d07f1f95f..99eb8972cf3 100644
--- a/board/beagle/beagleboneai64/beagleboneai64.c
+++ b/board/beagle/beagleboneai64/beagleboneai64.c
@@ -45,14 +45,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/board/beagle/beagleplay/beagleplay.c b/board/beagle/beagleplay/beagleplay.c
index fae69b37585..78635810585 100644
--- a/board/beagle/beagleplay/beagleplay.c
+++ b/board/beagle/beagleplay/beagleplay.c
@@ -41,14 +41,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c b/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
index e45cfd5d8a3..24363d21ab0 100644
--- a/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
+++ b/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
@@ -31,8 +31,6 @@ void set_dfu_alt_info(char *interface, char *devstr)
 {
 	if (strcmp(interface, "ram") == 0)
 		env_set("dfu_alt_info", "fitimage ram 0x08080000 0x4000000");
-	else if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
 }
 #endif
 
diff --git a/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c b/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c
index 94cf5b4361f..42442f26acc 100644
--- a/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c
+++ b/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c
@@ -38,8 +38,6 @@ void set_dfu_alt_info(char *interface, char *devstr)
 {
 	if (strcmp(interface, "ram") == 0)
 		env_set("dfu_alt_info", "fitimage ram 0x08080000 0x4000000");
-	else if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
 }
 #endif
 
diff --git a/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c b/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c
index f641db5a494..5223f1c8ab8 100644
--- a/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c
+++ b/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c
@@ -31,8 +31,6 @@ void set_dfu_alt_info(char *interface, char *devstr)
 {
 	if (strcmp(interface, "ram") == 0)
 		env_set("dfu_alt_info", "fitimage ram 0x08080000 0x4000000");
-	else if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
 }
 #endif
 
diff --git a/board/phytec/common/k3/board.c b/board/phytec/common/k3/board.c
index 9d833456810..58859f279f1 100644
--- a/board/phytec/common/k3/board.c
+++ b/board/phytec/common/k3/board.c
@@ -82,14 +82,6 @@ void configure_capsule_updates(void)
 }
 #endif
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 #if IS_ENABLED(CONFIG_ENV_IS_IN_FAT) || IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
 int mmc_get_env_dev(void)
 {
diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c
index 75359fa1614..379d1a5b316 100644
--- a/board/ti/am62px/evm.c
+++ b/board/ti/am62px/evm.c
@@ -41,14 +41,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
index 279ceba9554..3051a0a27a1 100644
--- a/board/ti/am62x/evm.c
+++ b/board/ti/am62x/evm.c
@@ -74,14 +74,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
index 6a17737d266..35fd30dbceb 100644
--- a/board/ti/am64x/evm.c
+++ b/board/ti/am64x/evm.c
@@ -54,14 +54,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
index 1fa78ff7b30..0525f6e6f97 100644
--- a/board/ti/j721e/evm.c
+++ b/board/ti/j721e/evm.c
@@ -65,14 +65,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/board/ti/j784s4/evm.c b/board/ti/j784s4/evm.c
index d317f3eccbb..c6e46b7ee0e 100644
--- a/board/ti/j784s4/evm.c
+++ b/board/ti/j784s4/evm.c
@@ -40,14 +40,6 @@ struct efi_capsule_update_info update_info = {
 	.images = fw_images,
 };
 
-#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
-void set_dfu_alt_info(char *interface, char *devstr)
-{
-	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
-		env_set("dfu_alt_info", update_info.dfu_string);
-}
-#endif
-
 int board_init(void)
 {
 	return 0;
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d4f6b56afaa..1be11aed901 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -304,7 +304,6 @@ config EFI_CAPSULE_FIRMWARE_FIT
 	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
 	select UPDATE_FIT
 	select DFU
-	select SET_DFU_ALT_INFO
 	select EFI_CAPSULE_FIRMWARE
 	help
 	  Select this option if you want to enable firmware management protocol
@@ -316,7 +315,6 @@ config EFI_CAPSULE_FIRMWARE_RAW
 	depends on SANDBOX || (!SANDBOX && !EFI_CAPSULE_FIRMWARE_FIT)
 	select DFU_WRITE_ALT
 	select DFU
-	select SET_DFU_ALT_INFO
 	select EFI_CAPSULE_FIRMWARE
 	help
 	  Select this option if you want to enable firmware management protocol
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index d8b6d34ccab..3e92a143b90 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -56,11 +56,6 @@ struct fmp_state {
 	u32 last_attempt_status; /* not used */
 };
 
-__weak void set_dfu_alt_info(char *interface, char *devstr)
-{
-	env_set("dfu_alt_info", update_info.dfu_string);
-}
-
 /**
  * efi_firmware_get_image_type_id - get image_type_id
  * @image_index:	image index
-- 
2.34.1

Re: [PATCH 1/2] efi_firmware: set EFI capsule dfu_alt_info env explicitly

[Mattijs Korpershoek]

Hi Jon,

Thank you for the patch.

On lun., févr. 03, 2025 at 15:53, Jonathan Humphreys <j-humphreys@ti.com> wrote:

> The current implementation of EFI capsule update uses set_dfu_alt_info() to
> set the dfu_alt_info environment variable with the settings it requires.
> However, set_dfu_alt_info() is doing this for all DFU operations, even
> those unrelated to capsule update.
>
> Thus other uses of DFU, such as DFU boot which sets its own value for the
> dfu_alt_info environment variable, will have that setting overwritten with
> the capsule update setting. Similarly, any user defined value for the
> dfu_alt_info environment variable would get overwritten when any DFU
> operation was performed, including simply performing a "dfu 0 list"
> command.
>
> The solution is stop using the set_dfu_alt_info() mechanism to set the
> dfu_alt_info environment variable and instead explicitly set it to the
> capsule update's setting just before performing the capsule update's DFU
> operation, and then restore the environment variable back to its original
> value.
>
> This patch implements the explicit setting and restoring of the
> dfu_alt_info environment variable as part of the EFI capsule update
> operation.
>
> The fix is fully implemented in a subsequent patch that removes the capsule
> update dfu_alt_info support in set_dfu_alt_info().
>
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> ---
>  lib/efi_loader/efi_firmware.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 5a754c9cd03..d8b6d34ccab 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	efi_status_t (*progress)(efi_uintn_t completion),
>  	u16 **abort_reason)
>  {
> +	int ret;
>  	efi_status_t status;
>  	struct fmp_state state = { 0 };
> +	char *orig_dfu_env;
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -663,9 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	if (status != EFI_SUCCESS)
>  		return EFI_EXIT(status);
>  
> +	orig_dfu_env = env_get("dfu_alt_info");
> +	ret = env_set("dfu_alt_info", update_info.dfu_string);
> +	if (ret) {
> +		pr_err("unable to set env variable \"dfu_alt_info\"!\n");

I could not find any other usage of pr_err() in this file so I think
we should use the log_err() instead of pr_err() here.

> +		return -EINVAL;
> +	}
> +
>  	if (fit_update(image))

Should we restore to orig_dfu_env in case fit_update() fails?
If not, why not? Maybe we can state this in the commit message or in a
code comment?

>  		return EFI_EXIT(EFI_DEVICE_ERROR);
>  
> +	ret = env_set("dfu_alt_info", orig_dfu_env);
> +	if (ret) {
> +		pr_err("unable to set env variable \"dfu_alt_info\"!\n");

pr_err() -> log_err()

> +		return -EINVAL;
> +	}
> +
>  	efi_firmware_set_fmp_state_var(&state, image_index);
>  
>  	return EFI_EXIT(EFI_SUCCESS);
> @@ -717,6 +732,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	u8 dfu_alt_num;
>  	efi_status_t status;
>  	struct fmp_state state = { 0 };
> +	char *orig_dfu_env;
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -747,10 +763,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  		}
>  	}
>  
> +	orig_dfu_env = env_get("dfu_alt_info");
> +	ret = env_set("dfu_alt_info", update_info.dfu_string);
> +	if (ret) {
> +		pr_err("unable to set env variable \"dfu_alt_info\"!\n");

pr_err() -> log_err()

> +		return -EINVAL;
> +	}
> +
>  	if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
>  			     NULL, NULL))
>  		return EFI_EXIT(EFI_DEVICE_ERROR);
>  
> +	ret = env_set("dfu_alt_info", orig_dfu_env);
> +	if (ret) {
> +		pr_err("unable to set env variable \"dfu_alt_info\"!\n");

pr_err() -> log_err()

> +		return -EINVAL;
> +	}
> +
>  	efi_firmware_set_fmp_state_var(&state, image_index);
>  
>  	return EFI_EXIT(EFI_SUCCESS);
> -- 
> 2.34.1

Re: [PATCH 2/2] board: remove capsule update support in set_dfu_alt_info()

[Mattijs Korpershoek]

Hi Jon,

Thank you for the patch.

On lun., févr. 03, 2025 at 15:53, Jonathan Humphreys <j-humphreys@ti.com> wrote:

> Now that capsule update sets the dfu_alt_info environment variable
> explicitly, there is no need to support it in the set_dfu_alt_info()
> function. Decouple SET_DFU_ALT_INFO from EFI_CAPSULE_FIRMWARE_FIT and
> EFI_CAPSULE_FIRMWARE_RAW. For many boards, this was the only use of
> set_dfu_alt_info() so remove the function entirely.
>
> Fixes commit a9e6f01a941f ("efi: Define set_dfu_alt_info() for boards with UEFI capsule update enabled")
>
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>

Nitpick: use standard Fixes syntax here please. It should be:
"""
Fixes: a9e6f01a941f ("efi: Define set_dfu_alt_info() for boards with UEFI capsule update enabled")
Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
"""

With that addressed, please add:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  board/beagle/beagleboneai64/beagleboneai64.c       | 8 --------
>  board/beagle/beagleplay/beagleplay.c               | 8 --------
>  board/libre-computer/aml-a311d-cc/aml-a311d-cc.c   | 2 --
>  board/libre-computer/aml-s805x-ac/aml-s805x-ac.c   | 2 --
>  board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c | 2 --
>  board/phytec/common/k3/board.c                     | 8 --------
>  board/ti/am62px/evm.c                              | 8 --------
>  board/ti/am62x/evm.c                               | 8 --------
>  board/ti/am64x/evm.c                               | 8 --------
>  board/ti/j721e/evm.c                               | 8 --------
>  board/ti/j784s4/evm.c                              | 8 --------
>  lib/efi_loader/Kconfig                             | 2 --
>  lib/efi_loader/efi_firmware.c                      | 5 -----
>  13 files changed, 77 deletions(-)
>
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.c b/board/beagle/beagleboneai64/beagleboneai64.c
> index e8d07f1f95f..99eb8972cf3 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.c
> +++ b/board/beagle/beagleboneai64/beagleboneai64.c
> @@ -45,14 +45,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/board/beagle/beagleplay/beagleplay.c b/board/beagle/beagleplay/beagleplay.c
> index fae69b37585..78635810585 100644
> --- a/board/beagle/beagleplay/beagleplay.c
> +++ b/board/beagle/beagleplay/beagleplay.c
> @@ -41,14 +41,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c b/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
> index e45cfd5d8a3..24363d21ab0 100644
> --- a/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
> +++ b/board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
> @@ -31,8 +31,6 @@ void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	if (strcmp(interface, "ram") == 0)
>  		env_set("dfu_alt_info", "fitimage ram 0x08080000 0x4000000");
> -	else if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
>  }
>  #endif
>  
> diff --git a/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c b/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c
> index 94cf5b4361f..42442f26acc 100644
> --- a/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c
> +++ b/board/libre-computer/aml-s805x-ac/aml-s805x-ac.c
> @@ -38,8 +38,6 @@ void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	if (strcmp(interface, "ram") == 0)
>  		env_set("dfu_alt_info", "fitimage ram 0x08080000 0x4000000");
> -	else if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
>  }
>  #endif
>  
> diff --git a/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c b/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c
> index f641db5a494..5223f1c8ab8 100644
> --- a/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c
> +++ b/board/libre-computer/aml-s905d3-cc/aml-s905d3-cc.c
> @@ -31,8 +31,6 @@ void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	if (strcmp(interface, "ram") == 0)
>  		env_set("dfu_alt_info", "fitimage ram 0x08080000 0x4000000");
> -	else if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
>  }
>  #endif
>  
> diff --git a/board/phytec/common/k3/board.c b/board/phytec/common/k3/board.c
> index 9d833456810..58859f279f1 100644
> --- a/board/phytec/common/k3/board.c
> +++ b/board/phytec/common/k3/board.c
> @@ -82,14 +82,6 @@ void configure_capsule_updates(void)
>  }
>  #endif
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  #if IS_ENABLED(CONFIG_ENV_IS_IN_FAT) || IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
>  int mmc_get_env_dev(void)
>  {
> diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c
> index 75359fa1614..379d1a5b316 100644
> --- a/board/ti/am62px/evm.c
> +++ b/board/ti/am62px/evm.c
> @@ -41,14 +41,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
> index 279ceba9554..3051a0a27a1 100644
> --- a/board/ti/am62x/evm.c
> +++ b/board/ti/am62x/evm.c
> @@ -74,14 +74,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
> index 6a17737d266..35fd30dbceb 100644
> --- a/board/ti/am64x/evm.c
> +++ b/board/ti/am64x/evm.c
> @@ -54,14 +54,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
> index 1fa78ff7b30..0525f6e6f97 100644
> --- a/board/ti/j721e/evm.c
> +++ b/board/ti/j721e/evm.c
> @@ -65,14 +65,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/board/ti/j784s4/evm.c b/board/ti/j784s4/evm.c
> index d317f3eccbb..c6e46b7ee0e 100644
> --- a/board/ti/j784s4/evm.c
> +++ b/board/ti/j784s4/evm.c
> @@ -40,14 +40,6 @@ struct efi_capsule_update_info update_info = {
>  	.images = fw_images,
>  };
>  
> -#if IS_ENABLED(CONFIG_SET_DFU_ALT_INFO)
> -void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> -		env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -#endif
> -
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index d4f6b56afaa..1be11aed901 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -304,7 +304,6 @@ config EFI_CAPSULE_FIRMWARE_FIT
>  	depends on EFI_CAPSULE_FIRMWARE_MANAGEMENT
>  	select UPDATE_FIT
>  	select DFU
> -	select SET_DFU_ALT_INFO
>  	select EFI_CAPSULE_FIRMWARE
>  	help
>  	  Select this option if you want to enable firmware management protocol
> @@ -316,7 +315,6 @@ config EFI_CAPSULE_FIRMWARE_RAW
>  	depends on SANDBOX || (!SANDBOX && !EFI_CAPSULE_FIRMWARE_FIT)
>  	select DFU_WRITE_ALT
>  	select DFU
> -	select SET_DFU_ALT_INFO
>  	select EFI_CAPSULE_FIRMWARE
>  	help
>  	  Select this option if you want to enable firmware management protocol
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index d8b6d34ccab..3e92a143b90 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -56,11 +56,6 @@ struct fmp_state {
>  	u32 last_attempt_status; /* not used */
>  };
>  
> -__weak void set_dfu_alt_info(char *interface, char *devstr)
> -{
> -	env_set("dfu_alt_info", update_info.dfu_string);
> -}
> -
>  /**
>   * efi_firmware_get_image_type_id - get image_type_id
>   * @image_index:	image index
> -- 
> 2.34.1

Re: [PATCH 1/2] efi_firmware: set EFI capsule dfu_alt_info env explicitly

[Ilias Apalodimas]

Hi Jonathan,

On Mon, 3 Feb 2025 at 23:54, Jonathan Humphreys <j-humphreys@ti.com> wrote:
>
> The current implementation of EFI capsule update uses set_dfu_alt_info() to
> set the dfu_alt_info environment variable with the settings it requires.
> However, set_dfu_alt_info() is doing this for all DFU operations, even
> those unrelated to capsule update.
>
> Thus other uses of DFU, such as DFU boot which sets its own value for the
> dfu_alt_info environment variable, will have that setting overwritten with
> the capsule update setting. Similarly, any user defined value for the
> dfu_alt_info environment variable would get overwritten when any DFU
> operation was performed, including simply performing a "dfu 0 list"
> command.
>
> The solution is stop using the set_dfu_alt_info() mechanism to set the
> dfu_alt_info environment variable and instead explicitly set it to the
> capsule update's setting just before performing the capsule update's DFU
> operation, and then restore the environment variable back to its original
> value.
>
> This patch implements the explicit setting and restoring of the
> dfu_alt_info environment variable as part of the EFI capsule update
> operation.
>
> The fix is fully implemented in a subsequent patch that removes the capsule
> update dfu_alt_info support in set_dfu_alt_info().
>
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> ---
>  lib/efi_loader/efi_firmware.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 5a754c9cd03..d8b6d34ccab 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -649,8 +649,10 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>         efi_status_t (*progress)(efi_uintn_t completion),
>         u16 **abort_reason)
>  {
> +       int ret;
>         efi_status_t status;
>         struct fmp_state state = { 0 };
> +       char *orig_dfu_env;
>
>         EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>                   image_size, vendor_code, progress, abort_reason);
> @@ -663,9 +665,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>         if (status != EFI_SUCCESS)
>                 return EFI_EXIT(status);
>
> +       orig_dfu_env = env_get("dfu_alt_info");

I don't think this will work. env_get returns a pointer to the data
which is going to change after the env_set
IOW this piece

        env_set("dfu_alt_info", "test1");
        orig_dfu_env = env_get("dfu_alt_info");
        printf("now %s\n", orig_dfu_env); -> will print test1

        env_set("dfu_alt_info", "test2");
        dfu_env = env_get("dfu_alt_info");
        printf("Orig %s now %s\n", orig_dfu_env, lala); -> will print
test2 test2

You have to copy the value of the pointer to an allocated piece of
memory to restore it

Thanks
/Ilias


> +       ret = env_set("dfu_alt_info", update_info.dfu_string);
> +       if (ret) {
> +               pr_err("unable to set env variable \"dfu_alt_info\"!\n");
> +               return -EINVAL;
> +       }
> +
>         if (fit_update(image))
>                 return EFI_EXIT(EFI_DEVICE_ERROR);
>
> +       ret = env_set("dfu_alt_info", orig_dfu_env);
> +       if (ret) {
> +               pr_err("unable to set env variable \"dfu_alt_info\"!\n");
> +               return -EINVAL;
> +       }
> +
>         efi_firmware_set_fmp_state_var(&state, image_index);
>
>         return EFI_EXIT(EFI_SUCCESS);
> @@ -717,6 +732,7 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>         u8 dfu_alt_num;
>         efi_status_t status;
>         struct fmp_state state = { 0 };
> +       char *orig_dfu_env;
>
>         EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>                   image_size, vendor_code, progress, abort_reason);
> @@ -747,10 +763,23 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>                 }
>         }
>
> +       orig_dfu_env = env_get("dfu_alt_info");
> +       ret = env_set("dfu_alt_info", update_info.dfu_string);
> +       if (ret) {
> +               pr_err("unable to set env variable \"dfu_alt_info\"!\n");
> +               return -EINVAL;
> +       }
> +
>         if (dfu_write_by_alt(dfu_alt_num, (void *)image, image_size,
>                              NULL, NULL))
>                 return EFI_EXIT(EFI_DEVICE_ERROR);
>
> +       ret = env_set("dfu_alt_info", orig_dfu_env);
> +       if (ret) {
> +               pr_err("unable to set env variable \"dfu_alt_info\"!\n");
> +               return -EINVAL;
> +       }
> +
>         efi_firmware_set_fmp_state_var(&state, image_index);
>
>         return EFI_EXIT(EFI_SUCCESS);
> --
> 2.34.1
>