[PATCH] dfu: Prevent set_dfu_alt_info() from overwriting a previous value
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com --- drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
+ str_env = env_get("dfu_alt_info"); #ifdef CONFIG_SET_DFU_ALT_INFO - set_dfu_alt_info(interface, devstr); + if (!str_env) { + set_dfu_alt_info(interface, devstr); + str_env = env_get("dfu_alt_info"); + } #endif - str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
On Tue, Dec 17, 2024 at 02:48:35PM -0600, Jonathan Humphreys wrote:
Hello Jon,
Thank you for posting this patch. I will drop the equivalent of this patch when I post the v2 series for: https://patchwork.ozlabs.org/project/uboot/cover/20241217131658.2920799-1-s-...
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info
nitpick: s/it's/its
before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
nitpick: s/get's/gets
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
Rather than referring to "patch", we could rephrase the above as:
Check that dfu_alt_info isn't already set before calling set_dfu_alt_info() when CONFIG_SET_DFU_ALT_INFO is enabled, in order to avoid overwriting it.
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
Regards, Siddharth.
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
* board/xilinx/zynq/board.c * board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
* board/libre-computer/aml-a311d-cc/aml-a311d-cc.c * board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs Korpershoek mkorpershoek@baylibre.com writes:
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
- board/xilinx/zynq/board.c
- board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
- board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
- board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Because I would then need to duplicate the same logic for every board that wanted both USB DFU boot and EFI capsules to work. And the paramters passed in do not allow the function to know the use case (am I DFU booting or updating EFI capsules?). See more below.
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs, thanks for the thorough reply. I did wrestle a lot with how wide of a fix to propose for this problem, and in the end, decided on the narrow fix of simply preventing the overwriting of the variable.
Yes it is a policy change, but the policy is already unclear, inconsistent, and confusing, IMO.
For example: 1) EFI capsule update wants to very strictly control the dfu alt values by setting it in set_dfu_alt_info(), but then any other DFU use case breaks. USB DFU boot is now broken. 2) The behavior the user sees wrt the dfu_alt_info env variable is very confusing and non-intuitive. Take this example:
=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000" => env print dfu_alt_info dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000 => dfu 0 list DFU alt settings list: dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR => env print dfu_alt_info dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000 =>
As you can see, the user set's the dfu_alt_info variable according to their specific use case, then simply tries to list the DFU alt settings, and because this code goes through the dfu_init_env_entities() path, it gets changed to the EFI capsule settings.
I was hoping to get a simpler fix in now so we can get USB DFU boot working again, and we can visit the overall policy design next. As you suggest, I could also push the testing of overwriting into the board specific set_dfu_alt_info() function, but then I need to duplicate the code in 8 different places for the TI boards, and other vendors may still have the problem.
Looking to the longer term solution, here are my thoughts. 1) We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. The only reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI capsule update is enabled. Outside of a few legacy uses (I think - it appears they were introduced prior to supporting multi-interface dfu alt strings), I think this is true for other vendor's boards as well. 2) Have EFI capsule support do as USB DFU boot does today, and set the dfu alt string it wants used *before* initiating the DFU operation. With CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get overridden. 3) Have the actual value of the dfu alt string used in the DFU operation be passed in, rather than read from the dfu_alt_info environment variable. The USB DFU and EFI capsule use case will pass in the dfu alt string they want. The standard 'dfu' command can pass in the value of the dfu_alt_info env variable. Note that this effectively decouples the dfu command from the alt settings that USB DFU boot and EFI capsules use, but I think this is what we want.
This then allows both USB DFU boot and EFI capsule use cases to work as intended and allows the dfu command to operate on the user defined dfu_alt_info value.
I welcome comments from those that have the history and intended behavior background of the DFU support :).
I also welcome comments on how to proceed for 2025.01. Should we live with USB DFU boot broken until we get the long term fix in, or ok with the patch posted here. The patch posted here does allow for a user to change EFI capsule's dfu alt settings, as Mattijs says, but especially given capsules can be authenticated, I'm not sure how this would be exploited, and if that risk is worse that broken DFU boot.
thank Jon
Hi Jon,
Sorry for the (very) late reply. I had some long holidays in between and since this is a difficult topic for me, I kept pushing this to the end of my backlog.
On mer., déc. 18, 2024 at 17:09, Jon Humphreys j-humphreys@ti.com wrote:
Mattijs Korpershoek mkorpershoek@baylibre.com writes:
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
- board/xilinx/zynq/board.c
- board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
- board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
- board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Because I would then need to duplicate the same logic for every board that wanted both USB DFU boot and EFI capsules to work. And the paramters passed in do not allow the function to know the use case (am I DFU booting or updating EFI capsules?). See more below.
I understand that duplicating logic for every board you maintain is not optimal, however, it gives each vendor the freedom of implementing their policy.
I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.
Heinrich, Ilias, Sugosh, do you have any opinion on this patch?
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs, thanks for the thorough reply. I did wrestle a lot with how wide of a fix to propose for this problem, and in the end, decided on the narrow fix of simply preventing the overwriting of the variable.
Yes it is a policy change, but the policy is already unclear, inconsistent, and confusing, IMO.
For example:
- EFI capsule update wants to very strictly control the dfu alt values by setting it in set_dfu_alt_info(), but then any other DFU use case breaks. USB DFU boot is now broken.
- The behavior the user sees wrt the dfu_alt_info env variable is very confusing and non-intuitive. Take this example:
=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000" => env print dfu_alt_info dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000 => dfu 0 list DFU alt settings list: dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR => env print dfu_alt_info dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000 =>
As you can see, the user set's the dfu_alt_info variable according to their specific use case, then simply tries to list the DFU alt settings, and because this code goes through the dfu_init_env_entities() path, it gets changed to the EFI capsule settings.
I was hoping to get a simpler fix in now so we can get USB DFU boot working again, and we can visit the overall policy design next. As you suggest, I could also push the testing of overwriting into the board specific set_dfu_alt_info() function, but then I need to duplicate the code in 8 different places for the TI boards, and other vendors may still have the problem.
I agree that the above behaviour is confusing and I'm reconsidering to take up this patch. I'd like some buy-in from either Heinrich, Ilias or Sughosh on this since I'm not 100% confortable with the "policy change"
Looking to the longer term solution, here are my thoughts.
- We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. The only reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI capsule update is enabled. Outside of a few legacy uses (I think - it appears they were introduced prior to supporting multi-interface dfu alt strings), I think this is true for other vendor's boards as well.
- Have EFI capsule support do as USB DFU boot does today, and set the dfu alt string it wants used *before* initiating the DFU operation. With CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get overridden.
- Have the actual value of the dfu alt string used in the DFU operation be passed in, rather than read from the dfu_alt_info environment variable. The USB DFU and EFI capsule use case will pass in the dfu alt string they want. The standard 'dfu' command can pass in the value of the dfu_alt_info env variable. Note that this effectively decouples the dfu command from the alt settings that USB DFU boot and EFI capsules use, but I think this is what we want.
This then allows both USB DFU boot and EFI capsule use cases to work as intended and allows the dfu command to operate on the user defined dfu_alt_info value.
I welcome comments from those that have the history and intended behavior background of the DFU support :).
I do as well. I have taken over maintaince on this subsystem a year ago and have not had much patches/work done on the subsystem. Therefore I'm not as knowledgeable as I would have liked to be. I'm sorry about that.
I also welcome comments on how to proceed for 2025.01. Should we live with USB DFU boot broken until we get the long term fix in, or ok with the patch posted here. The patch posted here does allow for a user to change EFI capsule's dfu alt settings, as Mattijs says, but especially given capsules can be authenticated, I'm not sure how this would be exploited, and if that risk is worse that broken DFU boot.
thank Jon
Hi Mattijs,
On Thu, 16 Jan 2025 at 10:37, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Hi Jon,
Sorry for the (very) late reply. I had some long holidays in between and since this is a difficult topic for me, I kept pushing this to the end of my backlog.
On mer., déc. 18, 2024 at 17:09, Jon Humphreys j-humphreys@ti.com wrote:
Mattijs Korpershoek mkorpershoek@baylibre.com writes:
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
- board/xilinx/zynq/board.c
- board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
- board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
- board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Because I would then need to duplicate the same logic for every board that wanted both USB DFU boot and EFI capsules to work. And the paramters passed in do not allow the function to know the use case (am I DFU booting or updating EFI capsules?). See more below.
I understand that duplicating logic for every board you maintain is not optimal, however, it gives each vendor the freedom of implementing their policy.
I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.
Heinrich, Ilias, Sugosh, do you have any opinion on this patch?
Looking at it quickly, this would break the capsule update functionality if a user sets a dfu_alt_info (or if he has one defined in their board env file). A board is generally expected to reset after a capsule update, so should we consider implementing something different? set_dfu_alt_info() unconditionally runs when dfu_init_env_entities() runs. We could teach the EFI subsystem to set the dfu_alt_info just before a capsule update runs and store any existing values. Then we can restore it after the capsule update finishes
Cheers /Ilias
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs, thanks for the thorough reply. I did wrestle a lot with how wide of a fix to propose for this problem, and in the end, decided on the narrow fix of simply preventing the overwriting of the variable.
Yes it is a policy change, but the policy is already unclear, inconsistent, and confusing, IMO.
For example:
- EFI capsule update wants to very strictly control the dfu alt values by setting it in set_dfu_alt_info(), but then any other DFU use case breaks. USB DFU boot is now broken.
- The behavior the user sees wrt the dfu_alt_info env variable is very confusing and non-intuitive. Take this example:
=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000" => env print dfu_alt_info dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000 => dfu 0 list DFU alt settings list: dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR => env print dfu_alt_info dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000 =>
As you can see, the user set's the dfu_alt_info variable according to their specific use case, then simply tries to list the DFU alt settings, and because this code goes through the dfu_init_env_entities() path, it gets changed to the EFI capsule settings.
I was hoping to get a simpler fix in now so we can get USB DFU boot working again, and we can visit the overall policy design next. As you suggest, I could also push the testing of overwriting into the board specific set_dfu_alt_info() function, but then I need to duplicate the code in 8 different places for the TI boards, and other vendors may still have the problem.
I agree that the above behaviour is confusing and I'm reconsidering to take up this patch. I'd like some buy-in from either Heinrich, Ilias or Sughosh on this since I'm not 100% confortable with the "policy change"
Looking to the longer term solution, here are my thoughts.
- We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. The only reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI capsule update is enabled. Outside of a few legacy uses (I think - it appears they were introduced prior to supporting multi-interface dfu alt strings), I think this is true for other vendor's boards as well.
- Have EFI capsule support do as USB DFU boot does today, and set the dfu alt string it wants used *before* initiating the DFU operation. With CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get overridden.
- Have the actual value of the dfu alt string used in the DFU operation be passed in, rather than read from the dfu_alt_info environment variable. The USB DFU and EFI capsule use case will pass in the dfu alt string they want. The standard 'dfu' command can pass in the value of the dfu_alt_info env variable. Note that this effectively decouples the dfu command from the alt settings that USB DFU boot and EFI capsules use, but I think this is what we want.
This then allows both USB DFU boot and EFI capsule use cases to work as intended and allows the dfu command to operate on the user defined dfu_alt_info value.
I welcome comments from those that have the history and intended behavior background of the DFU support :).
I do as well. I have taken over maintaince on this subsystem a year ago and have not had much patches/work done on the subsystem. Therefore I'm not as knowledgeable as I would have liked to be. I'm sorry about that.
I also welcome comments on how to proceed for 2025.01. Should we live with USB DFU boot broken until we get the long term fix in, or ok with the patch posted here. The patch posted here does allow for a user to change EFI capsule's dfu alt settings, as Mattijs says, but especially given capsules can be authenticated, I'm not sure how this would be exploited, and if that risk is worse that broken DFU boot.
thank Jon
On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Hi Jon,
Sorry for the (very) late reply. I had some long holidays in between and since this is a difficult topic for me, I kept pushing this to the end of my backlog.
On mer., déc. 18, 2024 at 17:09, Jon Humphreys j-humphreys@ti.com wrote:
Mattijs Korpershoek mkorpershoek@baylibre.com writes:
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
- board/xilinx/zynq/board.c
- board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
- board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
- board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Because I would then need to duplicate the same logic for every board that wanted both USB DFU boot and EFI capsules to work. And the paramters passed in do not allow the function to know the use case (am I DFU booting or updating EFI capsules?). See more below.
I understand that duplicating logic for every board you maintain is not optimal, however, it gives each vendor the freedom of implementing their policy.
I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.
Heinrich, Ilias, Sugosh, do you have any opinion on this patch?
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs, thanks for the thorough reply. I did wrestle a lot with how wide of a fix to propose for this problem, and in the end, decided on the narrow fix of simply preventing the overwriting of the variable.
Yes it is a policy change, but the policy is already unclear, inconsistent, and confusing, IMO.
For example:
- EFI capsule update wants to very strictly control the dfu alt values by setting it in set_dfu_alt_info(), but then any other DFU use case breaks. USB DFU boot is now broken.
- The behavior the user sees wrt the dfu_alt_info env variable is very confusing and non-intuitive. Take this example:
=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000" => env print dfu_alt_info dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000 => dfu 0 list DFU alt settings list: dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR => env print dfu_alt_info dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000 =>
As you can see, the user set's the dfu_alt_info variable according to their specific use case, then simply tries to list the DFU alt settings, and because this code goes through the dfu_init_env_entities() path, it gets changed to the EFI capsule settings.
I was hoping to get a simpler fix in now so we can get USB DFU boot working again, and we can visit the overall policy design next. As you suggest, I could also push the testing of overwriting into the board specific set_dfu_alt_info() function, but then I need to duplicate the code in 8 different places for the TI boards, and other vendors may still have the problem.
I agree that the above behaviour is confusing and I'm reconsidering to take up this patch. I'd like some buy-in from either Heinrich, Ilias or Sughosh on this since I'm not 100% confortable with the "policy change"
A little context here. The DFU driver already had this policy in place where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string would be set by U-Boot, instead of taking the user provided string. It was decided to use this for EFI capsule updates, as getting the string which determines the location of writing the update images from within U-Boot is more resilient than taking some user provided string.
Looking to the longer term solution, here are my thoughts.
- We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. The only reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI capsule update is enabled. Outside of a few legacy uses (I think - it appears they were introduced prior to supporting multi-interface dfu alt strings), I think this is true for other vendor's boards as well.
- Have EFI capsule support do as USB DFU boot does today, and set the dfu alt string it wants used *before* initiating the DFU operation. With CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get overridden.
- Have the actual value of the dfu alt string used in the DFU operation be passed in, rather than read from the dfu_alt_info environment variable. The USB DFU and EFI capsule use case will pass in the dfu alt string they want. The standard 'dfu' command can pass in the value of the dfu_alt_info env variable. Note that this effectively decouples the dfu command from the alt settings that USB DFU boot and EFI capsules use, but I think this is what we want.
I think either of 2) or 3) above can be looked at. Although not sure if 2) will be breaking the current DFU policy.
-sughosh
This then allows both USB DFU boot and EFI capsule use cases to work as intended and allows the dfu command to operate on the user defined dfu_alt_info value.
I welcome comments from those that have the history and intended behavior background of the DFU support :).
I do as well. I have taken over maintaince on this subsystem a year ago and have not had much patches/work done on the subsystem. Therefore I'm not as knowledgeable as I would have liked to be. I'm sorry about that.
I also welcome comments on how to proceed for 2025.01. Should we live with USB DFU boot broken until we get the long term fix in, or ok with the patch posted here. The patch posted here does allow for a user to change EFI capsule's dfu alt settings, as Mattijs says, but especially given capsules can be authenticated, I'm not sure how this would be exploited, and if that risk is worse that broken DFU boot.
thank Jon
Sughosh Ganu sughosh.ganu@linaro.org writes:
On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Hi Jon,
Sorry for the (very) late reply. I had some long holidays in between and since this is a difficult topic for me, I kept pushing this to the end of my backlog.
On mer., déc. 18, 2024 at 17:09, Jon Humphreys j-humphreys@ti.com wrote:
Mattijs Korpershoek mkorpershoek@baylibre.com writes:
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
- board/xilinx/zynq/board.c
- board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
- board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
- board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Because I would then need to duplicate the same logic for every board that wanted both USB DFU boot and EFI capsules to work. And the paramters passed in do not allow the function to know the use case (am I DFU booting or updating EFI capsules?). See more below.
I understand that duplicating logic for every board you maintain is not optimal, however, it gives each vendor the freedom of implementing their policy.
I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.
Heinrich, Ilias, Sugosh, do you have any opinion on this patch?
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs, thanks for the thorough reply. I did wrestle a lot with how wide of a fix to propose for this problem, and in the end, decided on the narrow fix of simply preventing the overwriting of the variable.
Yes it is a policy change, but the policy is already unclear, inconsistent, and confusing, IMO.
For example:
- EFI capsule update wants to very strictly control the dfu alt values by setting it in set_dfu_alt_info(), but then any other DFU use case breaks. USB DFU boot is now broken.
- The behavior the user sees wrt the dfu_alt_info env variable is very confusing and non-intuitive. Take this example:
=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000" => env print dfu_alt_info dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000 => dfu 0 list DFU alt settings list: dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR => env print dfu_alt_info dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000 =>
As you can see, the user set's the dfu_alt_info variable according to their specific use case, then simply tries to list the DFU alt settings, and because this code goes through the dfu_init_env_entities() path, it gets changed to the EFI capsule settings.
I was hoping to get a simpler fix in now so we can get USB DFU boot working again, and we can visit the overall policy design next. As you suggest, I could also push the testing of overwriting into the board specific set_dfu_alt_info() function, but then I need to duplicate the code in 8 different places for the TI boards, and other vendors may still have the problem.
I agree that the above behaviour is confusing and I'm reconsidering to take up this patch. I'd like some buy-in from either Heinrich, Ilias or Sughosh on this since I'm not 100% confortable with the "policy change"
A little context here. The DFU driver already had this policy in place where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string would be set by U-Boot, instead of taking the user provided string. It was decided to use this for EFI capsule updates, as getting the string which determines the location of writing the update images from within U-Boot is more resilient than taking some user provided string.
Looking to the longer term solution, here are my thoughts.
- We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. The only reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI capsule update is enabled. Outside of a few legacy uses (I think - it appears they were introduced prior to supporting multi-interface dfu alt strings), I think this is true for other vendor's boards as well.
- Have EFI capsule support do as USB DFU boot does today, and set the dfu alt string it wants used *before* initiating the DFU operation. With CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get overridden.
- Have the actual value of the dfu alt string used in the DFU operation be passed in, rather than read from the dfu_alt_info environment variable. The USB DFU and EFI capsule use case will pass in the dfu alt string they want. The standard 'dfu' command can pass in the value of the dfu_alt_info env variable. Note that this effectively decouples the dfu command from the alt settings that USB DFU boot and EFI capsules use, but I think this is what we want.
I think either of 2) or 3) above can be looked at. Although not sure if 2) will be breaking the current DFU policy.
-sughosh
Thanks for the comments. I have looked into this a bit further and see 2 options we can take for the EFI capsule update use case:
1) stick with the more traditional approach and do as DFU BOOT does by setting the value of the dfu_alt_info env variable just before initiating the DFU operation. As Ilias sugggested, we could also add a save/restore so this is transparent to other DFU users.
2) move to a model where we explicitly pass in to the DFU operation the value of dfu_alt_info that we want used. In the normal/legacy DFU use cases, this would involve calling env_get() for the dfu_alt_info env variable and pass that value. I like this approach because it is cleaner and more explicit. However, there are many layers of function calls between the driver of the DFU operation (the one that would decide what dfu_alt_info value to use) and dfu_init_env_entities() where it is used, and passing this info across the function interfaces would involved lots of function interface updates.
I'm curious if there is apetite for 2) or should we go with the more traditional approach in 1).
The above assumes we decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. For platforms still setting CONFIG_SET_DFU_ALT_INFO, the presumption is that if they override the dfu_alt_info env variable, they know what they are doing.
Thanks Jon
This then allows both USB DFU boot and EFI capsule use cases to work as intended and allows the dfu command to operate on the user defined dfu_alt_info value.
I welcome comments from those that have the history and intended behavior background of the DFU support :).
I do as well. I have taken over maintaince on this subsystem a year ago and have not had much patches/work done on the subsystem. Therefore I'm not as knowledgeable as I would have liked to be. I'm sorry about that.
I also welcome comments on how to proceed for 2025.01. Should we live with USB DFU boot broken until we get the long term fix in, or ok with the patch posted here. The patch posted here does allow for a user to change EFI capsule's dfu alt settings, as Mattijs says, but especially given capsules can be authenticated, I'm not sure how this would be exploited, and if that risk is worse that broken DFU boot.
thank Jon
Hi Jon,
On Fri, 17 Jan 2025 at 00:02, Jon Humphreys j-humphreys@ti.com wrote:
Sughosh Ganu sughosh.ganu@linaro.org writes:
On Thu, 16 Jan 2025 at 14:07, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Hi Jon,
Sorry for the (very) late reply. I had some long holidays in between and since this is a difficult topic for me, I kept pushing this to the end of my backlog.
On mer., déc. 18, 2024 at 17:09, Jon Humphreys j-humphreys@ti.com wrote:
Mattijs Korpershoek mkorpershoek@baylibre.com writes:
Hi Jonathan,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:48, Jonathan Humphreys j-humphreys@ti.com wrote:
If CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable is dynamically set when initializing the DFU entities, which is done as part of normal flow of a DFU operation.
The USB DFU boot support will set it's own specific value for dfu_alt_info before performing the DFU operation. This means that if CONFIG_SET_DFU_ALT_INFO is enabled, the dfu_alt_info environment variable that the USB DFU boot path had set is overwritten, causing USB DFU boot to fail.
Likewise, if the user sets their own value for dfu_alt_info, say at the U-Boot prompt, it get's overwritten if CONFIG_SET_DFU_ALT_INFO is enabled.
This patch will first check that dfu_alt_info isn't already set before calling set_dfu_alt_info(), when CONFIG_SET_DFU_ALT_INFO is enabled.
To me, this is a policy change: before we could override the environment via set_dfu_alt_info(). Now we cannot anymore (if "dfu_alt_info" is already set in the environment).
Also, it seems that this change goes against the uefi doc which states:
""" A string is defined which is to be used for populating the dfu_alt_info variable. This string is used by the function set_dfu_alt_info. Instead of taking the variable from the environment, the capsule update feature requires that the variable be set through the function, since that is more robust. Allowing the user to change the location of the firmware updates is not a very secure practice. Getting this information from the firmware itself is more secure, assuming the firmware has been verified by a previous stage boot loader. """
See: https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#performing-the-upda...
Moreover, looking at various boards that implement set_dfu_alt_info(), we can see different behaviours:
Boards examples that won't override "dfu_alt_info" via set_dfu_alt_info() if "dfu_alt_info" is already set via environment
- board/xilinx/zynq/board.c
- board/emulation/common/qemu_dfu.c
Boards examplesthat will always override the "dfu_alt_info" via set_dfu_alt_info():
- board/libre-computer/aml-a311d-cc/aml-a311d-cc.c
- board/ti/am62px/evm.c
Since set_dfu_alt_info() is a board specific callback, why can't this logic be implemented for boards that want this behaviour change?
Because I would then need to duplicate the same logic for every board that wanted both USB DFU boot and EFI capsules to work. And the paramters passed in do not allow the function to know the use case (am I DFU booting or updating EFI capsules?). See more below.
I understand that duplicating logic for every board you maintain is not optimal, however, it gives each vendor the freedom of implementing their policy.
I've added a couple of folks who I think could help giving their opinion on EFI capsules/policy.
Heinrich, Ilias, Sugosh, do you have any opinion on this patch?
Regards,
Mattijs
Signed-off-by: Jonathan Humphreys j-humphreys@ti.com
drivers/dfu/dfu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 756569217bb..ab8abae1d89 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -169,10 +169,13 @@ int dfu_init_env_entities(char *interface, char *devstr) dfu_reinit_needed = false; dfu_alt_info_changed = false;
- str_env = env_get("dfu_alt_info");
#ifdef CONFIG_SET_DFU_ALT_INFO
- set_dfu_alt_info(interface, devstr);
- if (!str_env) {
set_dfu_alt_info(interface, devstr);str_env = env_get("dfu_alt_info");- }
#endif
- str_env = env_get("dfu_alt_info"); if (!str_env) { pr_err(""dfu_alt_info" env variable not defined!\n"); return -EINVAL;
-- 2.34.1
Mattijs, thanks for the thorough reply. I did wrestle a lot with how wide of a fix to propose for this problem, and in the end, decided on the narrow fix of simply preventing the overwriting of the variable.
Yes it is a policy change, but the policy is already unclear, inconsistent, and confusing, IMO.
For example:
- EFI capsule update wants to very strictly control the dfu alt values by setting it in set_dfu_alt_info(), but then any other DFU use case breaks. USB DFU boot is now broken.
- The behavior the user sees wrt the dfu_alt_info env variable is very confusing and non-intuitive. Take this example:
=> env set dfu_alt_info "sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000" => env print dfu_alt_info dfu_alt_info=sf 0:0=exe1.bin raw 0 88000;exe2.bin raw 88000 100000 => dfu 0 list DFU alt settings list: dev: SF alt: 0 name: tiboot3.bin layout: RAW_ADDR dev: SF alt: 1 name: tispl.bin layout: RAW_ADDR dev: SF alt: 2 name: u-boot.img layout: RAW_ADDR => env print dfu_alt_info dfu_alt_info=sf 0:0=tiboot3.bin raw 0 80000;tispl.bin raw 80000 200000;u-boot.img raw 280000 400000 =>
As you can see, the user set's the dfu_alt_info variable according to their specific use case, then simply tries to list the DFU alt settings, and because this code goes through the dfu_init_env_entities() path, it gets changed to the EFI capsule settings.
I was hoping to get a simpler fix in now so we can get USB DFU boot working again, and we can visit the overall policy design next. As you suggest, I could also push the testing of overwriting into the board specific set_dfu_alt_info() function, but then I need to duplicate the code in 8 different places for the TI boards, and other vendors may still have the problem.
I agree that the above behaviour is confusing and I'm reconsidering to take up this patch. I'd like some buy-in from either Heinrich, Ilias or Sughosh on this since I'm not 100% confortable with the "policy change"
A little context here. The DFU driver already had this policy in place where, if the CONFIG_SET_DFU_ALT_INFO was set, the dfu_alt_info string would be set by U-Boot, instead of taking the user provided string. It was decided to use this for EFI capsule updates, as getting the string which determines the location of writing the update images from within U-Boot is more resilient than taking some user provided string.
Looking to the longer term solution, here are my thoughts.
- We need to decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. The only reason TI boards are now setting CONFIG_SET_DFU_ALT_INFO is because EFI capsule update is enabled. Outside of a few legacy uses (I think - it appears they were introduced prior to supporting multi-interface dfu alt strings), I think this is true for other vendor's boards as well.
- Have EFI capsule support do as USB DFU boot does today, and set the dfu alt string it wants used *before* initiating the DFU operation. With CONFIG_SET_DFU_ALT_INFO no longer enabled, the value it set will not get overridden.
- Have the actual value of the dfu alt string used in the DFU operation be passed in, rather than read from the dfu_alt_info environment variable. The USB DFU and EFI capsule use case will pass in the dfu alt string they want. The standard 'dfu' command can pass in the value of the dfu_alt_info env variable. Note that this effectively decouples the dfu command from the alt settings that USB DFU boot and EFI capsules use, but I think this is what we want.
I think either of 2) or 3) above can be looked at. Although not sure if 2) will be breaking the current DFU policy.
-sughosh
Thanks for the comments. I have looked into this a bit further and see 2 options we can take for the EFI capsule update use case:
stick with the more traditional approach and do as DFU BOOT does by setting the value of the dfu_alt_info env variable just before initiating the DFU operation. As Ilias sugggested, we could also add a save/restore so this is transparent to other DFU users.
move to a model where we explicitly pass in to the DFU operation the value of dfu_alt_info that we want used. In the normal/legacy DFU use cases, this would involve calling env_get() for the dfu_alt_info env variable and pass that value. I like this approach because it is cleaner and more explicit. However, there are many layers of function calls between the driver of the DFU operation (the one that would decide what dfu_alt_info value to use) and dfu_init_env_entities() where it is used, and passing this info across the function interfaces would involved lots of function interface updates.
I'm curious if there is apetite for 2) or should we go with the more traditional approach in 1).
The above assumes we decouple CONFIG_SET_DFU_ALT_INFO from EFI capsules. For platforms still setting CONFIG_SET_DFU_ALT_INFO, the presumption is that if they override the dfu_alt_info env variable, they know what they are doing.
Decoupling that should be pretty easy. I personally like 2) more since it's much more scalable and doesn't involve saving.restoring values. But I am not sure how big of a task it is
Cheers /Ilias
Thanks Jon
This then allows both USB DFU boot and EFI capsule use cases to work as intended and allows the dfu command to operate on the user defined dfu_alt_info value.
I welcome comments from those that have the history and intended behavior background of the DFU support :).
I do as well. I have taken over maintaince on this subsystem a year ago and have not had much patches/work done on the subsystem. Therefore I'm not as knowledgeable as I would have liked to be. I'm sorry about that.
I also welcome comments on how to proceed for 2025.01. Should we live with USB DFU boot broken until we get the long term fix in, or ok with the patch posted here. The patch posted here does allow for a user to change EFI capsule's dfu alt settings, as Mattijs says, but especially given capsules can be authenticated, I'm not sure how this would be exploited, and if that risk is worse that broken DFU boot.
thank Jon
participants (6)
-
Ilias Apalodimas -
Jon Humphreys -
Jonathan Humphreys
-
Mattijs Korpershoek -
Siddharth Vadapalli -
Sughosh Ganu